Confirmed, also repros on Linux. Bisect points to:

https://chromium.googlesource.com/chromium/src/+log/2dfd4ba707f617a44641c4e732ec986a331b4618..35c8d1f820bf8d439088da996941207004358ebc

But nothing obvious jumps out...

This is a pretty significant bug if it happens all the time. Marking RBB, TPMs feel free to argue with me :)

Agree that nothing jumps out from the revision range. +skobes, could this be connected to any scroll anchoring stuff?

I'll do a manual bisect. Agree with RBB

Hm. Any chance it's connected to 7ae0fd5c2969a7253c23fdad6993898323aba694? That's about reloads.

toyoshim, can you confirm/deny?

gab, can you get the old behavior by forcing off NonValidatingReloadOnNormalReload?

I don't think it's related to scroll anchoring, unless it's correlated with enabling scroll anchoring.

Manual testing shows 7ae0fd5c2969a7253c23fdad6993898323aba694 to be the culprit. Over to toyoshim@.

@4: yes disabling fixed the issue, but I can't verify that it's not just because it's fixed on trunk (I had a pending update) because I can't re-enable ( issue 634409 ) lol..!

Thank you for capturing this problem.
New reload behavior is not behind a flag as follows,
chrome://flags/#enable-non-validating-reload-on-normal-reload
and controlled via a field trial.

I guess gab@ caught an enabling config for this study recently.

My change is intended to modify reload behaviors to be same with one of omnibox+enter. But as you reported, it seems to have a side effect.

I will fix this issue before enabling the feature by default. Also I will remove the ReleaseBlock label since this is a feature controlled via field trial, and does not affect most users.

Note: The CL mentioned was just enabling the flag for variation tests.

Ah, priority should be changed too.

toyoshim@ : Gentle Ping..!

Note : Its working fine on Mac 10.11.6 and Ubuntu 14.04 using 54.0.2827.0 and does not work for Windows 7.
gab@ : Could you please update further on this.

What "further update" are you looking for? It doesn't work on Windows, that's my report and it's still true..?

Updated OS labels to highlight that this is Windows specific.

toyoshim@ appears to be working on this per #8, no feedback needed.

I didn't notice that this happens only on Windows. So, this information is helpful. Let me check the root cause again.

CC: kenji for sharing some thoughts around reload behaviors (See *1)

Expectation:
Same page navigation (omnibox+enter): weak reload + reset scroll position (*1)
Reload/Hard reload: reload + restore scroll position (*2)

               | Same page | Reload | Reload with flag |
---------------+-----------+--------+------------------+
Windows Stable |    ok     |   ok   | ng (reset scroll)|
Windows Canary |    ok     |   ok   | ng (reset scroll)|
Linux   Stable |    ok     |   ok   | ng (reset scroll)|
Linux   head   |    ok     |   ok   | ng (reset scroll)|

(*1): This behavior has room for discussion. Probably, it's reasonable to restore scroll position here too. Especially after we change the normal reload behavior to be same with the same page navigation.

(*2): chrome://flags/#enable-non-validating-reload-on-normal-reload is disabled explicitly here to check behavior for normal users

So this isn't Windows specific bug, but just a behavior difference for the new reload implementation. I still think this should be fixed before we enable this feature for Stable channel on desktops by default.

This feature is enabled for 50% of Canary/Dev and 10% of Beta users.

Thanks Toyoshima-san for looping me in.

We should match the previous behavior of Reload. That is, restore the scroll position.

As for same page navigation, I don't see the use case for not restoring the scroll position... So, settling on just one behavior seems appropriate. Can this be easily controlled via the same flag? This way we can have bundle this change as part of the launch process. If it's non trivial, then proceed with the change and I'll double confirm with UX and other leads in parallel.

Finally, do we do anything else differently for same page navigation vs. reload (e.g. form restoration in one case but not the other)?

Thank you Kenji-san.
Your plan sounds reasonable, and exactly what I want.

> Finally, do we do anything else differently for same page navigation vs. reload (e.g. form restoration in one case but not the other)?

Forms:
 - For GET: Chrome resets form data on reload and the same page navigation
 - For POST: Chrome asks if it's ok to leave the page, then resets it

