Different replacement behavior for main frames created via CreateMainFrame vs CreateProvisional |
|
Issue descriptionWhat steps will reproduce the problem? (1) Start Chrome with --site-per-process (2) Go to http://csreis.github.io (3) window.open("about:blank", "foo") (4) From the new foo tab, window.open("about:blank", "bar") (5) Navigate the bar tab to https://csreis.github.io via omnibox. Notice that the back button is enabled - you can go back to about:blank, as expected. (6) Switch to the foo tab and navigate that to https://csreis.github.io. This time the back button is disabled and you can't go back, even though this should be the same as step (5)! The difference between (5) and (6) is that in (6), we've got a proxy already created for the foo tab in the https:// SiteInstance, so the main frame will be created via RenderFrameImpl::CreateFrame -> CreateProvisional path. Whereas in (5), it was created via RenderViewImpl::Initialize -> RenderFrameImpl::CreateMainFrame. The different behavior appears to trigger in FrameLoader::DetermineFrameLoadType, here: if (request.ReplacesCurrentItem() || (!state_machine_.CommittedMultipleRealLoads() && DeprecatedEqualIgnoringCase(frame_->GetDocument()->Url(), BlankURL()))) { return kFrameLoadTypeReplaceCurrentItem; } In both cases, request.ReplacesCurrentItem() is false. Also, in both cases we're passing in request_params.has_committed_real_load as false to RenderFrameImpl::NavigateInternal(), and state_machine_.CommittedMultipleRealLoads() is false in both. But, it appears that with CreateMainFrame, we end up with the initial document's URL being empty, whereas with CreateProvisional, we end up with the initial document's URL being "about:blank". So then with CreateProvisional we go down this path because its URL matches BlankURL(). CreateProvisional sets the document URL to about:blank when constructing the initial empty Document, because initializer.ShouldSetURL() returns true. ShouldSetURL() looks like this: bool DocumentInit::ShouldSetURL() const { LocalFrame* frame = FrameForSecurityContext(); return (frame && frame->Owner()) || !url_.IsEmpty(); } That thing return true, because all provisional frames have a DummyFrameOwner, even main frame ones (see comment in WebLocalFrameImpl::CreateProvisional). Sigh. I found this as part of working on issue 756790, where a test (RenderFrameHostManagerTest.PopupPendingAndBackToSameSiteInstance) got switched from the CreateMainFrame to the CreateProvisional codepath. Daniel/Nate - why does ShouldSetURL need to check the Owner() at all? Nate/Charlie - IIUC, because window.open() explicitly specified "about:blank", we should be able to go back to it after another navigation, right? I know Nate was working on some of this logic. The whole flow of how we avoid replacement in the working case seems fragile. Is this how it's intended to work, or should we be passing information to the https renderer in NavigateInternal, saying that the new frame already committed an explicit "about:blank" navigation beyond the initial one, so that we can go back to it? Currently, FrameTreeNode::has_committed_real_load() ignores all navigations to about:blank.
,
Sep 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/27caae83cb530daaf49f9a38793e427cdf493a65 commit 27caae83cb530daaf49f9a38793e427cdf493a65 Author: Alex Moshchuk <alexmos@chromium.org> Date: Mon Sep 11 23:11:18 2017 Always create a proxy for cross-process navigations. Previously, if a frame F navigated cross-process from a.com to b.com, and this was the first navigation to the b.com process in that BrowsingInstance, we did the following: 1. Create proxies for all frames in FrameTrees on F's opener chain. 2. Create proxies for all frames in F's FrameTree, except for F and its subtree. 3. If F is a main frame: 3a. Create a new active RenderView, which also creates the main frame, already swapped into the tree even though it's still pending and may never commit. Otherwise, if F is a cross-process subframe: 3b. send a CreateFrame IPC, referencing the parent proxy, which should've been created in step 2. This flow breaks down if, while the navigation in F is still pending, other frames in the same BrowsingInstance also navigate to b.com and then try to interact with F (where the current RFH is still at a.com). For example, if F is a main frame, and a subframe of F at a.com navigates to b.com, the subframe won't find the parent proxy for F, because we didn't create one in step 3a. This crashes on CHECK(parent) in CreateFrame(). Furthermore, if the subframe commits first, it should be able to reference F. For example, if the subframe sends a postMessage to F, that should go to F's proxy in b.com, which should forward it to F's current RenderFrame, which is still in a.com. So there should be a proxy created for F in step 3a. This is especially important if F never commits. The same kind of race is possible with openers: a frame at C might window.open() a new tab, then the opener frame starts a cross-process navigation to D, then the opened tab also starts a cross-process navigation to D. If the opener's navigation never succeeds but the opened tab's does, the opened tab at D should still be able to communicate with its old opener at C. To address these races, this CL changes step 2, so that when a frame navigates cross-process, we pre-create a proxy for that frame in the new SiteInstance for cases where other frames might also navigate to the same new SiteInstance and script this frame. This switches the frame creation/navigation in steps 3a or 3b to instead follow the remote-to-local navigation path, i.e., sending the CreateFrame IPC and using the existing-proxy (CreateProvisional) flow. As part of the fix, RenderViewCreated is changed to also be generated for RVHs created for provisional main frame navigations, and RenderViewHostImpl::GetMainFrame() will also return pending main frame RFH if it exists. This keeps the embedder's assumptions about RenderViewHost valid. Longer-term, these APIs should be deprecated and migrated to use RenderFrameCreated. This followup work will be tracked in issue 763548. Bug: 756790, 626802, 591478, 760403 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I6b85d135ec1e507110a51c9fe1b2a05754633a50 Reviewed-on: https://chromium-review.googlesource.com/636786 Reviewed-by: Emily Stark <estark@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Jialiu Lin <jialiul@chromium.org> Reviewed-by: Charlie Reis (slow) <creis@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#501081} [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/chrome/browser/ssl/insecure_sensitive_input_driver_unittest.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/chrome/browser/ui/browser_browsertest.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/frame_host/frame_tree.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/frame_host/frame_tree.h [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/frame_host/frame_tree_unittest.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/renderer_host/render_view_host_delegate.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/renderer_host/render_view_host_delegate.h [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/renderer_host/render_view_host_impl.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/renderer_host/render_view_host_impl.h [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/security_exploit_browsertest.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/web_contents/web_contents_impl.h [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/browser/web_contents/web_contents_impl_browsertest.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/public/test/test_renderer_host.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/public/test/web_contents_tester.h [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/renderer/render_frame_impl.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/test/test_web_contents.cc [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/content/test/test_web_contents.h [modify] https://crrev.com/27caae83cb530daaf49f9a38793e427cdf493a65/third_party/WebKit/Source/core/dom/DocumentInit.cpp
,
Sep 20 2017
|
|
►
Sign in to add a comment |
|
Comment 1 by alex...@chromium.org
, Sep 1 2017Owner: alex...@chromium.org
Status: Started (was: Available)