New issue
Advanced search Search tips

Issue 712269 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Task



Sign in to add a comment

webView:didCommitNavigation: handles correctly only latest navigaiton

Project Member Reported by eugene...@chromium.org, Apr 17 2017

Issue description

WKWebView may have more than one in progress navigation. 
webView:didCommitNavigation: assumes that only the last navigation can be committed, which is not true. webView:didCommitNavigation: should be fixed to handle non-latest committed navigation as well.
 
Possible fix is:
1.) Store NavigationItem in NavigationContext
2.) Associate NavigationContext with WKNavigation

This way webView:didCommitNavigation: will be able to look up for corresponding NavigationItem and make it committed.
When can WKWebView have more than one in-progress navigation?  Conceptually, that doesn't make much sense to me; is it just a matter of the WKNavigation callbacks being interleaved such that a new navigation is committed before we receive the didFail callback for the previous one?  In your suggestion above, how do we choose the corresponding NavigationItem if we only store one pending item?  Is this specific to quickly tapping the back/forward buttons?
>> When can WKWebView have more than one in-progress navigation?
When we ask WKWebView to navigate twice in a raw (f.e. double tab on back button). This is known situation, we just don't handle it everywhere:
https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?q=crw_web_c+package:%5Echromium$&l=4485

For double tap, both navigation may actually succeed, so it's not the case that older navigation fails and newer succeeds. 

We already store multiple WKNavigation objects, so my proposal is to associate them with NavigationContext and every navigation context will have a pointer to NavigationItem. 


>>> Is this specific to quickly tapping the back/forward buttons?
That's the only use case I observed, but I believe there can be other cases as well.


Project Member

Comment 4 by bugdroid1@chromium.org, May 8 2017

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

commit 5ef672aa5ffed4146ede2c1d22a04fb91dd5f168
Author: eugenebut <eugenebut@chromium.org>
Date: Mon May 08 18:58:35 2017

Store NavigationContext in CRWWKNavigationStates.

This CL (1 of 7) is a part of large refactoring to create
NavigationContext when navigation was started, not when it is finished.
This refactoring will allow to pass the same instance of
NavigationContext to DidStartNavigation and DidFinishNavigation
( crbug.com/713836 ). Also this will allow to associate NavigationItem
with NavigationContext object to commit non-latest navigation
( crbug.com/712269 ).

This CL adds API to CRWWKNavigationStates to allow associating
NavigationContextImpl with NKNavigation objects. This will allow to understand
which NavigationContextImpl should be used in WKNavigationDelegate callbacks.

BUG= 713836 , 712269 

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

[modify] https://crrev.com/5ef672aa5ffed4146ede2c1d22a04fb91dd5f168/ios/web/web_state/navigation_context_impl.h
[modify] https://crrev.com/5ef672aa5ffed4146ede2c1d22a04fb91dd5f168/ios/web/web_state/navigation_context_impl.mm
[modify] https://crrev.com/5ef672aa5ffed4146ede2c1d22a04fb91dd5f168/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/5ef672aa5ffed4146ede2c1d22a04fb91dd5f168/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/5ef672aa5ffed4146ede2c1d22a04fb91dd5f168/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Project Member

Comment 5 by bugdroid1@chromium.org, May 8 2017

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

commit e92530274bb519ab0480a83f877ef62b8e681897
Author: eugenebut <eugenebut@chromium.org>
Date: Mon May 08 21:45:31 2017

Return WKNavigation from loadRequest and loadPostRequest.

This CL (2 of 7) is a part of large refactoring to create
NavigationContext when navigation was started, not when it is finished.
This refactoring will allow to pass the same instance of
NavigationContext to DidStartNavigation and DidFinishNavigation
( crbug.com/713836 ). Also this will allow to associate NavigationItem
with NavigationContext object to commit non-latest navigation
( crbug.com/712269 ).

This CL makes load*Request API return WKNavigation object, which can be
used to store NavigationContext in CRWWKNavigationStates.