Referrer:
 - Chrome does not update referrer on reload, but update on the same page navigation

Generally speaking, referrer should be updated for contents(html/js)-initiated navigation(anchor, location.reload(), meta refresh, and so on). Back/forward navigation does not. Since reload from UI is a kind of retry, not updating referrer sounds reasonable.

Ok, we should probably update the referrer on the reload via Omnibox (same page navigation) too then.

FYI, this also affects pinch-zoom so that a same page navigation no longer resets the zoom level which could lead users to get stuck at bad zoom level if they don't know how to zoom out. It could also affect pages that read values like innerWidth/Height if the page assumes that each load starts fully zoomed out (or just doesn't take pinch-zoom into account).

These issues would already exist on reload so perhaps that's ok, just wanted to make sure you have all the details.

bokan:
Thank you for pointing that. Actually, I didn't check about the zoom until today.
I quickly checked behavior. It seems Chrome handles zoom and scroll position in the same way.

Zoom will be also restored on the new reload behavior and the same page navigation after my fix.


In the middle of fixing this issue, I'm seeing WebView test failures that may be related to the zoom issue.

When I changed to restore page state for the same page navigation, following WebView tests fail.

org.chromium.android_webview.test.AwSettingsTest#testLoadWithOverviewModeWithTwoViews
org.chromium.android_webview.test.AwSettingsTest#testLoadWithOverviewModeViewportTagWithTwoViews
org.chromium.android_webview.test.AwSettingsTest#testLoadWithOverviewModeWithTwoViews
with {--webview-sandboxed-renderer}
org.chromium.android_webview.test.AwSettingsTest#testLoadWithOverviewModeViewportTagWithTwoViews
with {--webview-sandboxed-renderer}

This seems to be because WebSettings.setLoadWithOverviewMode() does not trigger onScaleChanged() callbacks for the same page navigation in these tests with my change. If I consider this, I slightly prefer not to handle "omnibox+enter" as a kind of reload, but just a standard load.

Now I have a fix that works with Android overview mode.
https://codereview.chromium.org/2257743002/

Probably this issue does not block the feature launch on mobiles. For desktop, I will request a merge to 53.

Now I realize that we should have a clear design and spec that implementation follows. Otherwise, we can not avoid contradiction.

Spec:
 - https://html.spec.whatwg.org/multipage/browsers.html#persisted-user-state-restoration
 There is a spec for persisted user state restoration. We should refer History.scrollRestoration flag to restore scroll position and maybe other state like scale.
 For history navigation, we need to restore state in case the flag being "auto".

 - https://html.spec.whatwg.org/multipage/browsers.html#dom-location-reload
 Reload should run "Navigate" algorithm with "replacement enabled".

 - https://html.spec.whatwg.org/multipage/browsers.html#traverse-the-history
 Reload eventually run "History traversal". There, we should not restore the user state on running with "replacement enabled".


So what the spec expects is not to restore scroll position and scale on any kind of reloads. Same page navigation should be same.


Firefox - same page navigation:
 - navigates as usual, but does not add a new history item
 - reset user state

Firefox - reload
 - reload, and does not add a new history item
 - restore user state (seems to be wrong in terms of the spec)

Chrome - same page navigation:
 - reload main frame, as a result it does not add a new history item (because of being a kind of reload)
 - reset user state (explicitly avoid restoring state for main resource reload => this leads the bug we discuss)

Chrome - reload
 - reload, and does not add a new history item
 - restore user state (seems to be wrong in terms of the spec)

Chrome - new reload
 - reload main frame, and it does not add a new history item
 - reset user state (this is correct in terms of the spec)


Ideally, I want to stop restoring user state on old/new reload to avoid unnecessarily complicated design and discussion. Otherwise, we need to define another reload-ish load type to keep the current behavior.

After searching the web, I noticed that developer had needed to restore scroll position by JavaScript before, but some years ago, all modern browsers changed this behavior to restore automatically.

So, I should investigate historical background about user state restoration, and should know which spec led such browsers' implementation changes.

+majidvp@ (OOO until next week) but worked on the scroll restoration API and should have context on the history and current state of scroll restoration.

