New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 838406 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug
Proj-Servicification

Blocking:
issue 721414



Sign in to add a comment

Network Servicification with Extensions: Navigation Requests don't have -1 render_process_id.

Project Member Reported by karandeepb@chromium.org, Apr 30 2018

Issue description

- Patch in https://chromium-review.googlesource.com/c/chromium/src/+/1036477.
- Run out/Default/browser_tests --gtest_filter="ExtensionApiTest.YahooNavigation" with and without Network service.
- See the test fail crash with network service.

Navigation network requests are initiated by the browser and should have a render process id of -1.

This is the cause behind failure of LocalNTPInterceptionWebRequestAPITest.OneGoogleBarRequestsHidden which https://chromium.googlesource.com/chromium/src/+/5d11c9e146eeba027517ac40816839d0f1bc5416 introduced.
 
Blocking: 721414
Cc: roc...@chromium.org jam@chromium.org
Components: Platform>Extensions Internals>Services>Network
+John and Ken. See the trybot failure. Let me know if anything isn't clear or if this is somehow WAI.

Comment 2 by jam@chromium.org, May 2 2018

Owner: karandeepb@chromium.org
Status: Assigned (was: Untriaged)
Can you please provide more information? i.e. which variable in which struct is not -1.
The linked in CL (https://chromium-review.googlesource.com/c/chromium/src/+/1036477) does show some of it. So for navigation network requests, WebRequestInfo::render_process_id is not -1 in the network service path (unlike the non-network service path). Hence clients can think that the request is coming from a renderer. 

I didn't try it, but following the code, it seems the code path that should be responsible for this should be at https://cs.chromium.org/chromium/src/content/browser/loader/navigation_url_loader_network_service.cc?type=cs&sq=package:chromium&l=1258. Maybe we can just pass a render_process_id of -1 in WebRequestAPI::MaybeProxyURLLoaderFactory when |is_navigation| is true. 

But somebody who understands the WillCreateURLLoaderFactory(frame, is_navigation, factory_request) interface more should own this. For example, from a preliminary look, it's not clear to me what does the |frame| denote when |is_navigation| is true. 

Comment 4 by jam@chromium.org, May 3 2018

Owner: jam@chromium.org
Status: Started (was: Assigned)
thanks for the repro and info, that's very helpful.
Project Member

Comment 5 by bugdroid1@chromium.org, May 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/639c214c00188808bbe82546ff4aacc96fc39776

commit 639c214c00188808bbe82546ff4aacc96fc39776
Author: John Abd-El-Malek <jam@chromium.org>
Date: Fri May 04 14:16:12 2018

Fix render_process_id and render_frame_id being specified with webRequest for navigations.

Bug:  838406 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I19a1e40a87bad542c99e3c0abe83559dff9a4199
Reviewed-on: https://chromium-review.googlesource.com/1042807
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556044}
[modify] https://crrev.com/639c214c00188808bbe82546ff4aacc96fc39776/extensions/browser/api/web_request/web_request_api.cc
[modify] https://crrev.com/639c214c00188808bbe82546ff4aacc96fc39776/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Comment 6 by jam@chromium.org, May 4 2018

Status: Fixed (was: Started)

Sign in to add a comment