BUG= 713836 ,  712269 

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

[modify] https://crrev.com/e92530274bb519ab0480a83f877ef62b8e681897/ios/web/web_state/js/crw_js_post_request_loader.h
[modify] https://crrev.com/e92530274bb519ab0480a83f877ef62b8e681897/ios/web/web_state/js/crw_js_post_request_loader.mm
[modify] https://crrev.com/e92530274bb519ab0480a83f877ef62b8e681897/ios/web/web_state/ui/crw_web_controller.mm

Project Member

Comment 6 by bugdroid1@chromium.org, May 9 2017

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

commit f97229b40443d92c57c3a08ebef8aff466932da0
Author: eugenebut <eugenebut@chromium.org>
Date: Tue May 09 04:46:31 2017

Return NavigationContext from registerLoadRequest.

This CL (3 of 7) is a part of large refactoring to create
NavigationContext when navigation was started, not when it is finished.
This refactoring will allow to pass the same instance of
NavigationContext to DidStartNavigation and DidFinishNavigation
( crbug.com/713836 ). Also this will allow to associate NavigationItem
with NavigationContext object to commit non-latest navigation
( crbug.com/712269 ).

This CL makes registerLoadRequest API return NavigationContext object, which
can be used to store NavigationContext in CRWWKNavigationStates.

BUG= 713836 ,  712269 

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

[modify] https://crrev.com/f97229b40443d92c57c3a08ebef8aff466932da0/ios/web/web_state/ui/crw_web_controller.mm

Project Member

Comment 7 by bugdroid1@chromium.org, May 9 2017

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

commit 1000a00b446127c2c0148fe11b2bf2b8223e85b4
Author: eugenebut <eugenebut@chromium.org>
Date: Tue May 09 13:52:33 2017

Added WebStateImpl::OnNavigationFinished().

This CL (4 of 7) is a part of large refactoring to create
NavigationContext when navigation was started, not when it is finished.
This refactoring will allow to pass the same instance of
NavigationContext to DidStartNavigation and DidFinishNavigation
( crbug.com/713836 ). Also this will allow to associate NavigationItem
with NavigationContext object to commit non-latest navigation
( crbug.com/712269 ).

This CL adds WebStateimpl::OnNavigationFinished which simply calls
WebStateObserver::DidFinishNavigation. This method will be used to
pass NavigationContext (which will be created earlier) to DidFinishNavigation.

BUG= 713836 ,  712269 

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

[modify] https://crrev.com/1000a00b446127c2c0148fe11b2bf2b8223e85b4/ios/web/web_state/web_state_impl.h
[modify] https://crrev.com/1000a00b446127c2c0148fe11b2bf2b8223e85b4/ios/web/web_state/web_state_impl.mm
[modify] https://crrev.com/1000a00b446127c2c0148fe11b2bf2b8223e85b4/ios/web/web_state/web_state_impl_unittest.mm

Project Member

Comment 8 by bugdroid1@chromium.org, May 9 2017

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

commit c6a32ecf073cb1616e51c927b2bfdad46a539c31
Author: eugenebut <eugenebut@chromium.org>
Date: Tue May 09 17:39:15 2017

Added Setters to NavigationContextImpl.

This CL (5 of 7) is a part of large refactoring to create
NavigationContext when navigation was started, not when it is finished.
This refactoring will allow to pass the same instance of
NavigationContext to DidStartNavigation and DidFinishNavigation
( crbug.com/713836 ). Also this will allow to associate NavigationItem
with NavigationContext object to commit non-latest navigation
( crbug.com/712269 ).

This CL adds setters to NavigationContextImpl, which will be created only with
web state and URL and will be configured via setters before committing.

BUG= 713836 ,  712269 

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

