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

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

FrameLoader::SaveScrollState attempts to avoid clobbering state that
will be used for a back/forward restore by checking the committed
DocumentLoader::LoadType(). As of https://codereview.chromium.org/2653673006,
same-document navigations don't modify that load type, so a same-document
back-forward navigation can get its scroll state clobbered by creative
use of an onpopstate event handler.

Store the needed state on the stack, so that even if onpopstate does
trigger a SaveScrollState(), we can still restore. This also pulls
scroll and view state out into a helper class of HistoryItem. This
object is nullptr until state is saved for the first time, so we don't
need a bool to track whether the state is default or real.

BUG=734276
TEST=fast/history/change-viewport-height-in-onpopstate.html

Review-Url: https://codereview.chromium.org/2949073002
Cr-Commit-Position: refs/heads/master@{#487706}
Committed: https://chromium.googlesource.com/chromium/src/+/64015d479a86f06c19cac283a6e6214b38d1db10

[Nate Chapin, 2017-06-21 18:56:29]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 18:57:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 20:23:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 20:23:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nate Chapin, 2017-06-21 22:38:53]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 22:39:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 23:35:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 23:35:48]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Nate Chapin, 2017-06-22 00:04:44]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 00:04:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 01:07:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 01:07:50]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Nate Chapin, 2017-06-22 02:30:37]

Description was changed from

==========
Fire onpopstate during same doc nav before configuring history state

BUG=734276
==========

to

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

FrameLoader::SaveScrollState attempts to avoid clobbering state that
will be used for a back/forward restore by checking the committed
DocumentLoader::LoadType(). As of https://codereview.chromium.org/2653673006,
same-document navigations don't modify that load type, so a same-document
back-forward navigation can get its scroll state clobbered by creative
use of an onpopstate event handler.

Store the needed state on the stack, so that even if onpopstate does
trigger a SaveScrollState(), we can still restore. This also pulls
scroll and view state out into a helper class of HistoryItem. This
object is nullptr until state is saved for the first time, so we don't
need a bool to track whether the state is default or real.

BUG=734276
TEST=fast/history/change-viewport-height-in-onpopstate.html
==========

[Nate Chapin, 2017-06-22 02:31:01]

japhet@chromium.org changed reviewers:
+ majidvp@chromium.org

[Nate Chapin, 2017-06-22 02:31:02]

majidvp, what do you think of this bugfix+cleanup?

[majidvp, 2017-06-26 16:19:27]