Thanks. I want to understand the right requirement for the overview mode.

I will decide to restore user state, scroll position and scale, for reloads and same page navigation. But I'm not sure what is the expected behavior for the overview mode, e.g. scale should be reset or not for each history/normal/same page navigation and reload. If I can just reset scale for reload and same page navigation, it's ideal for me (though I need to modify AwSettingsTest tests to pass)

When you say same page navigation, do you mean omnibox+enter or navigating to an anchor on the current page?

The way overview has worked until now is:

1) Reload: Don't reset scale/scroll, use current values
2) Omnibox+Enter: Reset scale/scroll to minimum scale and origin
3) Same page navigation (anchor on same page): Don't reset scale
4) New page navigation: Reset scale/scroll to minimum scale and origin
5) History Nav: don't reset scale, use the saved scale in the history item

The place this happens is in WebViewImpl::didCommitLoad, pageScaleConstraintsSet::setNeedsReset tells Blink to reset the page scale to the minimum (i.e. overview).

I'm not sure what the original design decisions were for overview mode. IMO, having omnibox+enter reset state is probably useful. +aelias@ since he probably knows the original reasons.

Generally, the design principle I followed is that scale should be treated like simply a part of the scroll position data structure.  Whenever scroll is reset to (0,0), then scale should also be reset to overview.  Whenever scroll is restored from historyitem, then scale should also be restored from historyitem.  There are some additional nuances in that resetting scale to overview is a dynamic/complex process, but that's the basic idea.

On Android, we didn't make a conscious decision about the specific question of whether reloads or omnibox+enter same-navigation should reset scale.  We tied scale to scroll and therefore got the analogous behavior.

Here are answers for design discussion

Re:#27

Today, Chrome have many reload variants, and no one understand it correctly. It's too complicated for users to utilize these variants.

The basic concept of new reload behaviors assumes two major use cases. We want to merge other minor use cases into these two as we can as possible.

 1. Get fresh contents
 2. Fix broken contents

To follow this concept, resetting scroll position and scale will belong to 2). The same page navigation and normal reload will be replaced with 1). Shift-reload will be replaced with 2). For mobile, we are planning to trigger 2) if users ask another reload after a reload.

Re:#28
Agreed the principle that scale should be treated like simply a part of the scroll position data structure. This is aligned with the spec that defines 'user state'.

Here are answers for details.

Re:#27
The same page navigation I said contained "Omnibox+Enter", and I didn't notice the difference of 2) and 3) until you pointed. Omnibox+Enter is handled as a normal navigation in content's navigation controller, and in blink it's reassigned to the same page navigation to avoid updating history item. Is the difference between 2) and 3) intended?

Also, I notice current Android behavior on 2) is a little inconsistent.

- Omnibox+Enter without any scale change => Don't reset scroll position (and scale)
- Omnibox+Enter after scale being changed => Reset scroll position and scale

Similar inconsistent behavior exists on 3)

- Clink an anchor to the current page without any scale change => Reset scroll position (and scale)
- Clink an anchor to the current page after scale being changed => Don't reset scale, scroll position jumps to a half position (maybe this is another bug of restoring scroll position)


To follow UI's design principle, we should take the same behavior regardless of the scale being changed or not for each.

My current plan how to fix this issue is

1) Restore scale and scroll position on reload and the same page navigation when the new reload flag is enabled. This is by design.

2) Keep current behavior for old reload

3) Fix Android WebView tests not to depend on reload or the same page navigation behavior

For 3), now the WebView test loads a page via data URL. If the page content is the same, the data URL gets to be the same too, and it results in the same page navigation in Blink. If the page content is different, it results in a normal navigation in Blink. Probably, it should be better to make all loads normal navigation by adding a sequence number like <!-- 1 --> at the end of the loading data.

The plan LGTM. Thanks!

Upload my plan at #31 as the Patch Set 5. If we do not need to keep the current behavior for old reload, Patch Set 4 will work and easier to merge to release branches than 5.
https://codereview.chromium.org/2257743002/

Ideally, we want to merge this to m52, but probably m53 is the reasonable target for desktops?

