New issue
Advanced search Search tips

Issue 628286 link

Starred by 1 user
Status: Fixed
Owner:
creis@chromium.org
Closed: Jul 2016
Cc:
a...@chromium.org
japhet@chromium.org
dcheng@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug
Proj-Servicification

Blocking:
issue 585194



Sign in to add a comment

Subframe HistoryItem state lost on back/forward with recreated frames

Project Member Reported by creis@chromium.org, Jul 14 2016 
Version: 54.0.2794.0 and earlier
OS: All

What steps will reproduce the problem?
(1) Navigate to http://csreis.github.io/tests/cross-site-iframe.html
(2) Click "Go cross-site (complex page)"
(3) Scroll to the bottom of the iframe.
(4) Navigate to http://csreis.github.io/tests/, destroying the iframe.
(5) Go back.

What is the expected output?

The iframe should be scrolled to the bottom, which is what happens if you replace step 4 with:
(4) Click "Go same-site"

What do you see instead?

The iframe is not scrolled to the bottom.  Other state is lost as well, such as the document sequence number.

This bug is present in default Chrome as well as OOPIF-enabled modes.

Comment 1 by creis@chromium.org, Jul 14 2016 

The bug is in FrameLoader::setHistoryItemStateForCommit, which checks for BackForwardCommit but not the case of a history commit in a newly created subframe.  I'm guessing this bug has been around for a long time.  (That's in contrast to similar uses of isBackForwardLoadType, which checks for both cases.)

I found this when working on the fix for  issue 585194 , since the document sequence number wasn't being preserved when going back.

One challenge is that we can't just write a similar isBackForwardCommitType, because HistoryCommitType combines FrameLoadTypeInitialHistoryLoad and the non-history FrameLoadTypeInitialInChildFrame into a single InitialCommitInChildFrame.  We can't tell whether it was a history commit or not.

I'm trying out a fix that looks at the FrameLoadType instead, or we could split apart those two cases for HistoryCommitType.
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e3f962bc6418327ee700d11eadee4501fa5b12f

commit 4e3f962bc6418327ee700d11eadee4501fa5b12f
Author: creis <creis@chromium.org>
Date: Fri Jul 15 20:24:37 2016

Fix setHistoryItemStateForCommit for back/forward in subframes.

We were not preserving m_provisionalItem on an initial history
commit of a newly created subframe, meaning that we lost things
like the scroll position and document sequence number.

BUG= 628286 
TEST=See bug for repro steps
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2148313002
Cr-Commit-Position: refs/heads/master@{#405844}

[modify] https://crrev.com/4e3f962bc6418327ee700d11eadee4501fa5b12f/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/4e3f962bc6418327ee700d11eadee4501fa5b12f/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/4e3f962bc6418327ee700d11eadee4501fa5b12f/third_party/WebKit/Source/core/loader/FrameLoader.h

Comment 3 by creis@chromium.org, Jul 15 2016

Status: Fixed (was: Started)

Comment 4 by laforge@google.com, Nov 7 2017

Components: Internals>Network>Service

Comment 5 by laforge@google.com, Nov 7 2017

Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.
►

Sign in to add a comment