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

third_party/WebKit/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.
@@ skipping 542 matching lines @@
   if (history_item)
     document_loader_->SetItemForHistoryNavigation(history_item);
   UpdateForSameDocumentNavigation(url, kSameDocumentNavigationDefault, nullptr,
                                   kScrollRestorationAuto, frame_load_type,
                                   initiating_document);
 
   document_loader_->GetInitialScrollState().was_scrolled_by_user = false;
 
   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.
   ProcessFragment(url, frame_load_type, kNavigationWithinSameDocument);
 
   TakeObjectSnapshot();
 }
 
 // static
 void FrameLoader::SetReferrerForFrameRequest(FrameLoadRequest& frame_request) {
@@ skipping 474 matching lines @@
   Client()->TransitionToCommittedForNewPage();
 
   frame_->GetNavigationScheduler().Cancel();
 }
 
 bool FrameLoader::IsLoadingMainFrame() const {
   return frame_->IsMainFrame();
 }
 
 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
   // 2. not overriding user scroll (TODO(majidvp): also respect user scale)
   // 3. detecting clamping to avoid repeatedly popping the scroll position down
   //    as the page height increases
   // 4. ignore clamp detection if we are not restoring scroll or after load
   //    completes because that may be because the page will never reach its
   //    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() ||
        !should_restore_scroll);
   if (!can_restore_without_annoying_user)
     return;
 
   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
     // the main frame and the visual viewport as best as we can.
     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));
     }
 
     if (ScrollingCoordinator* scrolling_coordinator =
             frame_->GetPage()->GetScrollingCoordinator())
       scrolling_coordinator->FrameViewRootLayerDidChange(view);
   }
 
   GetDocumentLoader()->GetInitialScrollState().did_restore_from_history = true;
@@ skipping 558 matching lines @@
   // TODO(japhet): This is needed because the browser process DCHECKs if the
   // first entry we commit in a new frame has replacement set. It's unclear
   // whether the DCHECK is right, investigate removing this special case.
   bool replace_current_item = load_type == kFrameLoadTypeReplaceCurrentItem &&
                               (!Opener() || !request.Url().IsEmpty());
   loader->SetReplacesCurrentHistoryItem(replace_current_item);
   return loader;
 }
 
 }  // namespace blink