Changing scroll and view state in onpopstate shouldn't overwrite back/forward state restore

third_party/WebKit/Source/core/loader/FrameLoader.cpp

Index: third_party/WebKit/Source/core/loader/FrameLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index e60229e6e1d6fa3c23a8f598e340ea32d782a184..a8c4eeed61730dc4ccb2d26ca4b57ce7214fe3d2 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -560,12 +560,19 @@ void FrameLoader::LoadInSameDocument(
 
   frame_->GetDocument()->CheckCompleted();
 
+  // onpopstate might change view state, so stash for later restore.
+  HistoryItem::ScrollAndViewState* scroll_and_view_state =
+      history_item ? history_item->CopyScrollAndViewState() : nullptr;
+

[majidvp, 2017/06/26 16:19:26]

This may be something that can be captured by a stack only class which clones
the state and restores it on destruction.

I think however this pattern is only used once here. So I will leave it
up to you to decide if this is something worth doing.

[Nate Chapin, 2017/07/11 22:17:59]

I *hope* this will stay a one-off, so I'm disinclined to both with a helper
class.

   frame_->DomWindow()->StatePopped(state_object
                                        ? std::move(state_object)
                                        : SerializedScriptValue::NullValue());
 
-  if (history_item)
-    RestoreScrollPositionAndViewStateForLoadType(frame_load_type);
+  if (history_item) {
+    RestoreScrollPositionAndViewStateForLoadType(
+        frame_load_type, scroll_and_view_state,
+        history_item->ScrollRestorationType());
+  }
 
   // We need to scroll to the fragment whether or not a hash change occurred,
   // since the user might have scrolled since the previous navigation.
@@ -1060,28 +1067,31 @@ bool FrameLoader::IsLoadingMainFrame() const {
 }
 
 void FrameLoader::RestoreScrollPositionAndViewState() {
-  if (!frame_->GetPage() || !GetDocumentLoader())
+  if (!frame_->GetPage() || !GetDocumentLoader() ||
+      !GetDocumentLoader()->GetHistoryItem())
     return;
-  RestoreScrollPositionAndViewStateForLoadType(GetDocumentLoader()->LoadType());
+  RestoreScrollPositionAndViewStateForLoadType(
+      GetDocumentLoader()->LoadType(),
+      GetDocumentLoader()->GetHistoryItem()->GetScrollAndViewState(),
+      GetDocumentLoader()->GetHistoryItem()->ScrollRestorationType());
 }
 
 void FrameLoader::RestoreScrollPositionAndViewStateForLoadType(
-    FrameLoadType load_type) {
+    FrameLoadType load_type,
+    HistoryItem::ScrollAndViewState* scroll_and_view_state,
+    HistoryScrollRestorationType scroll_restoration_type) {
   LocalFrameView* view = frame_->View();
   if (!view || !view->LayoutViewportScrollableArea() ||
       !state_machine_.CommittedFirstRealDocumentLoad() ||
       !frame_->IsAttached()) {
     return;
   }
-  if (!NeedsHistoryItemRestore(load_type))
-    return;
-  HistoryItem* history_item = document_loader_->GetHistoryItem();
-  if (!history_item || !history_item->DidSaveScrollOrScaleState())
+  if (!NeedsHistoryItemRestore(load_type) || !scroll_and_view_state)
     return;
 
   bool should_restore_scroll =
-      history_item->ScrollRestorationType() != kScrollRestorationManual;
-  bool should_restore_scale = history_item->PageScaleFactor();
+      scroll_restoration_type != kScrollRestorationManual;
+  bool should_restore_scale = scroll_and_view_state->page_scale_factor_;
 
   // This tries to balance:
   // 1. restoring as soon as possible
@@ -1093,7 +1103,8 @@ void FrameLoader::RestoreScrollPositionAndViewStateForLoadType(
   //    previous height
   bool can_restore_without_clamping =
       view->LayoutViewportScrollableArea()->ClampScrollOffset(
-          history_item->GetScrollOffset()) == history_item->GetScrollOffset();
+          scroll_and_view_state->scroll_offset_) ==
+      scroll_and_view_state->scroll_offset_;
   bool can_restore_without_annoying_user =
       !GetDocumentLoader()->GetInitialScrollState().was_scrolled_by_user &&
       (can_restore_without_clamping || !frame_->IsLoading() ||
@@ -1103,13 +1114,13 @@ void FrameLoader::RestoreScrollPositionAndViewStateForLoadType(
 
   if (should_restore_scroll) {
     view->LayoutViewportScrollableArea()->SetScrollOffset(
-        history_item->GetScrollOffset(), kProgrammaticScroll);
+        scroll_and_view_state->scroll_offset_, kProgrammaticScroll);
   }
 
   // For main frame restore scale and visual viewport position
   if (frame_->IsMainFrame()) {
     ScrollOffset visual_viewport_offset(
-        history_item->VisualViewportScrollOffset());
+        scroll_and_view_state->visual_viewport_scroll_offset_);
 
     // If the visual viewport's offset is (-1, -1) it means the history item
     // is an old version of HistoryItem so distribute the scroll between
@@ -1117,16 +1128,17 @@ void FrameLoader::RestoreScrollPositionAndViewStateForLoadType(
     if (visual_viewport_offset.Width() == -1 &&
         visual_viewport_offset.Height() == -1) {
       visual_viewport_offset =
-          history_item->GetScrollOffset() -
+          scroll_and_view_state->scroll_offset_ -
           view->LayoutViewportScrollableArea()->GetScrollOffset();
     }
 
     VisualViewport& visual_viewport = frame_->GetPage()->GetVisualViewport();
     if (should_restore_scale && should_restore_scroll) {
-      visual_viewport.SetScaleAndLocation(history_item->PageScaleFactor(),
-                                          FloatPoint(visual_viewport_offset));
+      visual_viewport.SetScaleAndLocation(
+          scroll_and_view_state->page_scale_factor_,
+          FloatPoint(visual_viewport_offset));
     } else if (should_restore_scale) {
-      visual_viewport.SetScale(history_item->PageScaleFactor());
+      visual_viewport.SetScale(scroll_and_view_state->page_scale_factor_);
     } else if (should_restore_scroll) {
       visual_viewport.SetLocation(FloatPoint(visual_viewport_offset));
     }