Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2383773002:
Prevent RFH from attempting to transfer after it's been swapped out (Closed)
DescriptionPrevent RFH from attempting to transfer after it's been swapped out
This CL fixes a race condition in which we will attempt to transfer a
navigation in an RFH that has just been swapped out by the commit of
another navigation.
BUG=651503
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/152381c57c5c04d07b9ebb2496e369ba239a98f7
Cr-Commit-Position: refs/heads/master@{#422972}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Rebase #Patch Set 3 : Addressed comments #
Total comments: 6
Patch Set 4 : Addressed comments #
Dependent Patchsets: Messages
Total messages: 31 (19 generated)
Description was changed from ========== Prevent RFh from attempting to transfer after it's been swapped out This CL fixes a race condition in which we will attempt to transfer a navigation in an RFH that has just been swapped out by the commit of another navigation. BUG=651503 ========== to ========== Prevent RFh from attempting to transfer after it's been swapped out This CL fixes a race condition in which we will attempt to transfer a navigation in an RFH that has just been swapped out by the commit of another navigation. BUG=651503 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by clamy@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...
Description was changed from ========== Prevent RFh from attempting to transfer after it's been swapped out This CL fixes a race condition in which we will attempt to transfer a navigation in an RFH that has just been swapped out by the commit of another navigation. BUG=651503 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Prevent RFH from attempting to transfer after it's been swapped out This CL fixes a race condition in which we will attempt to transfer a navigation in an RFH that has just been swapped out by the commit of another navigation. BUG=651503 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
@nasko, creis: PTAL https://codereview.chromium.org/2383773002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2383773002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.cc:673: return false; This will just suspend the navigation. Alternatively, I could reset the NavigationHandle in the RFH, which would just cancel the navigation. Other option: we delete the NavHandle when swapping out the RFH, this would ensure the navigation will eventually stop & that no updates are sent to the embedder about a navigation in an RFH that is pending deletion.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Mostly looks good, but I want to be sure we cancel the navigation. Also see my question on the bug about the pending RFH case. (I'll mostly defer to Nasko on the TestNavigationManager class changes.) https://codereview.chromium.org/2383773002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2383773002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.cc:671: if (render_frame_host_ != manager->current_frame_host() && We used to use is_active() for this (in OnCrossSiteResponse). That's probably a better check to use here, given that we're getting |manager| from |render_frame_host_| anyway. https://codereview.chromium.org/2383773002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.cc:673: return false; On 2016/09/30 14:06:15, clamy wrote: > This will just suspend the navigation. Alternatively, I could reset the > NavigationHandle in the RFH, which would just cancel the navigation. We definitely want to cancel the navigation. The old code did that by deleting cross_site_transferring_request (unless leak_requests_for_testing_ was true). > Other option: we delete the NavHandle when swapping out the RFH, this would > ensure the navigation will eventually stop & that no updates are sent to the > embedder about a navigation in an RFH that is pending deletion. I like this idea. Can we ensure the navigation gets canceled if the IO thread posts its task and no handle is found? And if we did that, could we put a CHECK(render_frame_host_->is_active()) here?
Great job on tracking this down! https://codereview.chromium.org/2383773002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2383773002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.cc:673: return false; On 2016/09/30 20:25:26, Charlie Reis wrote: > On 2016/09/30 14:06:15, clamy wrote: > > This will just suspend the navigation. Alternatively, I could reset the > > NavigationHandle in the RFH, which would just cancel the navigation. > > We definitely want to cancel the navigation. The old code did that by deleting > cross_site_transferring_request (unless leak_requests_for_testing_ was true). +1 on cancelling the navigation. Keeping it suspended only creates ambiguity. > > Other option: we delete the NavHandle when swapping out the RFH, this would > > ensure the navigation will eventually stop & that no updates are sent to the > > embedder about a navigation in an RFH that is pending deletion. > > I like this idea. Can we ensure the navigation gets canceled if the IO thread > posts its task and no handle is found? And if we did that, could we put a > CHECK(render_frame_host_->is_active()) here? https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... content/public/test/browser_test_utils.cc:1682: if (navigation_paused_in_will_start_) How would that happen? https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... content/public/test/browser_test_utils.cc:1729: // Resume any clients that are waiting for the end of the navigation. Note I'm a bit confused on how we can get multiple message loops to be spinning simultaneously. Maybe land this CL as is to ensure we make branch point, but overall I'd like to understand this a bit better and see if it can be simplified. https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... content/public/test/browser_test_utils.h:619: WARN_UNUSED_RESULT bool WaitForWillProcessResponse(); nit: I think it will be a bit more readable as WaitForResponse. While the lengthy name is fully explanatory of what exact event will cause it, I think the WillProcess part doesn't add as much to be needed. If you go that route, we can rename the above method too in a separate CL. https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... content/public/test/browser_test_utils.h:652: scoped_refptr<MessageLoopRunner> will_process_response_loop_runner_; Why do we need separate MessageLoopRunner?
The CQ bit was checked by clamy@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...
Thanks! https://codereview.chromium.org/2383773002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2383773002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.cc:671: if (render_frame_host_ != manager->current_frame_host() && On 2016/09/30 20:25:26, Charlie Reis wrote: > We used to use is_active() for this (in OnCrossSiteResponse). That's probably a > better check to use here, given that we're getting |manager| from > |render_frame_host_| anyway. Acknowledged. https://codereview.chromium.org/2383773002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.cc:673: return false; On 2016/09/30 23:21:33, nasko (slow) wrote: > On 2016/09/30 20:25:26, Charlie Reis wrote: > > On 2016/09/30 14:06:15, clamy wrote: > > > This will just suspend the navigation. Alternatively, I could reset the > > > NavigationHandle in the RFH, which would just cancel the navigation. > > > > We definitely want to cancel the navigation. The old code did that by > deleting > > cross_site_transferring_request (unless leak_requests_for_testing_ was true). > > +1 on cancelling the navigation. Keeping it suspended only creates ambiguity. > > > > Other option: we delete the NavHandle when swapping out the RFH, this would > > > ensure the navigation will eventually stop & that no updates are sent to the > > > embedder about a navigation in an RFH that is pending deletion. > > > > I like this idea. Can we ensure the navigation gets canceled if the IO thread > > posts its task and no handle is found? And if we did that, could we put a > > CHECK(render_frame_host_->is_active()) here? > Done. The cancellation if the NavigationHandle isn't found has been in place since the previous patch (it was what was causing issues on Android). I've added a check a bit earlier at the start of the check for transfer sequence. https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... content/public/test/browser_test_utils.cc:1682: if (navigation_paused_in_will_start_) On 2016/09/30 23:21:33, nasko (slow) wrote: > How would that happen? We always pause by default in WillStart since on PlzNavigate it may happen synchronously when we start the navigation. Not pausing there all the time would make us miss the event when the user really wants to pause the navigation before it starts. https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... content/public/test/browser_test_utils.cc:1729: // Resume any clients that are waiting for the end of the navigation. Note On 2016/09/30 23:21:33, nasko (slow) wrote: > I'm a bit confused on how we can get multiple message loops to be spinning > simultaneously. Maybe land this CL as is to ensure we make branch point, but > overall I'd like to understand this a bit better and see if it can be > simplified. Well we shouldn't have them spinning, and this would probably be better expressed as else if statements. I agree that it should be improved, but would like to land this before branch point :). https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... content/public/test/browser_test_utils.h:619: WARN_UNUSED_RESULT bool WaitForWillProcessResponse(); On 2016/09/30 23:21:33, nasko (slow) wrote: > nit: I think it will be a bit more readable as WaitForResponse. While the > lengthy name is fully explanatory of what exact event will cause it, I think the > WillProcess part doesn't add as much to be needed. > > If you go that route, we can rename the above method too in a separate CL. Acknowledged. Since more of the code needs to be refactored, I'll do a followup CL to improve the TestNavigationManager. https://codereview.chromium.org/2383773002/diff/1/content/public/test/browser... content/public/test/browser_test_utils.h:652: scoped_refptr<MessageLoopRunner> will_process_response_loop_runner_; On 2016/09/30 23:21:33, nasko (slow) wrote: > Why do we need separate MessageLoopRunner? I added another one since it meant the least amount of modifications to the existing code, and wanted to land this ASAP. The code can probably be refactored, but to use only one MessageLoopRunner we'd need to keep track of the current & desired states. I'd rather do this in another patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Nasko: there seems to be an issue with the WebNavigation API tests. We expect the commit of the new navigation in the pending RFH to arrive before the end of the failed navigation in the current RFH, but with this patch the order switches. I can try to reset the NavigationHandle later (but is less handy to do so), or we can modify the test expectations.
On 2016/10/04 12:45:49, clamy wrote: > Nasko: there seems to be an issue with the WebNavigation API tests. We expect > the commit of the new navigation in the pending RFH to arrive before the end of > the failed navigation in the current RFH, but with this patch the order > switches. I can try to reset the NavigationHandle later (but is less handy to do > so), or we can modify the test expectations. As discussed offline, I'm fine with the reordering, as it will make it consistent in cross-process navigations and same-process ones.
Thanks for the quick fix! LGTM https://codereview.chromium.org/2383773002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2383773002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:648: // A navigation form a renderer that is no longer active should not attempt to minor nit: s/renderer/RenderFrame/ https://codereview.chromium.org/2383773002/diff/40001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2383773002/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:8234: RFHTransfersWhilePendingDeletion) { I think this makes more sense to be in render_frame_host_manager_browsertests.cc instead of here. However, let's move it along with the other followup items from this CL. https://codereview.chromium.org/2383773002/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:8255: TestNavigationManager same_site_manager(shell()->web_contents(), nit: same_site_manager is inaccurate, since the navigation is cross-site (a.com -> c.com).
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2383773002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2383773002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:648: // A navigation form a renderer that is no longer active should not attempt to On 2016/10/04 16:07:39, nasko (slow) wrote: > minor nit: s/renderer/RenderFrame/ Done. https://codereview.chromium.org/2383773002/diff/40001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2383773002/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:8234: RFHTransfersWhilePendingDeletion) { On 2016/10/04 16:07:39, nasko (slow) wrote: > I think this makes more sense to be in render_frame_host_manager_browsertests.cc > instead of here. However, let's move it along with the other followup items from > this CL. Acknowledged. https://codereview.chromium.org/2383773002/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:8255: TestNavigationManager same_site_manager(shell()->web_contents(), On 2016/10/04 16:07:39, nasko (slow) wrote: > nit: same_site_manager is inaccurate, since the navigation is cross-site (a.com > -> c.com). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! LGTM, too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2383773002/#ps60001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Prevent RFH from attempting to transfer after it's been swapped out This CL fixes a race condition in which we will attempt to transfer a navigation in an RFH that has just been swapped out by the commit of another navigation. BUG=651503 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Prevent RFH from attempting to transfer after it's been swapped out This CL fixes a race condition in which we will attempt to transfer a navigation in an RFH that has just been swapped out by the commit of another navigation. BUG=651503 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Prevent RFH from attempting to transfer after it's been swapped out This CL fixes a race condition in which we will attempt to transfer a navigation in an RFH that has just been swapped out by the commit of another navigation. BUG=651503 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Prevent RFH from attempting to transfer after it's been swapped out This CL fixes a race condition in which we will attempt to transfer a navigation in an RFH that has just been swapped out by the commit of another navigation. BUG=651503 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/152381c57c5c04d07b9ebb2496e369ba239a98f7 Cr-Commit-Position: refs/heads/master@{#422972} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/152381c57c5c04d07b9ebb2496e369ba239a98f7 Cr-Commit-Position: refs/heads/master@{#422972} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
