New issue
Advanced search Search tips

Issue 905688 link

Starred by 1 user
Status: Fixed
Owner:
danyao@chromium.org
Closed: Nov 16
Cc:
eugene...@chromium.org
justincohen@chromium.org
ios-bugs-priority@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug-Regression

Blocking:
issue 807428



Sign in to add a comment

NavigationTestCase/testRestoreHistoryToNTPAndNavigateForward fails with new NTP and WKBasedNavigationManager

Project Member Reported by danyao@chromium.org, Nov 15 
NavigationTestCase/testRestoreHistoryToNTPAndNavigationForward is failing on ToTT (0e55d4452afdab2949faf5141aac8224626bcefc) when --enable-features=SlimNavigationManager is used.

Failure message:
../../ios/chrome/test/earl_grey/chrome_earl_grey.mm:215: error: -[NavigationTestCase testRestoreHistoryToNTPAndNavigateForward] : Exception: AssertionFailedException

Exception Name: AssertionFailedException
Exception Reason: (([condition waitWithTimeout:base::test::ios::kWaitForUIElementTimeout]) is true) failed
Exception Details: Failed waiting for web view containing pony

The problem seems to be that the go forward after a session restore doesn't dismiss the NTP.

I can reproduce this by going through the same steps in the test manually:
1. Load NTP
2. Navigate to any page (e.g. wikipedia.org)
3. Tap Back button
4. Force app crash (in simulator, just relaunch the simulation)
5. Wait for NTP is restored
6. Tap Forward button

Expected: wikipedia.org should load
Actual: NTP is still visible, but omnibox is show at the normal location of a web view.

Video is attached.
restore_ntp_bug.mov
2.5 MB View Download

Comment 1 by danyao@chromium.org, Nov 15 

Here is a log of navigation events that took place after tapping "Restore":

// This is the start of the session restore

- URL did change: file:///.../Chromium.app/restore_session.html#session=%7B%22offset%22%3A-1%2C%22titles%22%3A%5B%22New%20Tab%22%2C%22Wikipedia%2C%20the%20free%20encyclopedia%22%5D%2C%22urls%22%3A%5B%22about%3A%2F%2Fnewtab%2F%22%2C%22https%3A%2F%2Fen.m.wikipedia.org%2Fwiki%2FMain_Page%22%5D%7D isLoading: 1
- loading changed: 1
- didStart [<WKNavigation: 0x7f85fe1312a0>] file:///.../Chromium.app/restore_session.html#session=%7B%22offset%22%3A-1%2C%22titles%22%3A%5B%22New%20Tab%22%2C%22Wikipedia%2C%20the%20free%20encyclopedia%22%5D%2C%22urls%22%3A%5B%22about%3A%2F%2Fnewtab%2F%22%2C%22https%3A%2F%2Fen.m.wikipedia.org%2Fwiki%2FMain_Page%22%5D%7D
- didCommit [<WKNavigation: 0x7f85fe1312a0>] file:///.../Chromium.app/restore_session.html#session=%7B%22offset%22%3A-1%2C%22titles%22%3A%5B%22New%20Tab%22%2C%22Wikipedia%2C%20the%20free%20encyclopedia%22%5D%2C%22urls%22%3A%5B%22about%3A%2F%2Fnewtab%2F%22%2C%22https%3A%2F%2Fen.m.wikipedia.org%2Fwiki%2FMain_Page%22%5D%7D


// This is history.replaceState(about:newtab)

- URL did change: file:///.../Chromium.app/restore_session.html#targetUrl=about%3A%2F%2Fnewtab%2F isLoading: 1

// This is history.pushState(wikipedia.org)

- URL did change: file:///.../Chromium.app/restore_session.html#targetUrl=https%3A%2F%2Fen.m.wikipedia.org%2Fwiki%2FMain_Page isLoading: 1
- loading changed: 0
- didFinish [<WKNavigation: 0x7f85fe1312a0>] file:///.../Chromium.app/restore_session.html#targetUrl=https%3A%2F%2Fen.m.wikipedia.org%2Fwiki%2FMain_Page


