Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2403813002:
Do not create NavigationHandles for inactive RenderFrameHosts (Closed)
DescriptionDo not create NavigationHandles for inactive RenderFrameHosts
This CL ensures that no NavigationHandle will be created in a
RenderFrameHost that is no longer active. This also fixes a similar
issue in PlzNavigate, where a renderer that is no longer active could
start a navigation.
BUG=621856
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/ea67b131fe91c553c319964f809c42d06d9786b3
Cr-Commit-Position: refs/heads/master@{#424405}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed commments + Rebase #
Messages
Total messages: 17 (11 generated)
Description was changed from ========== Do not create NavigationHandles for inactive RenderFrameHosts This CL ensures that no NavigationHandle will be created in a RenderFrameHost that is no longer active. This also fixes a similar issue in PlzNavigate, where a renderer that is no longer active could start a navigation. BUG=621856 ========== to ========== Do not create NavigationHandles for inactive RenderFrameHosts This CL ensures that no NavigationHandle will be created in a RenderFrameHost that is no longer active. This also fixes a similar issue in PlzNavigate, where a renderer that is no longer active could start a navigation. BUG=621856 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Do not create NavigationHandles for inactive RenderFrameHosts This CL ensures that no NavigationHandle will be created in a RenderFrameHost that is no longer active. This also fixes a similar issue in PlzNavigate, where a renderer that is no longer active could start a navigation. BUG=621856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Do not create NavigationHandles for inactive RenderFrameHosts This CL ensures that no NavigationHandle will be created in a RenderFrameHost that is no longer active. This also fixes a similar issue in PlzNavigate, where a renderer that is no longer active could start a navigation. BUG=621856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + nasko@chromium.org
@nasko: PTAL
lfg@ recently disabled all navigations from unload handler, so this change seems safe to me. Adding him and dcheng@ for FYI. LGTM https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (left): https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:1307: base::debug::DumpWithoutCrashing(); Why not keep this around instead of the CHECK, in case there are other cases we might have missed? We can convert it to a check after few days of bake time, as crashing the browser process is disruptive. https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:1041: if (!is_active()) Do we want to add the same to all navigation related methods in RFH? For example, DidFailProvisionalLoad/DidFailLoad/etc. https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager_unittest.cc:3162: // The initial RFH receives a DidStartProvisionalLoad IPC. It shoul dnot s/shoul dnot/should not/
Thanks! https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (left): https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:1307: base::debug::DumpWithoutCrashing(); On 2016/10/10 20:41:10, nasko (slow) wrote: > Why not keep this around instead of the CHECK, in case there are other cases we > might have missed? We can convert it to a check after few days of bake time, as > crashing the browser process is disruptive. Done. https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:1041: if (!is_active()) On 2016/10/10 20:41:11, nasko (slow) wrote: > Do we want to add the same to all navigation related methods in RFH? For > example, DidFailProvisionalLoad/DidFailLoad/etc. Added a TODO. https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/2403813002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager_unittest.cc:3162: // The initial RFH receives a DidStartProvisionalLoad IPC. It shoul dnot On 2016/10/10 20:41:11, nasko (slow) wrote: > s/shoul dnot/should not/ Done.
The CQ bit was checked by clamy@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/2403813002/#ps20001 (title: "Addressed commments + Rebase")
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 ========== Do not create NavigationHandles for inactive RenderFrameHosts This CL ensures that no NavigationHandle will be created in a RenderFrameHost that is no longer active. This also fixes a similar issue in PlzNavigate, where a renderer that is no longer active could start a navigation. BUG=621856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Do not create NavigationHandles for inactive RenderFrameHosts This CL ensures that no NavigationHandle will be created in a RenderFrameHost that is no longer active. This also fixes a similar issue in PlzNavigate, where a renderer that is no longer active could start a navigation. BUG=621856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Do not create NavigationHandles for inactive RenderFrameHosts This CL ensures that no NavigationHandle will be created in a RenderFrameHost that is no longer active. This also fixes a similar issue in PlzNavigate, where a renderer that is no longer active could start a navigation. BUG=621856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Do not create NavigationHandles for inactive RenderFrameHosts This CL ensures that no NavigationHandle will be created in a RenderFrameHost that is no longer active. This also fixes a similar issue in PlzNavigate, where a renderer that is no longer active could start a navigation. BUG=621856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/ea67b131fe91c553c319964f809c42d06d9786b3 Cr-Commit-Position: refs/heads/master@{#424405} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ea67b131fe91c553c319964f809c42d06d9786b3 Cr-Commit-Position: refs/heads/master@{#424405} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