This is a nice clean up.

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:358: if
(HistoryItem::ScrollAndViewState* scroll_and_scale_state =
s/scroll_and_scale_state/scroll_and_view_state/

Instead of copying each member individually this should just copy the old
object, either via a copy constructor or just use of|CopyScrollAndViewState()|.

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:566: 
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.

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.h:250: void
RestoreScrollPositionAndViewStateForLoadType(
nit: Given that load type is no longer the sole argument to this function
perhaps we should change its name.

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HistoryItem.h:62: class ScrollAndViewState
: public GarbageCollected<ScrollAndViewState> {
I consider scroll position to be part of the view state. In that light it seems
that a just calling this History::ViewState is more appropriate.

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HistoryItem.h:87: copy->page_scale_factor_
= scroll_and_view_state_->page_scale_factor_;
This feels like it should be a copy constructor on the ScrollAndViewState class.

[Nate Chapin, 2017-07-11 19:28:35]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 19:28:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:358: if
(HistoryItem::ScrollAndViewState* scroll_and_scale_state =
On 2017/06/26 16:19:26, majidvp wrote:
> s/scroll_and_scale_state/scroll_and_view_state/
> 
> Instead of copying each member individually this should just copy the old
> object, either via a copy constructor or just use
of|CopyScrollAndViewState()|.
> 
> 

Done.

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:566: 
On 2017/06/26 16:19:26, majidvp wrote:
> 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.

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

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.h:250: void
RestoreScrollPositionAndViewStateForLoadType(
On 2017/06/26 16:19:26, majidvp wrote:
> nit: Given that load type is no longer the sole argument to this function
> perhaps we should change its name. 

Dropped "ForLoadType", which leaves multiple functions called
RestoreScrollPositionAndViewState. I don't like that very much, but it's
probably better than leaving the existing name.

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HistoryItem.h:62: class ScrollAndViewState
: public GarbageCollected<ScrollAndViewState> {
On 2017/06/26 16:19:26, majidvp wrote:
> I consider scroll position to be part of the view state. In that light it
seems
> that a just calling this History::ViewState is more appropriate.

Done.

https://codereview.chromium.org/2949073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HistoryItem.h:87: copy->page_scale_factor_
= scroll_and_view_state_->page_scale_factor_;
On 2017/06/26 16:19:26, majidvp wrote:
> This feels like it should be a copy constructor on the ScrollAndViewState
class.

Reworked class to not be GCed, so it can have a copy constructor :)

[commit-bot: I haz the power, 2017-07-11 22:53:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-11 22:53:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[majidvp, 2017-07-17 20:06:15]

https://codereview.chromium.org/2949073002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/2949073002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HistoryItem.h:76: view_state_ =
WTF::MakeUnique<ViewState>(*other->view_state_.get());
shouldn't we reset() in the nullptr case?

[Nate Chapin, 2017-07-17 20:38:24]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-17 20:38:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2017-07-17 20:38:43]

https://codereview.chromium.org/2949073002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/2949073002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HistoryItem.h:76: view_state_ =
WTF::MakeUnique<ViewState>(*other->view_state_.get());
On 2017/07/17 20:06:15, majidvp wrote:
> shouldn't we reset() in the nullptr case?

That is closer to the old behavior, yes. Done.

[commit-bot: I haz the power, 2017-07-17 23:39:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-17 23:39:06]

Dry run: This issue passed the CQ dry run.

[majidvp, 2017-07-18 12:26:34]

lgtm

[Nate Chapin, 2017-07-18 18:05:26]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-07-18 18:05:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-18 18:19:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-18 18:19:33]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Nate Chapin, 2017-07-18 18:23:36]

japhet@chromium.org changed reviewers:
+ alexmos@chromium.org

[Nate Chapin, 2017-07-18 18:23:37]

alexmos, would you mind reviewing the content/renderer/ change?

[alexmos, 2017-07-18 20:48:28]

content/renderer/ LGTM

[Nate Chapin, 2017-07-18 23:15:32]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-07-18 23:15:49]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-19 01:23:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1500419732019120,
"parent_rev": "724dbb5f562ebb4c62c6c52015f7a916b73c1923", "commit_rev":
"64015d479a86f06c19cac283a6e6214b38d1db10"}

[commit-bot: I haz the power, 2017-07-19 01:23:18]

Description was changed from

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

FrameLoader::SaveScrollState attempts to avoid clobbering state that
will be used for a back/forward restore by checking the committed
DocumentLoader::LoadType(). As of https://codereview.chromium.org/2653673006,
same-document navigations don't modify that load type, so a same-document
back-forward navigation can get its scroll state clobbered by creative
use of an onpopstate event handler.

Store the needed state on the stack, so that even if onpopstate does
trigger a SaveScrollState(), we can still restore. This also pulls
scroll and view state out into a helper class of HistoryItem. This
object is nullptr until state is saved for the first time, so we don't
need a bool to track whether the state is default or real.

BUG=734276
TEST=fast/history/change-viewport-height-in-onpopstate.html
==========

to

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

FrameLoader::SaveScrollState attempts to avoid clobbering state that
will be used for a back/forward restore by checking the committed
DocumentLoader::LoadType(). As of https://codereview.chromium.org/2653673006,
same-document navigations don't modify that load type, so a same-document
back-forward navigation can get its scroll state clobbered by creative
use of an onpopstate event handler.

Store the needed state on the stack, so that even if onpopstate does
trigger a SaveScrollState(), we can still restore. This also pulls
scroll and view state out into a helper class of HistoryItem. This
object is nullptr until state is saved for the first time, so we don't
need a bool to track whether the state is default or real.

BUG=734276
TEST=fast/history/change-viewport-height-in-onpopstate.html

Review-Url: https://codereview.chromium.org/2949073002
Cr-Commit-Position: refs/heads/master@{#487706}
Committed:
https://chromium.googlesource.com/chromium/src/+/64015d479a86f06c19cac283a6e6...
==========

[commit-bot: I haz the power, 2017-07-19 01:23:19]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/64015d479a86f06c19cac283a6e6...