New issue
Advanced search Search tips

Issue 819805 link

Starred by 0 users

Issue metadata

Status: Verified
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug



Sign in to add a comment

Offline articles are not loaded with slim-navigation-manger enabled.

Project Member Reported by srikanthg@chromium.org, Mar 7 2018

Issue description

App Version: 66.0.3359.12 beta
iOS Version:  10.3.3, 11.2.6
Device: iPhone, iPad
URL: any

Precondition: Enable #slim-navigation-manager from about://flags

Steps to reproduce:
  1. Launch Google Chrome
  2. Navigate to any webpage and add to reading list
  3. Wait for the article tobe ready for offline view
  4. Turn on Airplane mode and open the article from reading list

Observed results: No internet connection error page is displayed.

Expected results: Article should be displayed for offline view.

Note: Article loads fine when long tap and select Veiw Offline version in new tab. But again reloading this page doesn't work

Number of times you were able to reproduce: 5/5
Bug reproducible after clean install: Yes
Bug reproducible after clearing cache and cookies: Yes
Bug reproducible on Chrome Mobile on Android: NA
Bug reproducible on Safari/Firefox: Firefox: NA, Safari: NA
Bug reproducible on current stable build (App Version, iOS Version): M65 NA New feature
Bug reproducible on the current beta channel build (App Version, iOS Version): M66 YES

Link to video/image: https://drive.google.com/file/d/1A11JbAPBxZ-1ZqYSfCjVHPis5ZUezgMO/view 
 
Cc: -danyao@chromium.org olivierrobin@chromium.org
Owner: danyao@chromium.org
Status: Assigned (was: Untriaged)

Comment 2 by danyao@chromium.org, Mar 27 2018

This is caused by a conflict between native error view and reading list. The following happens after step 4:

5. Original URL of the reading list item is loaded in web view
6. Since we're in Airplane Mode, |webView:didFailProvisionalNavigation| is called.
7. Native error handling triggers a load of placeholder URL (about:blank?for=originalURL) in the web view, in preparation for presenting native error
8. |webView:didFinishNavigation| is called for placeholder URL. It calls CRWWebController's |didFinishNavigation|, which calls WebState::OnPageLoaded().
9. ReadingListWebStateObserver::PageLoaded() is called with *load_completion_status = SUCCESS*, which early exits before loading offline content.

In step 9, load_completion_status should be FAILURE. Changing this will cause ReadingListWebStateObserver::LoadOfflineReadingListEntry() to run (good). However, LoadOfflineReadingListEntry() also modifies the NavigationItem's URL (from originalURL to chrome://offline...) and reloads the item. Because WKBasedNavigationManager delegates to WKWebView for reload, changing NavigationItem's URL has no effect, so about:blank?for=originalURL is loaded again. When it reaches the |webView:didFinishNavigation| stage, it early exists (incorrectly) at this condition:

} else if (item->GetURL() != originalURL) {
  // The |didFinishNavigation| callback can arrive after another
  // navigation has started. Abort in this case.
  return;
}

What it should do is recognize that this is an offline content load and directly triggers the offline content native controller. Care needs to be taken to make sure the NavigationItem's ErrorRetryState stays in DisplayingErrorForFailedNavigation, such that a subsequent reload will attempt to reload the original URL again (this requires ReadingListWebStateObserver to not change the navigation item's URL).
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 5 2018

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

commit e423142a5bdd917f867ee23c3f36158183b0b8b6
Author: Danyao Wang <danyao@google.com>
Date: Thu Apr 05 16:44:05 2018

[Nav Experiment] Fix offline reading list for WKBasedNavigationManager.

When handling a no-Internet-connection error, WKBasedNavigationManager
incorrectly triggers WebState::OnPageLoaded with load completion status
SUCCESS when the native error view is loaded. This prevented
ReadingListWebStateObserver from triggering and loading offline content
and is now fixed.

Since offline reading list is essentially another type of native error,
we must not update the navigation item's |errorRetryState| when loading
a chrome://offline URL. This is so that a reload of the page will retry
loading the original URL. This is fixed by only updating the error retry
state machine when the URL is a web URL. This is OK because app-specific
URL should never fail to load.

Bug:  819805 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I870ef99522ba6f5aeb4d37c58289155aa8cb79c9
Reviewed-on: https://chromium-review.googlesource.com/996434
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548454}
[modify] https://crrev.com/e423142a5bdd917f867ee23c3f36158183b0b8b6/ios/web/web_state/ui/crw_web_controller.mm

Status: Fixed (was: Assigned)
Status: Verified (was: Fixed)
Verified on chrome canary version 67.0.3393.0 on iPhone 8 plus with iOS 11.4 beta 1 and iPad Pro with iOS 11.2.6 following the steps mentioned in comment #0.  Article are displayed in offline view.  Looks good.

Sign in to add a comment