Don't reuse HistoryItems for multiple navigations

Source/core/loader/FrameLoader.cpp

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2008 Alp Toker <alp@atoker.com>
  * Copyright (C) Research In Motion Limited 2009. All rights reserved.
  * Copyright (C) 2011 Kris Jordan <krisjordan@gmail.com>
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ skipping 178 matching lines @@
 
     m_currentItem->setScrollPoint(m_frame->view()->scrollPosition());
     m_currentItem->setPinchViewportScrollPoint(m_frame->host()->pinchViewport().visibleRect().location());
 
     if (m_frame->isMainFrame())
         m_currentItem->setPageScaleFactor(m_frame->page()->pageScaleFactor());
 
     client()->didUpdateCurrentHistoryItem();
 }
 
-void FrameLoader::clearScrollPositionAndViewState()
-{
-    ASSERT(m_frame->isMainFrame());
-    if (!m_currentItem)
-        return;
-    m_currentItem->clearScrollPoint();
-    m_currentItem->setPageScaleFactor(0);
-}
-
 void FrameLoader::dispatchUnloadEvent()
 {
     saveScrollState();
 
     if (m_frame->document() && !SVGImage::isInSVGImage(m_frame->document()))
         m_frame->document()->dispatchUnloadEvents();
 
     if (Page* page = m_frame->page())
         page->undoStack().didUnloadFrame(*m_frame);
 }
@@ skipping 71 matching lines @@
     clear();
 
     // detachChildren() potentially detaches the frame from the document. The
     // loading cannot continue in that case.
     if (!m_frame->page())
         return;
 
     documentLoader->replaceDocumentWhileExecutingJavaScriptURL(init, source, ownerDocument);
 }
 
-void FrameLoader::setHistoryItemStateForCommit(HistoryCommitType historyCommitType, bool isPushOrReplaceState, HistoryScrollRestorationType scrollRestorationType, PassRefPtr<SerializedScriptValue> stateObject)
+void FrameLoader::setHistoryItemStateForCommit(HistoryCommitType historyCommitType, HistoryNavigationType navigationType)
 {
-    if (m_provisionalItem)
-        m_currentItem = m_provisionalItem.release();
-
-    if (!m_currentItem || historyCommitType == StandardCommit) {
-        m_currentItem = HistoryItem::create();
-    } else if (!isPushOrReplaceState && m_documentLoader->url() != m_currentItem->url()) {
-        m_currentItem->generateNewItemSequenceNumber();
-        if (!equalIgnoringFragmentIdentifier(m_documentLoader->url(), m_currentItem->url()))
-            m_currentItem->generateNewDocumentSequenceNumber();
-    }
-
+    RefPtrWillBeRawPtr<HistoryItem> oldItem = m_currentItem;
+    m_currentItem = historyCommitType == BackForwardCommit ? m_provisionalItem.release() : HistoryItem::create();
     m_currentItem->setURL(m_documentLoader->urlForHistory());
     m_currentItem->setDocumentState(m_frame->document()->formElementsState());
     m_currentItem->setTarget(m_frame->tree().uniqueName());
-    if (isPushOrReplaceState) {
-        m_currentItem->setStateObject(stateObject);
-        m_currentItem->setScrollRestorationType(scrollRestorationType);
-    }
     m_currentItem->setReferrer(SecurityPolicy::generateReferrer(m_documentLoader->request().referrerPolicy(), m_currentItem->url(), m_documentLoader->request().httpReferrer()));
     m_currentItem->setFormInfoFromRequest(m_documentLoader->request());
+
+    // Don't propagate state from the old item to the new item if there isn't an old item (obviously),
+    // or if this is a back/forward navigation, since we explicitly want to restore the state we just
+    // committed.
+    if (!oldItem || historyCommitType == BackForwardCommit)
+        return;
+    // Don't propagate state from the old item if this is a different-document navigation, unless the before
+    // and after pages are logically related. This means they have the same url (ignoring fragment) and
+    // the new item was loaded via reload or client redirect.
+    if (navigationType == HistoryNavigationType::DifferentDocument && (historyCommitType != HistoryInertCommit || !equalIgnoringFragmentIdentifier(oldItem->url(), m_currentItem->url())))
+        return;
+    m_currentItem->setDocumentSequenceNumber(oldItem->documentSequenceNumber());
+    m_currentItem->setScrollPoint(oldItem->scrollPoint());
+    m_currentItem->setPinchViewportScrollPoint(oldItem->pinchViewportScrollPoint());
+    m_currentItem->setPageScaleFactor(oldItem->pageScaleFactor());
+    m_currentItem->setFrameSequenceNumber(oldItem->frameSequenceNumber());
+    m_currentItem->setStateObject(oldItem->stateObject());
+    // The item sequence number determines whether items are "the same", such back/forward navigation
+    // between items with the same item sequence number is a no-op. Only treat this as identical if the
+    // navigation did not create a back/forward entry and the url is identical or it was loaded via
+    // history.replaceState().
+    if (historyCommitType == HistoryInertCommit && (navigationType == HistoryNavigationType::HistoryApi || oldItem->url() == m_currentItem->url()))
+        m_currentItem->setItemSequenceNumber(oldItem->itemSequenceNumber());
 }
 
 static HistoryCommitType loadTypeToCommitType(FrameLoadType type)
 {
     switch (type) {
     case FrameLoadTypeStandard:
         return StandardCommit;
     case FrameLoadTypeInitialInChildFrame:
     case FrameLoadTypeInitialHistoryLoad:
         return InitialCommitInChildFrame;
     case FrameLoadTypeBackForward:
         return BackForwardCommit;
     default:
         break;
     }
     return HistoryInertCommit;
 }
 
 void FrameLoader::receivedFirstData()
 {
     if (m_stateMachine.creatingInitialEmptyDocument())
         return;
 
     HistoryCommitType historyCommitType = loadTypeToCommitType(m_loadType);
     if (historyCommitType == StandardCommit && (m_documentLoader->urlForHistory().isEmpty() || (opener() && !m_currentItem && m_documentLoader->originalRequest().url().isEmpty())))
         historyCommitType = HistoryInertCommit;
     else if (historyCommitType == InitialCommitInChildFrame && MixedContentChecker::isMixedContent(m_frame->tree().top()->securityContext()->securityOrigin(), m_documentLoader->url()))
         historyCommitType = HistoryInertCommit;
-    setHistoryItemStateForCommit(historyCommitType);
+    setHistoryItemStateForCommit(historyCommitType, HistoryNavigationType::DifferentDocument);
 
     if (!m_stateMachine.committedMultipleRealLoads() && m_loadType == FrameLoadTypeStandard)
         m_stateMachine.advanceTo(FrameLoaderStateMachine::CommittedMultipleRealLoads);
 
     client()->dispatchDidCommitLoad(m_currentItem.get(), historyCommitType);
 
     TRACE_EVENT1("devtools.timeline", "CommitLoad", "data", InspectorCommitLoadEvent::data(m_frame));
     InspectorInstrumentation::didCommitLoad(m_frame, m_documentLoader.get());
     m_frame->page()->didCommitLoad(m_frame);
     dispatchDidClearDocumentOfWindowObject();
@@ skipping 214 matching lines @@
     // Generate start and stop notifications only when loader is completed so that we
     // don't fire them for fragment redirection that happens in window.onload handler.
     // See https://bugs.webkit.org/show_bug.cgi?id=31838
     if (m_frame->document()->loadEventFinished())
         client()->didStartLoading(NavigationWithinSameDocument);
 
     HistoryCommitType historyCommitType = loadTypeToCommitType(type);
     if (!m_currentItem)
         historyCommitType = HistoryInertCommit;
 
-    setHistoryItemStateForCommit(historyCommitType, sameDocumentNavigationSource == SameDocumentNavigationHistoryApi, scrollRestorationType, data);
+    setHistoryItemStateForCommit(historyCommitType, sameDocumentNavigationSource == SameDocumentNavigationHistoryApi ? HistoryNavigationType::HistoryApi : HistoryNavigationType::Fragment);
+    if (sameDocumentNavigationSource == SameDocumentNavigationHistoryApi) {
+        m_currentItem->setStateObject(data);
+        m_currentItem->setScrollRestorationType(scrollRestorationType);
+    }
     client()->dispatchDidNavigateWithinPage(m_currentItem.get(), historyCommitType);
     client()->dispatchDidReceiveTitle(m_frame->document()->title());
     if (m_frame->document()->loadEventFinished())
         client()->didStopLoading();
 }
 
 void FrameLoader::loadInSameDocument(const KURL& url, PassRefPtr<SerializedScriptValue> stateObject, FrameLoadType type, ClientRedirectPolicy clientRedirect)
 {
     // If we have a state object, we cannot also be a new navigation.
     ASSERT(!stateObject || type == FrameLoadTypeBackForward);
@@ skipping 433 matching lines @@
     // This tries to balance 1. restoring as soon as possible, 2. detecting
     // clamping to avoid repeatedly popping the scroll position down as the
     // page height increases, 3. ignore clamp detection after load completes
     // because that may be because the page will never reach its previous
     // height.
     bool canRestoreWithoutClamping = view->clampOffsetAtScale(m_currentItem->scrollPoint(), 1) == m_currentItem->scrollPoint();
     bool canRestoreWithoutAnnoyingUser = !view->wasScrolledByUser() && (canRestoreWithoutClamping || m_frame->isLoading());
     if (!canRestoreWithoutAnnoyingUser)
         return;
 
-    if (m_frame->isMainFrame() && m_currentItem->pageScaleFactor()) {
+    if (m_frame->isMainFrame() && m_currentItem->pageScaleFactor() != 1.0f) {

[bokan, 2015/06/03 15:07:29]

I don't think this is right. This check was added in
https://chromium.googlesource.com/chromium/blink/+/299030c000660b7e7e18980f99...
and relies on the fact that HistoryItem::m_pageScaleFactor is initialized to 0,
which you haven't changed. We definitely don't want to use 1 as the "magic
value" since 1 is a valid scale that we may want to restore. It's not a no-op
since page loading initializes to the minimum allowable scale, which on Android
is frequently < 1.

         FloatPoint pinchViewportOffset(m_currentItem->pinchViewportScrollPoint());
         IntPoint frameScrollOffset(m_currentItem->scrollPoint());
 
         m_frame->page()->setPageScaleFactor(m_currentItem->pageScaleFactor(), frameScrollOffset);
 
         // If the pinch viewport's offset is (-1, -1) it means the history item
         // is an old version of HistoryItem so distribute the scroll between
         // the main frame and the pinch viewport as best as we can.
         if (pinchViewportOffset.x() == -1 && pinchViewportOffset.y() == -1)
             pinchViewportOffset = FloatPoint(frameScrollOffset - view->scrollPosition());
@@ skipping 384 matching lines @@
     // FIXME: We need a way to propagate insecure requests policy flags to
     // out-of-process frames. For now, we'll always use default behavior.
     if (!parentFrame->isLocalFrame())
         return nullptr;
 
     ASSERT(toLocalFrame(parentFrame)->document());
     return toLocalFrame(parentFrame)->document()->insecureNavigationsToUpgrade();
 }
 
 } // namespace blink