Yes, M53 is a more reasonable choice.

Filed a separate bug for the existing inconsistency I pointed at #c30
https://bugs.chromium.org/p/chromium/issues/detail?id=642279

Re #29: I agree that simplification of different modes of reloads into two is a good idea and the scroll & scale restore/reset behaviour associate with each makes sense to me. 


Regarding scroll/scale restoration in general I agree with #28 that scroll and scale should be reset, restored together. 

However in our implementation of history.scrollRestoration we made a conscious choice that scrollRestoration='manual' should only disable scroll position restoration and should *not* disable scale restoration. (See https://codereview.chromium.org/1277583003).

This was decided because there is currently no way for developers to restore scale themselves so not restoring scale would penalize developers which are using scrollRestoration='manual' to implement their own restoration logic which is the recommended approach. In any case, the spec wording is such that it does not prescribe anything on scale restoration and only talks about scroll so we can change our implementation if need be though I don't think you need/want to do that for you change so I think we should keep that behaviour as is. 
See full rationale here: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/p1sy9aQDmtU/jndSickDCAAJ

Thank you for joining discussion here and in code review.
A fix will be submitted soon.

Here is the plan I discussed at the code review.

1. enabled by default at trunk, but keep the flag available until it goes to
stable (as usually we do)
2. roll out this feature to desktops under a field trial control, there problem
less likely happen.
3. roll out this feature to mobiles under a field trial control
4. wait until WebView with this feature goes to beta and stable, and see what
happens. If serious problem happens, flip back the flag to disabled.

Also I filed a issue to ask about the same page navigation from the viewpoint of web standard.
https://github.com/whatwg/html/issues/1742
Unfortunately, it seems we do not have a clear spec for the same page navigation.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a56f872fc3e82ccd819db4378ae2e1ac206c116

commit 2a56f872fc3e82ccd819db4378ae2e1ac206c116
Author: toyoshim <toyoshim@chromium.org>
Date: Wed Sep 07 05:52:08 2016

Restore user state on ReloadwithoutSubResourceCacheRevalidation

Currently, Chrome does not restore user state, scroll position
and scale factor for same page navigation regardless of
NonValidatingReloadOnNormalReload content feature flag.

This patch connects NonValidatingReloadOnNormalReload flag
to ReloadwithoutSubResourceCacheRevalidation blink feature flag,
and changes the behavior to restore user state even for the same
page navigation if ReloadwithoutSubResourceCacheRevalidation
flag is enabled.

BUG= 632358 

Review-Url: https://codereview.chromium.org/2257743002
Cr-Commit-Position: refs/heads/master@{#416855}

[modify] https://crrev.com/2a56f872fc3e82ccd819db4378ae2e1ac206c116/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
[modify] https://crrev.com/2a56f872fc3e82ccd819db4378ae2e1ac206c116/content/child/runtime_features.cc
[modify] https://crrev.com/2a56f872fc3e82ccd819db4378ae2e1ac206c116/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/2a56f872fc3e82ccd819db4378ae2e1ac206c116/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
[modify] https://crrev.com/2a56f872fc3e82ccd819db4378ae2e1ac206c116/third_party/WebKit/Source/web/WebRuntimeFeatures.cpp
[modify] https://crrev.com/2a56f872fc3e82ccd819db4378ae2e1ac206c116/third_party/WebKit/public/web/WebRuntimeFeatures.h

First, let me request merging the fix #39 to beta branch.

Your change meets the bar and is auto-approved for M54 (branch: 2840)

Could you please confirm whether this change is baked/verified in Canary and safe to merge?If yes, merge your change to M54 (branch: 2840) so that we could take this for next Beta Release.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e511817baa8b186aac81dd9e2aa4e49ece5d36e3

commit e511817baa8b186aac81dd9e2aa4e49ece5d36e3
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Fri Sep 09 05:37:30 2016

Restore user state on ReloadwithoutSubResourceCacheRevalidation

Currently, Chrome does not restore user state, scroll position
and scale factor for same page navigation regardless of
NonValidatingReloadOnNormalReload content feature flag.

This patch connects NonValidatingReloadOnNormalReload flag
to ReloadwithoutSubResourceCacheRevalidation blink feature flag,
and changes the behavior to restore user state even for the same
page navigation if ReloadwithoutSubResourceCacheRevalidation
flag is enabled.

BUG= 632358 

Review-Url: https://codereview.chromium.org/2257743002
Cr-Commit-Position: refs/heads/master@{#416855}
(cherry picked from commit 2a56f872fc3e82ccd819db4378ae2e1ac206c116)

TBR=toyoshim@chromium.org

Review URL: https://codereview.chromium.org/2326983002 .

Cr-Commit-Position: refs/branch-heads/2840@{#265}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/content/child/runtime_features.cc
[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/third_party/WebKit/Source/web/WebRuntimeFeatures.cpp
[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/third_party/WebKit/public/web/WebRuntimeFeatures.h

Verified the issue on windows 10, Ubuntu 14.04 and Mac 10.11.6 using chrome beta version #54.0.2840.27 as per comment #0
Observed that the fix is working as expected.

Attaching screencast for reference

Hence, adding the verified labels.

Deleted: 632358.mp4 3.8 MB

	632358.mp4 3.8 MB View Download

Thanks. Now I request a merge to the stable branch too.

[Automated comment] Request affecting a post-stable build (M53), manual review required.

Justification:
 - This fix works fine on ToT over a week
 - Fix is simple, just change a condition to switch behavior on edge cases, and less risky

The new faster reload feature was planned to be shipped at m53, but we cannot go without this fix. We can also control this feature via server side configurations. So, we do not have to respin the stable channel only for this fix. I can manage server side configuration to enable the feature only for platforms that have a chance to be refreshed.

dimu@, shall I wait for or do something to get an approval for merging this to 53 branch?

Verified the fix on Windows 7, MAC (10.11.6) & Ubuntu Trusty (14.04) for Google Chrome Stable Version - 53.0.2785.116 

TE-Verified Labels are added.

Screen-recording is attached.

Deleted: 632358.mp4 3.2 MB

	632358.mp4 3.2 MB View Download

Re:49

The fix wasn't merged to m53, and need a flag to reproduce the issue on m53.
chrome://flags/#enable-non-validating-reload-on-normal-reload should be set to 'Enabled'.

We're not taking a late change for a new feature, please wait until M54, merge rejected for M53.

Hum... on hangouts, I heard that this will be ok to merge to the stable branch, but we won't respin the stable binary only for this fix, but may be shipped together with another fix. So, I'm a bit confused for the comment 53.

This was a web compatibility breaking bug, but can be suppressed by a server side configuration. So, I do not have a strong opinion to merge this to stable, but just in case.

OK, I had a chat, and understood the situation and decision.
We will wait M54.

Change M-53 label to M-54, and marked as Verified because of TE-Verified-M54.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e511817baa8b186aac81dd9e2aa4e49ece5d36e3

commit e511817baa8b186aac81dd9e2aa4e49ece5d36e3
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Fri Sep 09 05:37:30 2016

Restore user state on ReloadwithoutSubResourceCacheRevalidation

Currently, Chrome does not restore user state, scroll position
and scale factor for same page navigation regardless of
NonValidatingReloadOnNormalReload content feature flag.

This patch connects NonValidatingReloadOnNormalReload flag
to ReloadwithoutSubResourceCacheRevalidation blink feature flag,
and changes the behavior to restore user state even for the same
page navigation if ReloadwithoutSubResourceCacheRevalidation
flag is enabled.

BUG= 632358 

Review-Url: https://codereview.chromium.org/2257743002
Cr-Commit-Position: refs/heads/master@{#416855}
(cherry picked from commit 2a56f872fc3e82ccd819db4378ae2e1ac206c116)

TBR=toyoshim@chromium.org

Review URL: https://codereview.chromium.org/2326983002 .

Cr-Commit-Position: refs/branch-heads/2840@{#265}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/content/child/runtime_features.cc
[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/third_party/WebKit/Source/web/WebRuntimeFeatures.cpp
[modify] https://crrev.com/e511817baa8b186aac81dd9e2aa4e49ece5d36e3/third_party/WebKit/public/web/WebRuntimeFeatures.h