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 172 matching lines @@
     if (!m_currentItem || !m_frame->view())
         return;
 
     // Shouldn't clobber anything if we might still restore later.
     if (needsHistoryItemRestore(m_loadType) && !m_frame->view()->wasScrolledByUser())
         return;
 
     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());
+    if (m_frame->isMainFrame()) {
+        float scaleFactor = m_frame->page()->pageScaleFactor();
+        m_currentItem->setPageScaleFactor(scaleFactor == 1.0f ? 0 : scaleFactor);

[dcheng, 2015/06/02 19:11:35]

Does 0 have some special meaning here? Why don't we want to pass 1.0 through?

[Nate Chapin, 2015/06/02 21:18:57]

0 is a magic value that means "we don't know, so don't change the scale factor".
The version I just uploaded changed that magic value to 1.0, which presumes that
the default behavior on a different document navigation is to reset the page
scale. I'm not sure that's true or not....but no tests broke.

+    }
 
     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, bool isSameDocument, bool isPushOrReplaceState)
 {
-    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();
-    }
-
+    RefPtr<HistoryItem> oldItem = m_currentItem;

[dcheng, 2015/06/02 19:11:35]

I think this needs to be a RefPtrWillBeRawPtr, since HistoryItems are in Oilpan.

[Nate Chapin, 2015/06/02 21:18:57]

Done.

+    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 isn't a same-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 (!isSameDocument && (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 && (isPushOrReplaceState || 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, false);
 
     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, true, sameDocumentNavigationSource == SameDocumentNavigationHistoryApi);
+    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 834 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