[modify] https://crrev.com/c6a32ecf073cb1616e51c927b2bfdad46a539c31/ios/web/web_state/navigation_context_impl.h
[modify] https://crrev.com/c6a32ecf073cb1616e51c927b2bfdad46a539c31/ios/web/web_state/navigation_context_impl.mm
[modify] https://crrev.com/c6a32ecf073cb1616e51c927b2bfdad46a539c31/ios/web/web_state/navigation_context_impl_unittest.mm

Project Member

Comment 9 by bugdroid1@chromium.org, May 9 2017

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

commit 2c79b906de7b97f04ea867398b884ec9c0341839
Author: eugenebut <eugenebut@chromium.org>
Date: Tue May 09 23:49:51 2017

Allow storing null navigations in CRWWKNavigationStates.

This CL (6 of 7) is a part of large refactoring to create
NavigationContext when navigation was started, not when it is finished.
This refactoring will allow to pass the same instance of
NavigationContext to DidStartNavigation and DidFinishNavigation
( crbug.com/713836 ). Also this will allow to associate NavigationItem
with NavigationContext object to commit non-latest navigation
( crbug.com/712269 ).

WKWebView passes null WKNavigation to WKNavigationDelegate callbacks for
window opening navigation actions. This is a normal flow so
CRWWKNavigationStates should be able to store null navigations. Because
there can be at most one window opening navigation it is safe to use
unique key for storing null navigation.

BUG= 713836 , 712269 

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

[modify] https://crrev.com/2c79b906de7b97f04ea867398b884ec9c0341839/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/2c79b906de7b97f04ea867398b884ec9c0341839/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/2c79b906de7b97f04ea867398b884ec9c0341839/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Labels: -Type-Bug Type-Task
Project Member

Comment 11 by bugdroid1@chromium.org, May 10 2017

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

commit 756b09442132dec9b01cbeda19d0a8d69e8dd551
Author: eugenebut <eugenebut@chromium.org>
Date: Wed May 10 20:28:39 2017

Create NavigationContext right after the navigation was started.

This CL (7 of 7) is a part of large refactoring to create
NavigationContext when navigation was started, not when it is finished.
This refactoring will allow to pass the same instance of
NavigationContext to DidStartNavigation and DidFinishNavigation
( crbug.com/713836 ). Also this will allow to associate NavigationItem
with NavigationContext object to commit non-latest navigation
( crbug.com/712269 ).

This CL changes the time of NavigationContext creation. Subsequent CLs
will fix:
 -  crbug.com/713836  by passing NavigationContext to
      OnProvisionalNavigationStarted
 -  crbug.com/712269  by looking up for pending NavigationContext and
      extracting NavigationItem from it

BUG= 713836 ,  712269 

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

[modify] https://crrev.com/756b09442132dec9b01cbeda19d0a8d69e8dd551/ios/web/web_state/ui/crw_web_controller.h
[modify] https://crrev.com/756b09442132dec9b01cbeda19d0a8d69e8dd551/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/756b09442132dec9b01cbeda19d0a8d69e8dd551/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Status: Started (was: Assigned)
Project Member

Comment 13 by bugdroid1@chromium.org, May 15 2017

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

commit 1f66b4a55804cd494ec380c0a08446656ce512b9
Author: eugenebut <eugenebut@chromium.org>
Date: Mon May 15 21:23:25 2017

Store NavigationItem's unique ID in navigation context.

This allows to correctly commit non-latest pending navigation in
webView:didCommitNavigation:

BUG= 712269 

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

[modify] https://crrev.com/1f66b4a55804cd494ec380c0a08446656ce512b9/ios/web/web_state/navigation_context_impl.h
[modify] https://crrev.com/1f66b4a55804cd494ec380c0a08446656ce512b9/ios/web/web_state/navigation_context_impl.mm
[modify] https://crrev.com/1f66b4a55804cd494ec380c0a08446656ce512b9/ios/web/web_state/ui/crw_web_controller.mm

Status: Fixed (was: Started)
Project Member

Comment 15 by sheriffbot@chromium.org, May 16 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 16 by sheriffbot@chromium.org, Aug 22 2017

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment