New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 725716 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Main frame commit after cross-process transfer incorrectly overwrites previous history entry

Project Member Reported by alex...@chromium.org, May 24 2017

Issue description

With --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.

 

Comment 1 by creis@chromium.org, May 25 2017

Wow, nice find.  I guess I didn't realize it applied to the main frame at the time!
Project Member

Comment 2 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
Verified that this is working in 61.0.3115.0.

Sign in to add a comment