There might be a bug, but I don't understand why the relative ordering of those two functions matters.

Note that history restore (FrameLoader::RestoreScrollPositionAndViewState) should take precedence over the fragment scroll (FrameLoader::ProcessFragment).

If we process the fragment first, the history restore will clobber the scroll offset.

If we process the fragment later, ProcessFragment will skip the fragment scroll because InitialScrollState::did_restore_from_history is true.

Yes, that's exactly what you are describing with
* FrameLoader::RestoreScrollPositionAndViewState(...)
* FrameLoader::ProcessFragment()
* InitialScrollState::did_restore_from_history.

It put some log. I executed the regression test I made. There is one navigation and then one reload.

# If response's size <= 2 * loading_buffer_size bytes.
```
[navigation_handle_impl.cc(189)] New navigation
[FrameLoader.cpp(1334)] ProcessFragment called
[FrameLoader.cpp(1366)]   view->ProcessUrlFragment called with should_scroll_to_fragment = 1
[FrameLoader.cpp(1179)] RestoreScrollPositionAndViewState called
[FrameLoader.cpp(1188)]   early exit
[FrameLoader.cpp(1179)] RestoreScrollPositionAndViewState called
[FrameLoader.cpp(1188)]   early exit
=> scroll y-position = 100px

[navigation_handle_impl.cc(189)] New navigation
[FrameLoader.cpp(1334)] ProcessFragment called
[FrameLoader.cpp(1366)]   view->ProcessUrlFragment called with should_scroll_to_fragment = 1
[FrameLoader.cpp(1179)] RestoreScrollPositionAndViewState called
[FrameLoader.cpp(1285)]  Set did_restore_from_history = true
[FrameLoader.cpp(1179)] RestoreScrollPositionAndViewState called
[FrameLoader.cpp(1285)]  Set did_restore_from_history = true
=> scroll y-position = 100px

```

# If response's size >= 2 * loading_buffer_size + 1 bytes
```
[navigation_handle_impl.cc(189)] New navigation
[FrameLoader.cpp(1179)] RestoreScrollPositionAndViewState called
[FrameLoader.cpp(1188)]   early exit
[FrameLoader.cpp(1179)] RestoreScrollPositionAndViewState called
[FrameLoader.cpp(1188)]   early exit
[FrameLoader.cpp(1334)] ProcessFragment called
[FrameLoader.cpp(1366)]   view->ProcessUrlFragment called with should_scroll_to_fragment = 1
=> scroll y-position = 100px

[navigation_handle_impl.cc(189)] New navigation
[FrameLoader.cpp(1179)] RestoreScrollPositionAndViewState called
[FrameLoader.cpp(1285)]  Set did_restore_from_history = true
[FrameLoader.cpp(1179)] RestoreScrollPositionAndViewState called
[FrameLoader.cpp(1285)]  Set did_restore_from_history = true
[FrameLoader.cpp(1334)] ProcessFragment called
[FrameLoader.cpp(1366)]   view->ProcessUrlFragment called with should_scroll_to_fragment = 0
=> scroll y-position = 0px
```

I am assigning the issue to myself for today. I will try getting familiar with this two functions.

One quick question: in the case of a reload, why does the scroll y-position is set to 100px?
A) Because the URL has the #d2 fragment
B) Because it is restored from history.

During a reload, the history restore "wins" over the fragment.  You can see this by:

1. Load a large page with a fragment, e.g. https://en.wikipedia.org/wiki/Cat#Behavior
2. Manually scroll to some other part of the page
3. Press Reload button

After reload the URL still has the original fragment, but the scroll position after reload is the same as it is after step 2.  This is why ProcessFragment sets should_scroll_to_fragment = false if we have already restored from history.

Thanks skobes@ for this confirmation. It is very useful. What I am seeing is the "restored" value is the wrong one (0px) and the value from the fragment is correct (100px). Depending on which on takes place first, the y-scroll-position is either correct(100px) or incorrect(0px).

I need to work on other issues. I am unassigning myself in case someone want to take a look. I will came back to it later.

I came back to this issue.
I think I understand why the test fails when response's size > 64ko.

In the test, there are two scrollable area.
* The main view.
* A div with "overflow-y: scroll"

After the first navigation, a scroll happens to the anchor.
* The document doesn't scroll (0px, 0px).
* The div is scrolled to (0px, 100px).

Then a reload happens, the previous view state is saved, but only the one for the main view (0px, 0px).

On reload, if RestoreScrollPositionAndViewState() is called before ProcessFragment, then the main view scroll is restored (0px, 0px), but not div's scroll. I think this is the issue.

