New issue
Advanced search Search tips

Issue 903497 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 4
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Navigation gets "committed" too early for provisional failures

Project Member Reported by eugene...@chromium.org, Nov 8

Issue description

DiFinishNavigation, PageLoaded and URL change events happen when didFailProvisionalNavigation is called.

These events should happen after placeholder URL is loaded.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 9

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

commit 386114b5bacc02e12b584786671d67b6080f02e7
Author: Eugene But <eugenebut@google.com>
Date: Fri Nov 09 16:00:50 2018

Call WebStateObserver::PageLoaded after loading placeholder URL.

Before this CL PageLoaded was called right from
didFailProvisionalNavigation:, where URL may still represent the
previous page.

Bug:  903497 
Change-Id: I712dd120340353f37d2fe739c5b05e843cb11d7a
Reviewed-on: https://chromium-review.googlesource.com/c/1327608
Reviewed-by: Danyao Wang <danyao@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606855}
[modify] https://crrev.com/386114b5bacc02e12b584786671d67b6080f02e7/ios/web/web_state/ui/crw_web_controller.mm

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 19

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

commit 7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3
Author: Eugene But <eugenebut@chromium.org>
Date: Mon Nov 19 19:38:34 2018

Add web::NavigationContext::IsPlaceholderNavigation

This is a step in multistep refactoring. The next steps will be:

1.) web::NavigationContext::URL is never set to placeholder
    URL and always represents navigation URL
2.) Same web::NavigationContext will be reused for placeholder
    navigation to extend it's lifetime.
3.) WebStateObserver::DidFinishNavigation will be caller after
    placeholder navigation is finished and will use original
    web::NavigationContext passed to WebStateObserver::DidStartNavigation

This will partially fix  crbug.com/903497  and will call
WebStateObserver::DidFinishNavigation after committed URL actually
changed.

Bug:  903497 
Change-Id: Ib6ba96d664ac352d038787aa74ac844d07282789
Reviewed-on: https://chromium-review.googlesource.com/c/1331044
Reviewed-by: Danyao Wang <danyao@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609398}
[modify] https://crrev.com/7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3/ios/chrome/browser/tabs/BUILD.gn
[modify] https://crrev.com/7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3/ios/chrome/browser/tabs/DEPS
[modify] https://crrev.com/7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3/ios/web/web_state/BUILD.gn
[modify] https://crrev.com/7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3/ios/web/web_state/navigation_context_impl.h
[modify] https://crrev.com/7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3/ios/web/web_state/navigation_context_impl.mm
[modify] https://crrev.com/7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3/ios/web/web_state/web_state_impl.h
[modify] https://crrev.com/7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3/ios/web/web_state/web_state_impl.mm
[modify] https://crrev.com/7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3/ios/web/web_state/web_state_impl_unittest.mm

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 19

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

commit 578c90a6596ae90b476b691867e3e5471090f20b
Author: Jun Choi <hongjunchoi@chromium.org>
Date: Mon Nov 19 23:18:33 2018

Revert "Add web::NavigationContext::IsPlaceholderNavigation"

This reverts commit 7b0f2bb852e0280fb3ce78860f7691e4e64ae6f3.

Reason for revert: Fails multiple IOS builds including Builder ios-slimnav <https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ios-slimnav/191>

Link to sample failure logs : https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8929398908718170816/+/steps/ios_chrome_unittests__iPhone_6s_iOS_12.1_/0/logs/TextToSpeechListenerTest.ValidAudioDataTest/0


Original change's description:
> Add web::NavigationContext::IsPlaceholderNavigation
> 
> This is a step in multistep refactoring. The next steps will be:
> 
> 1.) web::NavigationContext::URL is never set to placeholder
>     URL and always represents navigation URL
> 2.) Same web::NavigationContext will be reused for placeholder
>     navigation to extend it's lifetime.
> 3.) WebStateObserver::DidFinishNavigation will be caller after
>     placeholder navigation is finished and will use original
>     web::NavigationContext passed to WebStateObserver::DidStartNavigation
> 
> This will partially fix  crbug.com/903497  and will call
> WebStateObserver::DidFinishNavigation after committed URL actually
> changed.
> 
> Bug:  903497 
> Change-Id: Ib6ba96d664ac352d038787aa74ac844d07282789
> Reviewed-on: https://chromium-review.googlesource.com/c/1331044
> Reviewed-by: Danyao Wang <danyao@chromium.org>
> Commit-Queue: Eugene But <eugenebut@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#609398}

TBR=eugenebut@chromium.org,danyao@chromium.org

