This is caused by https://codereview.chromium.org/2756753002

Eugene, is there something I need to change to load with the new navigation code?

I reverted CL/2756753002 and still can reproduce the issue. From my understanding this CL does not change the behavior, but only removes dead code. Olivier, are you sure that CL/2756753002 is a culprit?

Could it be related to crbug.com/702431?

Issue 702431 was only related to preload Tabs, and it doesn't look like this is the case given the repro steps in comment #1.

More likely caused by https://codereview.chromium.org/2741413007

Olivier, is it possible to add an EG test to prevent this type of regressions?

There is a CL in flight that both fix the problem and adds EG tests.
I am polishing it an I will send it to review tonight (French time).

If cl/2741413007 introduced a bug then maybe instead of putting a workaround into chrome layer we should fix the bug in ios/web? Yuke, can you fix newly written reload code, so it does not break reading list?

Sure, I'll look into it ASAP

To be more specific on what changed, it is the fact that |navigationManager reload| now calls |webController reload|, which in turn call |nativeContent reload|.
- So when displaying the offline page, there is an error page displayed. |webController reload| calls |ErrorPageContent reload| which erase the chrome://offline url
- When navigating from the offline page, calling |navigationManager reload| from |nativeContent reload| now leads to circular call.

Thanks Olivier. Kurt, do you think that current Reload behavior is WAI? Oliver already has a fix in chrome layer.

I'm not exactly sure when Reload() is called in the Reading List flow.  Just to clarify we're rewriting the current NavigationItem's URL (and virtual URL), then calling reload to load the offline version, right?  I know we have that logic where if the page is loading too slowly, we'll fall back to the offline version, but I'm unfamiliar with the flow for when we're displaying an error page.  I think ideally, we'd want to catch the offline error before ErrorPageContent is displayed, and display the offline Reading List version.

In any case, I think CRWWebController's |-reload| should be updated to handle error pages; I'm going to update the implementation now (see blocking bug).  This should help with the scenario being described here, but I still think that it'd be better behavior to use a native Reading List view instead of showing an ErrorPageContents before reloading.

For clarification on the current behavior:
- We trigger the offline page either if the loading is too long, or of the page loading failed (WebStateObserver::Pageloaded(FAILURE) is called).
At that point, the error page is already created.

If we can branch our failure detection earlier, I have no objection at all.

I see.  Is there a brief flash of the error page before displaying the offline mode?  Or does it all occur within the same runloop, so there are no animation glitches when displaying the offline version?  I created a CL that will fix the reload behavior to use the NavigationItem's URL rather than the native content, which should restore old behavior: https://codereview.chromium.org/2765903003/

IIUC, it is in the same runloop.

I commented on your CL. I think we need an extra test in it.

Unfortunately your comment on the CL won't help because it'd still go through to the ErrorPageContent's |-reload|.  It's probably best to use your chrome-layer workaround in addition to my fix.

You are right.
I have no objection in committing your fix and my workaround.

Another possible solution would be to make |CRWNativeContent reload| return a BOOL saying if it handled the reload.
As ErrorPageContent has no reason to handle it, it should just return NO and WebController would handle the reload.

We going to get rid of NativeContent eventually, so maybe we should not put too much efforts into this.

Another option might be adding a condition to |CRWWebController reload| so that we can reset the native controller to OfflinePageNativeController, but we don't really want the web layer to know anything about Reading List.

ErrorPageContent is actually behaving correctly on Reload().  Reloading the error page should attempt to reload the URL that caused the error.  Since the ErrorPageContent is created with the online URL before we get a chance to rewrite the current NavigationItem's URL to the offline version, reloading the page will attempt reloading the online URL, which will create a new ErrorPageContent.

A more robust solution would be a version of what Yuke is suggesting.  We could cache the last URL that was loaded, and in CRWWebController's |-reload|, we can ask for a new native controller rather than reloading the current one if the current native URL is different than the one that vended the ErrorPageContent.

That being said, I think Eugene has a point about not worrying too much about the native content API since we're trying to get rid of it.  Your GoToItem(GetCurrentNavigationIndex()) hack will work fine to fix this scenario.

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

commit f266b4e8d109a11ac0e3fae0b4c8500281e7c457
Author: olivierrobin <olivierrobin@chromium.org>
Date: Wed Mar 29 11:41:11 2017

Reading List iOS: Adapt loading offline for new reload process

Loading offline pages relies on being able to reload page after
changing the URL.
Adapt to the new reload process which does not allow that.
Also add EG tests for catching future problem.

BUG=703393,  703206 

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

[modify] https://crrev.com/f266b4e8d109a11ac0e3fae0b4c8500281e7c457/ios/chrome/browser/reading_list/reading_list_web_state_observer.mm
[modify] https://crrev.com/f266b4e8d109a11ac0e3fae0b4c8500281e7c457/ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm
[modify] https://crrev.com/f266b4e8d109a11ac0e3fae0b4c8500281e7c457/ios/chrome/browser/ui/reading_list/BUILD.gn
[modify] https://crrev.com/f266b4e8d109a11ac0e3fae0b4c8500281e7c457/ios/chrome/browser/ui/reading_list/offline_page_native_content.mm
[modify] https://crrev.com/f266b4e8d109a11ac0e3fae0b4c8500281e7c457/ios/chrome/browser/ui/reading_list/reading_list_egtest.mm

Reading list offline page is opened correctly in offline.
Verified on M59.0.3062.0 dev
Device: iPhone7, iPad pro
iOS: 10.3, 10.1.1