// Start to reload the last visible page, about:newtab. This is triggered by history.go(-1)

- URL did change: file:///.../Chromium.app/restore_session.html#targetUrl=about%3A%2F%2Fnewtab%2F isLoading: 0
- loading changed: 1
- didStart [<WKNavigation: 0x7f85f8f016a0>] file:///.../Chromium.app/restore_session.html#targetUrl=about%3A%2F%2Fnewtab%2F
- didCommit [<WKNavigation: 0x7f85f8f016a0>] file:///.../Chromium.app/restore_session.html#targetUrl=about%3A%2F%2Fnewtab%2F
- loading changed: 0
- didFinish [<WKNavigation: 0x7f85f8f016a0>] file:///.../Chromium.app/restore_session.html#targetUrl=about%3A%2F%2Fnewtab%2F

// This is triggered by location.replace(about:newtab)

- URL did change: about://newtab/ isLoading: 1
- loading changed: 1
- didStart [<WKNavigation: 0x7f85f6518a80>] about://newtab/
- didCommit [<WKNavigation: 0x7f85f6518a80>] about://newtab/
- loading changed: 0
- didFinish [<WKNavigation: 0x7f85f6518a80>] about://newtab/

// -- Now history is fully restored, and NTP is visible.

// Tap Forward button

- URL did change: file:///.../Chromium.app/restore_session.html#targetUrl=https%3A%2F%2Fen.m.wikipedia.org%2Fwiki%2FMain_Page isLoading: 1
- loading changed: 1
- loading changed: 0

What should happen is that this page should redirect to wikipedia.org. So I suspect some logic in [CRWWebController -URLDidChangeWithoutDocumentChange] is not working as expected now that the BrowserContainerContainsNTP feature uses about:newtab. I'll dig a bit more.

Comment 2 by danyao@chromium.org, Nov 15

Labels: OS-iOS
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 15 

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

commit 4ca449e4c3aaeb36393c855ef259c5ee9d028e84
Author: Danyao Wang <danyao@chromium.org>
Date: Thu Nov 15 21:47:02 2018

[Nav Experiment] Temporarily disable broken NavigationTestCase test.

Bug:  905688 
Change-Id: I11f0b6320ba8221139323a285199a5570afab429
Reviewed-on: https://chromium-review.googlesource.com/c/1338308
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608526}
[modify] https://crrev.com/4ca449e4c3aaeb36393c855ef259c5ee9d028e84/ios/chrome/browser/web/navigation_egtest.mm
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 16 

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

commit 6bd66a17cc064a97ccda63b35921c3fa55731ab4
Author: Danyao Wang <danyao@chromium.org>
Date: Fri Nov 16 22:18:32 2018

[Nav Experiment] Handle about:newtab when traversing restored history.

Navigating away from an app-specific URL in restored session history is
a special case that requires a reload to trigger theclient-side redirect
in restore_session.html. about:newtab, which is used by the
BrowserContainerContainsNTP feature, should be treated as an app-specific
URL for this case.

This fixes the regression in
NavigationTestCase/testRestoreHistoryToNTPAndNavigateForward
after enabling BrowserContainerContainsNTP.

Bug:  905688 
Change-Id: I96d4612152a003f3c7275720a01eb6376c3ff806
Reviewed-on: https://chromium-review.googlesource.com/c/1340761
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608982}
[modify] https://crrev.com/6bd66a17cc064a97ccda63b35921c3fa55731ab4/ios/chrome/browser/web/navigation_egtest.mm
[modify] https://crrev.com/6bd66a17cc064a97ccda63b35921c3fa55731ab4/ios/web/web_state/ui/crw_web_controller.mm

Comment 5 by danyao@chromium.org, Nov 16

Status: Fixed (was: Assigned)
►

Sign in to add a comment