Change-Id: I8d7c9b66d752a792761d95ccf348f045eaa9de03
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  903497 
Reviewed-on: https://chromium-review.googlesource.com/c/1343351
Reviewed-by: Jun Choi <hongjunchoi@chromium.org>
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609494}
[modify] https://crrev.com/578c90a6596ae90b476b691867e3e5471090f20b/ios/chrome/browser/tabs/BUILD.gn
[modify] https://crrev.com/578c90a6596ae90b476b691867e3e5471090f20b/ios/chrome/browser/tabs/DEPS
[modify] https://crrev.com/578c90a6596ae90b476b691867e3e5471090f20b/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/578c90a6596ae90b476b691867e3e5471090f20b/ios/web/web_state/BUILD.gn
[modify] https://crrev.com/578c90a6596ae90b476b691867e3e5471090f20b/ios/web/web_state/navigation_context_impl.h
[modify] https://crrev.com/578c90a6596ae90b476b691867e3e5471090f20b/ios/web/web_state/navigation_context_impl.mm
[modify] https://crrev.com/578c90a6596ae90b476b691867e3e5471090f20b/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/578c90a6596ae90b476b691867e3e5471090f20b/ios/web/web_state/web_state_impl.h
[modify] https://crrev.com/578c90a6596ae90b476b691867e3e5471090f20b/ios/web/web_state/web_state_impl.mm
[modify] https://crrev.com/578c90a6596ae90b476b691867e3e5471090f20b/ios/web/web_state/web_state_impl_unittest.mm

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 21

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

commit 4a4dede926b598761008de679cb71bc40febbc23
Author: Eugene But <eugenebut@chromium.org>
Date: Wed Nov 21 22:33:57 2018

Reland Add web::NavigationContext::IsPlaceholderNavigation

This is a step in multistep refactoring. The next steps will be:

1.) web::NavigationContext::URL is never set to placeholder
    URL and always represents navigation URL
2.) Same web::NavigationContext will be reused for placeholder
    navigation to extend it's lifetime.
3.) WebStateObserver::DidFinishNavigation will be caller after
    placeholder navigation is finished and will use original
    web::NavigationContext passed to WebStateObserver::DidStartNavigation

This will partially fix  crbug.com/903497  and will call
WebStateObserver::DidFinishNavigation after committed URL actually
changed.

Originally Reviewed-on: https://chromium-review.googlesource.com/c/1331044

Bug:  903497 
Change-Id: I0ee2ecdfb4d20d2f14fd62e99028e8de1b81ff3b
Reviewed-on: https://chromium-review.googlesource.com/c/1347212
Reviewed-by: Danyao Wang <danyao@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610238}
[modify] https://crrev.com/4a4dede926b598761008de679cb71bc40febbc23/ios/chrome/browser/tabs/BUILD.gn
[modify] https://crrev.com/4a4dede926b598761008de679cb71bc40febbc23/ios/chrome/browser/tabs/DEPS
[modify] https://crrev.com/4a4dede926b598761008de679cb71bc40febbc23/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/4a4dede926b598761008de679cb71bc40febbc23/ios/web/web_state/BUILD.gn
[modify] https://crrev.com/4a4dede926b598761008de679cb71bc40febbc23/ios/web/web_state/navigation_context_impl.h
[modify] https://crrev.com/4a4dede926b598761008de679cb71bc40febbc23/ios/web/web_state/navigation_context_impl.mm
[modify] https://crrev.com/4a4dede926b598761008de679cb71bc40febbc23/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/4a4dede926b598761008de679cb71bc40febbc23/ios/web/web_state/web_state_impl.h
[modify] https://crrev.com/4a4dede926b598761008de679cb71bc40febbc23/ios/web/web_state/web_state_impl.mm
[modify] https://crrev.com/4a4dede926b598761008de679cb71bc40febbc23/ios/web/web_state/web_state_impl_unittest.mm

Summary: Navigation gets "committed" too early for provisional failures (was: Navigation gets "committed" to early for provisional failures)
Status: Started (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 3

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

commit 81321bf5213e465220d5c9515c1786d7b9c9011e
Author: Danyao Wang <danyao@chromium.org>
Date: Mon Dec 03 21:47:48 2018

[ios] Remove |loadNativeViewWithSuccess| and inline its content.

This makes it more clear which WebStateObserver callbacks are called
when. This change does not contain any function changes.

Bug:  903497 
Change-Id: Ie774c191ffc2681a7cf1df90089ea25e90a381ac
Reviewed-on: https://chromium-review.googlesource.com/c/1359296
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613265}
[modify] https://crrev.com/81321bf5213e465220d5c9515c1786d7b9c9011e/ios/web/web_state/ui/crw_web_controller.mm

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 4

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

commit b129becf000ebcb19d568da0ff5173028f38ee69
Author: Danyao Wang <danyao@chromium.org>
Date: Tue Dec 04 15:15:39 2018

[ios] Make |loadCompleteWithSuccess| use web::NavigationContext.

This makes it easier in the next step of the refactor to move
|loadCompleteWithSuccess| call to |loadErrorPageForNavigationItem|.

