Main frame commit after cross-process transfer incorrectly overwrites previous history entry |
||
Issue descriptionWith --site-per-process (not required, see below): (1) Go to http://csreis.github.io/tests/window-open.html (2) Click "Open about:blank window" (to keep the process alive) (3) From devtools, execute: location = "http://www.asdf.com" (4) From devtools, execute: location = "http://csreis.github.io" (5) Go back. What should happen? We go back to asdf.com. What happens instead? We skip asdf and go back to http://csreis.github.io/tests/window-open.html. --site-per-process isn't really required here; this should repro with any cross-process transfer. I originally reproed this while working on isolated origins in https://codereview.chromium.org/2831683002/ (in IsolatedOriginTest.MainFrameNavigation). AFAICT, the problem is that the navigation in (4) overwrites the asdf history entry, because FrameLoader::DetermineFrameLoadType thinks that state_machine_.CommittedMultipleRealLoads() is false, and the document's current URL is about:blank (which it is), so it sets the frame load type to kFrameLoadTypeReplaceCurrentItem. CommittedMultipleRealLoads() should really be true here, and we've got this code in RenderFrameImpl::NavigateInternal (added by Charlie in r332070 to fix issue 464014 ) to prevent exactly this scenario (incorrectly assuming no previously committed loads on a cross-process transfer): // If this frame isn't in the same process as the main frame, it may naively // assume that this is the first navigation in the iframe, but this may not // actually be the case. Inform the frame's state machine if this frame has // already committed other loads. if (request_params.has_committed_real_load && frame_->Parent()) frame_->SetCommittedFirstRealLoad(); BUT, notice that it gets skipped for main frames! It seems to me that we should be propagating has_committed_real_load for main frames as well, and removing the "frame_->Parent()" gets the repro to work properly. Charlie - do you agree? I can take this since I've already poked at this quite a bit and have a test almost written.
,
May 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c5e91fafd0be6f67afc2dd41d33c36492c5ffb43 commit c5e91fafd0be6f67afc2dd41d33c36492c5ffb43 Author: alexmos <alexmos@chromium.org> Date: Thu May 25 02:55:01 2017 Use has_committed_real_load for main frames in addition to subframes. This fixes a bug where a remote-to-local main frame navigation overwrote the previous history entry. BUG= 725716 Review-Url: https://codereview.chromium.org/2901093006 Cr-Commit-Position: refs/heads/master@{#474536} [modify] https://crrev.com/c5e91fafd0be6f67afc2dd41d33c36492c5ffb43/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/c5e91fafd0be6f67afc2dd41d33c36492c5ffb43/content/renderer/render_frame_impl.cc
,
May 30 2017
Verified that this is working in 61.0.3115.0. |
||
►
Sign in to add a comment |
||
Comment 1 by creis@chromium.org
, May 25 2017