It looks like there is a part in RestoreScrollPositionAndViewState() where it attempts to restore the anchor:
┌──┬─────────────────────────────────────────────────────────────────────────┐
│1 │// TODO(pnoland): attempt to restore the anchor in more places than this.│
│2 │// Anchor-based restore should allow for earlier restoration.            │
│3 │bool did_restore =                                                       │
│4 │    ShouldSerializeScrollAnchor() &&                                     │
│5 │    view->LayoutViewportScrollableArea()->RestoreScrollAnchor(           │
│6 │        {view_state->scroll_anchor_data_.selector_,                      │
│7 │         LayoutPoint(view_state->scroll_anchor_data_.offset_.x,          │
│8 │                     view_state->scroll_anchor_data_.offset_.y),         │
│9 │         view_state->scroll_anchor_data_.simhash_});                     │
│10│if (!did_restore) {                                                      │
│11│  view->LayoutViewportScrollableArea()->SetScrollOffset(                 │
│12│      view_state->scroll_offset_, kProgrammaticScroll);                  │
│13│}                                                                        │
│14│                                                                         │
│15│did_restore |= (previous_offset !=                                       │
│16│                view->LayoutViewportScrollableArea()->GetScrollOffset());│
└──┴─────────────────────────────────────────────────────────────────────────┘
But it is disabled by default. I tried to enable it by default, using: "enable-blink-features=ScrollAnchorSerialization", but it didn't improved anything. I have:
[1:1:0406/152710.569093:ERROR:FrameLoader.cpp(1228)] view_state->scroll_anchor_data_.selector_ = ""
[1:1:0406/152710.569232:ERROR:FrameLoader.cpp(1229)] view_state->scroll_anchor_data_.offset_.x = 0
[1:1:0406/152710.569382:ERROR:FrameLoader.cpp(1230)] view_state->scroll_anchor_data_.offset_.y = 0

+CC pnloand: FYI.


skobes@ Does my explanation makes sense?

I don't think there is an issue. We probably only need to update the two tests.
They are testing that FrameLoader::RestoreScrollPositionAndViewState() restore the view. The thing is that this function only tries to restore the 'main' view and the test is about restoring the view inside the 'div'.

I will update the two tests such that it is the 'main' view that scrolls and not the 'div's one.

Re. #5: yes that makes sense.  Fragment scroll may scroll <div> containers to make the fragment visible, but history restore (today) only scrolls the document. 
 Therefore the history restore may not entirely override the side effects of the fragment scroll.  So there is some non-determinism here, but I'm not sure offhand how to fix it.

Thanks for the confirmation!
I opened a new task (issue 839292). It better describe what happens than this thread.

I guess this issue will be fixed by itself when history restore tries to restore the more than the root area.

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

commit c13c16a87cd7bfb4b4883311cd1a2b7343311a57
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Thu May 03 14:53:33 2018

Update two scroll tests.

This updates the following tests:
- NavigationControllerBrowserTest.ReloadWithUrlAnchor
- NavigationControllerBrowserTest.ReloadWithUrlAnchorAndScroll

This two tests are making a <div> to scroll because of a navigation to
an anchor. Then the tests check the scroll position is restored after a
reload.

The issue is that it is not deterministic when the scrolled area is not
the root area. It is working consistently only because the main
resource's response is small (e.g <64ko) and
FrameLoader::ProcessFragment() is called before
FrameLoader::RestoreScrollPositionAndViewState().

This CL make the tests uses the root area instead of the <div>.

  ---

Description of what happens in the current code:

1) When ProcessFragment() is called first:

  a) ProcessFragment():
  Several scrollable layers are scrolled such that the anchor is
  displayed. It may includes the root one, but also some children.

  b) RestoreScrollPositionAndViewState():
  The root layer is scrolled using what is stored in the HistoryItem. It
  overrides the scroll done in a) for the root layer. Children layers
  are not overridden.

2) When RestoreScrollPositionAndViewState() is called first:

  a) RestoreScrollPositionAndViewState():
  The root layer is scrolled using what is stored in the HistoryItem.
  Children layers are not scrolled.

  b) ProcessFragment():
  Nothing happens because InitialScrollState::did_restore_from_history
  has been set to true in a). The ProcessFragment() will skip the
  fragment scroll.

At the end, the difference is that the children layers are scrolled in
1), but not in 2).

  ---

Bug:  821877 , 831155, 839292
Change-Id: I11c74224ba42ad58814c4d4e185d7db189551775
Reviewed-on: https://chromium-review.googlesource.com/1039827
Reviewed-by: Jianpeng Chao <chaopeng@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555727}
[modify] https://crrev.com/c13c16a87cd7bfb4b4883311cd1a2b7343311a57/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/c13c16a87cd7bfb4b4883311cd1a2b7343311a57/content/test/data/navigation_controller/reload-with-url-anchor.html