Bug:  903497 
Change-Id: I988c17afee10b56ac9baf720c147a59624afc9f3
Reviewed-on: https://chromium-review.googlesource.com/c/1359320
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613554}
[modify] https://crrev.com/b129becf000ebcb19d568da0ff5173028f38ee69/ios/web/web_state/ui/crw_web_controller.mm

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 4

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

commit 66d735a8758616bcedf19cefce8554d06de16b50
Author: Danyao Wang <danyao@chromium.org>
Date: Tue Dec 04 16:39:11 2018

[ios] Extend the life time of navigation context to error placeholder.

Also deferred WebStateObserver::DidFinishNavigation callback to after
placeholder load finishes. This removes an inconsistency between legacy
nav and slim nav handling of errors.

Bug:  903497 
Change-Id: I7ee20859b3293da305e2a0ba15d017ea7be0ced8
Reviewed-on: https://chromium-review.googlesource.com/c/1357613
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613573}
[modify] https://crrev.com/66d735a8758616bcedf19cefce8554d06de16b50/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/66d735a8758616bcedf19cefce8554d06de16b50/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/66d735a8758616bcedf19cefce8554d06de16b50/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/66d735a8758616bcedf19cefce8554d06de16b50/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm
[modify] https://crrev.com/66d735a8758616bcedf19cefce8554d06de16b50/ios/web/web_state/web_state_observer_inttest.mm

Status: Fixed (was: Started)
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 4

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

commit d54722d9763c73eba287f436365fc0542d88a2fa
Author: Robert Flack <flackr@chromium.org>
Date: Tue Dec 04 17:54:40 2018

Revert "[ios] Extend the life time of navigation context to error placeholder."

This reverts commit 66d735a8758616bcedf19cefce8554d06de16b50.

Reason for revert: Suspected cause for the new failures on ios-webview
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ios-webview/2465

Original change's description:
> [ios] Extend the life time of navigation context to error placeholder.
> 
> Also deferred WebStateObserver::DidFinishNavigation callback to after
> placeholder load finishes. This removes an inconsistency between legacy
> nav and slim nav handling of errors.
> 
> Bug:  903497 
> Change-Id: I7ee20859b3293da305e2a0ba15d017ea7be0ced8
> Reviewed-on: https://chromium-review.googlesource.com/c/1357613
> Commit-Queue: Danyao Wang <danyao@chromium.org>
> Reviewed-by: Eugene But <eugenebut@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#613573}

TBR=eugenebut@chromium.org,danyao@chromium.org

Change-Id: Ib83d75bca88feb7833f1100c297391caaae6d355
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  903497 
Reviewed-on: https://chromium-review.googlesource.com/c/1361640
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613593}
[modify] https://crrev.com/d54722d9763c73eba287f436365fc0542d88a2fa/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/d54722d9763c73eba287f436365fc0542d88a2fa/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/d54722d9763c73eba287f436365fc0542d88a2fa/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/d54722d9763c73eba287f436365fc0542d88a2fa/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm
[modify] https://crrev.com/d54722d9763c73eba287f436365fc0542d88a2fa/ios/web/web_state/web_state_observer_inttest.mm

Project Member

Comment 12 by bugdroid1@chromium.org, Dec 4

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

commit a4f95055278cb4d732b0a7c310bb38cb962070e4
Author: Danyao Wang <danyao@chromium.org>
Date: Tue Dec 04 23:57:05 2018

Reland "[ios] Extend the life time of navigation context to error placeholder."

This is a reland of 66d735a8758616bcedf19cefce8554d06de16b50 after fixing the
broken tests.

Original change's description:
> [ios] Extend the life time of navigation context to error placeholder.
>
> Also deferred WebStateObserver::DidFinishNavigation callback to after
> placeholder load finishes. This removes an inconsistency between legacy
> nav and slim nav handling of errors.
>
> Bug:  903497 
> Change-Id: I7ee20859b3293da305e2a0ba15d017ea7be0ced8
> Reviewed-on: https://chromium-review.googlesource.com/c/1357613
> Commit-Queue: Danyao Wang <danyao@chromium.org>
> Reviewed-by: Eugene But <eugenebut@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#613573}

Bug:  903497 
Change-Id: I911a5b76c62f9f27f6c32f1981cdb5476c1db10c
Reviewed-on: https://chromium-review.googlesource.com/c/1362000
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613768}
[modify] https://crrev.com/a4f95055278cb4d732b0a7c310bb38cb962070e4/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/a4f95055278cb4d732b0a7c310bb38cb962070e4/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/a4f95055278cb4d732b0a7c310bb38cb962070e4/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/a4f95055278cb4d732b0a7c310bb38cb962070e4/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/a4f95055278cb4d732b0a7c310bb38cb962070e4/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm
[modify] https://crrev.com/a4f95055278cb4d732b0a7c310bb38cb962070e4/ios/web/web_state/web_state_observer_inttest.mm
[modify] https://crrev.com/a4f95055278cb4d732b0a7c310bb38cb962070e4/ios/web_view/test/navigation_delegate_inttest.mm

Sign in to add a comment