Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1738233002:
Disallow was_within_same_page = true for a cross-process navigation (Closed)
|
Created:
4 years, 9 months ago by gzobqq Modified:
4 years, 9 months ago CC:
darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisallow was_within_same_page = true for a cross-process navigation.
BUG=590284
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/8be1ff11dc1fae61146dbcfaa38e12314d290dca
Cr-Commit-Position: refs/heads/master@{#378461}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Cleaner approach by creis #Patch Set 3 : Fix is_navigation_within_page #
Total comments: 10
Patch Set 4 : Address comments #
Messages
Total messages: 32 (17 generated)
Description was changed from ========== Disallow was_within_same_page = true for a navigation that swaps RFH BUG=590284 ========== to ========== Disallow was_within_same_page = true for a navigation that swaps RFH BUG=590284 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Disallow was_within_same_page = true for a navigation that swaps RFH BUG=590284 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Disallow was_within_same_page = true for a navigation that swaps RFH BUG=590284 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
gzobqq@gmail.com changed reviewers: + clamy@chromium.org
Work in progress. Not sure if this is the correct way to fix this. Need to do more thinking.
creis@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
[+nasko, since I'll be OOO after today] Yes, it's bad that we're allowing a cross-process navigation to claim that it's in-page. Some thoughts on a different way to fix it below. Hoping Nasko can follow up after I disappear. https://codereview.chromium.org/1738233002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1738233002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:877: params.url, params.was_within_same_page, rfh, false); This looks odd at first glance (since we do get here for cross-process navigations), but we don't seem to need the check in this case because we get here after committing the pending/speculative RFH. That seems like another reason to just put the check in NavigatorImpl::DidNavigate. https://codereview.chromium.org/1738233002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1738233002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:468: params.url, params.was_within_same_page, render_frame_host, will_swap); What if we left IsURLInPageNavigation as is and just verified that is_navigation_within_page can only be true if render_frame_host == render_frame_host->frame_tree_node()->render_manager()->current_host()? If the RFH isn't the current one, we should kill it here (and proceed with committing / showing the sad tab). https://codereview.chromium.org/1738233002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:515: } Should we also verify that |render_frame_host| is the current RFH after calling RFHM::DidNavigateFrame, and kill it if not?
Description was changed from ========== Disallow was_within_same_page = true for a navigation that swaps RFH BUG=590284 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Disallow was_within_same_page = true for a cross-process navigation. BUG=590284 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
gzobqq@gmail.com changed reviewers: + phajdan.jr@chromium.org
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
nasko, PTAL phajdan.jr, PTAL at content/test https://codereview.chromium.org/1738233002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1738233002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:468: params.url, params.was_within_same_page, render_frame_host, will_swap); On 2016/02/26 20:11:58, Charlie Reis (OOO til Mar 7) wrote: > What if we left IsURLInPageNavigation as is and just verified that > is_navigation_within_page can only be true if render_frame_host == > render_frame_host->frame_tree_node()->render_manager()->current_host()? If the > RFH isn't the current one, we should kill it here (and proceed with committing / > showing the sad tab). I agree, this is cleaner. https://codereview.chromium.org/1738233002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:515: } On 2016/02/26 20:11:58, Charlie Reis (OOO til Mar 7) wrote: > Should we also verify that |render_frame_host| is the current RFH after calling > RFHM::DidNavigateFrame, and kill it if not? From my reading of DidNavigateFrame, it doesn't seem to change current_frame_host() if render_frame_host == current_frame_host(). Shall I add a double check?
The CQ bit was checked by gzobqq@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1738233002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1738233002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/1738233002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/1738233002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:1160: // on the browser side and the renderer should be killed. nit: s/renderer/renderer process/ in first and last line of the comment. https://codereview.chromium.org/1738233002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:1174: main_test_rfh()->SendBeforeUnloadACK(true); Why not call PrepareForCommit? It should abstract all the details of beforeunload ack and possibly other parts of the state machine. https://codereview.chromium.org/1738233002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:1181: int bad_msg_count = process()->bad_msg_count(); Is process() the right one to use? Shouldn't the bad message be associated with the speculative_rfh, which is in a different process? https://codereview.chromium.org/1738233002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1738233002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1473: CHECK(!owner_delegate_); This looks like unrelated change. If it isn't, let's not put it in this CL. https://codereview.chromium.org/1738233002/diff/80001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/1738233002/diff/80001/content/test/test_rende... content/test/test_render_frame_host.cc:274: if (!IsBrowserSideNavigationEnabled()) Why is browser-side navigation different here?
Thanks for the review, PTAL https://codereview.chromium.org/1738233002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/1738233002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:1160: // on the browser side and the renderer should be killed. On 2016/03/01 00:27:01, nasko wrote: > nit: s/renderer/renderer process/ in first and last line of the comment. Done. https://codereview.chromium.org/1738233002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:1174: main_test_rfh()->SendBeforeUnloadACK(true); On 2016/03/01 00:27:01, nasko wrote: > Why not call PrepareForCommit? It should abstract all the details of > beforeunload ack and possibly other parts of the state machine. That's nice, done. https://codereview.chromium.org/1738233002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:1181: int bad_msg_count = process()->bad_msg_count(); On 2016/03/01 00:27:01, nasko wrote: > Is process() the right one to use? Shouldn't the bad message be associated with > the speculative_rfh, which is in a different process? process() should be good, it has logic to select the speculative RFH if one exists. https://codereview.chromium.org/1738233002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1738233002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1473: CHECK(!owner_delegate_); On 2016/03/01 00:27:01, nasko wrote: > This looks like unrelated change. If it isn't, let's not put it in this CL. It makes sure that a RWHI isn't deleted while owned by a scoped_ptr in RVHI. This could happen without the additional validation of was_within_same_page in this CL. So it's not completely unrelated, but not very related either. At first I thought it would be a good additional check to merge, but now I see that in 48 stable, RVHI still is-a RWHI and RWHI::Destroy is used even with owner_delegate_. So can't merge it to stable. I'll create a separate CL for this. I think it's a good CHECK to have. It could have prevented the UaF since chrome 49, beta. Even with this CL, the RWHI ownership logic is complicated and there might be more paths leading to this UaF. https://codereview.chromium.org/1738233002/diff/80001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/1738233002/diff/80001/content/test/test_rende... content/test/test_render_frame_host.cc:274: if (!IsBrowserSideNavigationEnabled()) On 2016/03/01 00:27:01, nasko wrote: > Why is browser-side navigation different here? With browser-side navigation RFHI::OnDidStartLoading(true) does nothing but ReceivedBadMessage(), which interferes with the new test. Renderer also sends FrameHostMsg_DidStartLoading only when !IsBrowserSideNavigationEnabled() || !to_different_document. It was a recent change by clamy in https://codereview.chromium.org/1608283002
LGTM
The CQ bit was checked by gzobqq@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1738233002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1738233002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gzobqq@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1738233002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1738233002/100001
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 gzobqq@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1738233002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1738233002/100001
Message was sent while issue was closed.
Description was changed from ========== Disallow was_within_same_page = true for a cross-process navigation. BUG=590284 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Disallow was_within_same_page = true for a cross-process navigation. BUG=590284 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Disallow was_within_same_page = true for a cross-process navigation. BUG=590284 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Disallow was_within_same_page = true for a cross-process navigation. BUG=590284 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/8be1ff11dc1fae61146dbcfaa38e12314d290dca Cr-Commit-Position: refs/heads/master@{#378461} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8be1ff11dc1fae61146dbcfaa38e12314d290dca Cr-Commit-Position: refs/heads/master@{#378461} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
