Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738643002:
Implement error page commit policy in PlzNavigate. (Closed)
DescriptionImplement error page commit policy in PlzNavigate.
This CL implements a new policy for which process do error pages commit.
When an error page is a result of a blocked request, it should be
committed in the same process as the document requesting the navigation.
Otherwise the error page should be committed in the process that would
render the destination URL.
BUG=685074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2738643002
Cr-Commit-Position: refs/heads/master@{#457532}
Committed: https://chromium.googlesource.com/chromium/src/+/f198985c0ffa2f7c4d75e001b14d2ac8c9300ae9
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fixes based on Charlie's review, also reordering a failing test case ... sigh! #Patch Set 3 : Fix from https://codereview.chromium.org/2739193004/ #Patch Set 4 : Fix with reordered webRequestBlocking test cases. #Patch Set 5 : Rebase #
Total comments: 2
Patch Set 6 : Add comment. #
Messages
Total messages: 41 (25 generated)
Description was changed from ========== Implement error page commit policy in PlzNavigate. This CL implements a new policy for which process do error pages commit. When an error page is a result of a blocked request, it should be committed in the same process as the document requesting the navigation. Otherwise the error page should be committed in the process that would render the destination URL. BUG=685074 ========== to ========== Implement error page commit policy in PlzNavigate. This CL implements a new policy for which process do error pages commit. When an error page is a result of a blocked request, it should be committed in the same process as the document requesting the navigation. Otherwise the error page should be committed in the process that would render the destination URL. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
nasko@chromium.org changed reviewers: + clamy@chromium.org, creis@chromium.org
Hey everyone, This CL implements the error page commit policy we discussed today with creis@ and alexmos@. Once it lands, it should allow us to make progress on Arthur's frame-src CL. Can you review it for me? Thanks in advance! Nasko https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1077: ErrorPageBlockedNavigation) { I put these tests here, since it has the TestNavigationThrottleInstaller helper objects. It belongs probably more to browser_side_navigation_browsertest.cc, but that requires a bit of moving code around. I want to keep this CL small, so I'm leaving it here for now and will move it later. https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1096: shell()->web_contents(), NavigationThrottle::BLOCK_REQUEST, This tests only WillStartRequest, as blocking during redirect is done in Arthur's CL. I will add this test once his CL lands as part of moving this to browser_side_navigation_browsertest.cc.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:583: if (net_error == net::ERR_BLOCKED_BY_CLIENT) { It turns out that the webRequest API blocking navigations go through here as well. A small patch I tried locally to only take this branch on calls originating internally in NavigationRequest (On*ChecksComplete) passed the test.
Thanks! https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1092: SiteInstance* site_instance = scoped_refptr would be safer (since in theory it could go away) https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1132: SiteInstance* site_instance = Same here. https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:583: if (net_error == net::ERR_BLOCKED_BY_CLIENT) { On 2017/03/07 07:08:12, nasko (slow) wrote: > It turns out that the webRequest API blocking navigations go through here as > well. A small patch I tried locally to only take this branch on calls > originating internally in NavigationRequest (On*ChecksComplete) passed the test. What's the problem with using the current RFH for webRequest API blocking cases? Those seem like they're more in the first category than the second, since a reload wouldn't be expected to work. Would it be better to find a way to update the ExtensionWebRequestApiTest.WebRequestBlocking test for the new expectation? (I'm not fully opposed to your workaround suggestion, but I like how simple this is at the moment.)
Just a quick update without any changes to code. On 2017/03/09 00:43:34, Charlie Reis (OOO til Mar 8) wrote: > Thanks! > > https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... > File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): > > https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... > content/browser/frame_host/navigation_handle_impl_browsertest.cc:1092: > SiteInstance* site_instance = > scoped_refptr would be safer (since in theory it could go away) > > https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... > content/browser/frame_host/navigation_handle_impl_browsertest.cc:1132: > SiteInstance* site_instance = > Same here. > > https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... > File content/browser/frame_host/navigation_request.cc (right): > > https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... > content/browser/frame_host/navigation_request.cc:583: if (net_error == > net::ERR_BLOCKED_BY_CLIENT) { > On 2017/03/07 07:08:12, nasko (slow) wrote: > > It turns out that the webRequest API blocking navigations go through here as > > well. A small patch I tried locally to only take this branch on calls > > originating internally in NavigationRequest (On*ChecksComplete) passed the > test. > > What's the problem with using the current RFH for webRequest API blocking cases? > Those seem like they're more in the first category than the second, since a > reload wouldn't be expected to work. I don't think there is actually a problem with behavior. I moved one test case of a test around in the webRequestBlocking test and it passed just fine. I suspect some test expectation or dependency on subtle behavior. > Would it be better to find a way to update the > ExtensionWebRequestApiTest.WebRequestBlocking test for the new expectation? Yes, this is what I'm trying to achieve, but I failed today due to the complexity of the webRequest API test harness : (. > (I'm not fully opposed to your workaround suggestion, but I like how simple this > is at the moment.) I like the simplicity too :). I want to see if I can solve it tomorrow without making it more complex. If not, I will do the workaround to unblock the rest of the CLs from Arthur and follow up async.
Fixes based on Charlie's review, also reordering a failing test case ... sigh!
https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1092: SiteInstance* site_instance = On 2017/03/09 00:43:34, Charlie Reis (OOO til Mar 8) wrote: > scoped_refptr would be safer (since in theory it could go away) Done. https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1132: SiteInstance* site_instance = On 2017/03/09 00:43:34, Charlie Reis (OOO til Mar 8) wrote: > Same here. Done. https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2738643002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:583: if (net_error == net::ERR_BLOCKED_BY_CLIENT) { On 2017/03/09 00:43:34, Charlie Reis (OOO til Mar 8) wrote: > On 2017/03/07 07:08:12, nasko (slow) wrote: > > It turns out that the webRequest API blocking navigations go through here as > > well. A small patch I tried locally to only take this branch on calls > > originating internally in NavigationRequest (On*ChecksComplete) passed the > test. > > What's the problem with using the current RFH for webRequest API blocking cases? > Those seem like they're more in the first category than the second, since a > reload wouldn't be expected to work. > > Would it be better to find a way to update the > ExtensionWebRequestApiTest.WebRequestBlocking test for the new expectation? > > (I'm not fully opposed to your workaround suggestion, but I like how simple this > is at the moment.) I've done a lot of debugging but cannot see why this CL makes the test time out. It boils down to a callback not firing, which leaves ref-count unbalanced and therefore it cannot finish. Interestingly enough, if I just move around one test case, it works fine :(. Since I'm all out of ideas, I will actually try the move this test case and enlist the help of rdevlin.cronin@ debugging this further.
nasko@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Adding rdevlin.cronin@ for extensions API test changes.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
content/ LGTM! I'll defer to Devlin on the extension test.
On 2017/03/09 07:24:54, nasko (slow) wrote: > Adding rdevlin.cronin@ for extensions API test changes. Just to keep other folks in the loop, Nasko and I are debugging this weirdness in hopes of finding out what changed and why.
On 2017/03/09 23:12:37, Devlin wrote: > On 2017/03/09 07:24:54, nasko (slow) wrote: > > Adding rdevlin.cronin@ for extensions API test changes. > > Just to keep other folks in the loop, Nasko and I are debugging this weirdness > in hopes of finding out what changed and why. I managed to find the root cause of the failure and it is indeed due to the changes in this CL. I will think of a solution and describe the problem at a later point in time, since I need to finish some other high priority items.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/09 23:51:37, nasko (slow) wrote: > On 2017/03/09 23:12:37, Devlin wrote: > > On 2017/03/09 07:24:54, nasko (slow) wrote: > > > Adding rdevlin.cronin@ for extensions API test changes. > > > > Just to keep other folks in the loop, Nasko and I are debugging this weirdness > > in hopes of finding out what changed and why. > > I managed to find the root cause of the failure and it is indeed due to the > changes in this CL. I will think of a solution and describe the problem at a > later point in time, since I need to finish some other high priority items. I filed a bug for the root cause: https://crbug.com/700514 and have a CL with fix: https://codereview.chromium.org/2739193004/.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I have tried to fix the underlying issue, but it proves to be non-trivial and might take a while. I'm going to proceed with reordering the test cases and restore them back to normal once I fix the SiteProcessMap issue. Devlin, is that ok with you?
On 2017/03/16 18:02:56, nasko (slow) wrote: > I have tried to fix the underlying issue, but it proves to be non-trivial and > might take a while. I'm going to proceed with reordering the test cases and > restore them back to normal once I fix the SiteProcessMap issue. > > Devlin, is that ok with you? Yeah. I don't love it, but I feel better knowing it's an existing bug, rather than something we caused here. This is fine as a band-aid. :) lgtm https://codereview.chromium.org/2738643002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrequest/test_blocking.js (right): https://codereview.chromium.org/2738643002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrequest/test_blocking.js:96: // see the subresources. add something like: TODO(nasko): This test was reordered in order to accommodate an unrelated failure (crbug.com/nnn). Once that failure is fixed, the reorder should be reverted.
https://codereview.chromium.org/2738643002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/webrequest/test_blocking.js (right): https://codereview.chromium.org/2738643002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/webrequest/test_blocking.js:96: // see the subresources. On 2017/03/16 18:38:14, Devlin wrote: > add something like: > TODO(nasko): This test was reordered in order to accommodate an unrelated > failure (crbug.com/nnn). Once that failure is fixed, the reorder should be > reverted. Done.
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2738643002/#ps100001 (title: "Add comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489690076927900,
"parent_rev": "510e3b3a9e0d1fb0a132d9f3c2e762e6516b0bb8", "commit_rev":
"f198985c0ffa2f7c4d75e001b14d2ac8c9300ae9"}
Message was sent while issue was closed.
Description was changed from ========== Implement error page commit policy in PlzNavigate. This CL implements a new policy for which process do error pages commit. When an error page is a result of a blocked request, it should be committed in the same process as the document requesting the navigation. Otherwise the error page should be committed in the process that would render the destination URL. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement error page commit policy in PlzNavigate. This CL implements a new policy for which process do error pages commit. When an error page is a result of a blocked request, it should be committed in the same process as the document requesting the navigation. Otherwise the error page should be committed in the process that would render the destination URL. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2738643002 Cr-Commit-Position: refs/heads/master@{#457532} Committed: https://chromium.googlesource.com/chromium/src/+/f198985c0ffa2f7c4d75e001b14d... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/f198985c0ffa2f7c4d75e001b14d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
