LoadIfNecessaryTest.RestoredFromHistory fails with WKBasedNavigationManager |
||
Issue descriptionThis test creates a WebState with restored session history, and then calls [CRWWebController loadCurrentURLIfNecessary] to restore the session history. It fails at this DCHECK: crw_web_controller.mm(4530)] Check failed: _documentURL == _lastRegisteredRequestURL || !isLastNavigation || (!_lastRegisteredRequestURL.is_valid() && _documentURL.spec() == url::kAboutBlankURL). WKBasedNavigationManager uses ios/web/navigation/restore_session.html to replay history into the WKWebView. It triggers a latent race condition in CRWWebController. This is the normal sequence of events: 1. The |webView:didCommitNavigation| callback calls |injectHTML5HistoryScriptWithHashChange| 2. |injectHTML5HistoryScriptWithHashChange| executes JavaScript in WKWebView 3. The completion handler of the |executeJavaScript| updates _lastRegisteredRequestURL 4. |webView:didStartProvisionalNavigation| callback for new navigation gets called, which updates _lastRegisteredRequestURL to new URL 5. |webView:didCommitNavigation| callback for new navigation gets called. It updates _documentURL to the new URL and passes the DCHECK that _documentURL is the same as _lastRegisteredRequestURL. Somehow when loading restore_session.html, #3 happens after #4 and clobbers _lastRegisteredRequestURL. This causes the DCHECK to fail in #5. Since JavaScript execution is asynchronous, it seems that we shouldn't assume that the completion handler call (#3) always happens before #4.
,
Nov 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/414a100e00163ba2ed3b66c0094ca52709ec3b24 commit 414a100e00163ba2ed3b66c0094ca52709ec3b24 Author: Danyao Wang <danyao@chromium.org> Date: Mon Nov 27 23:25:28 2017 [Nav Experiment] Fixed latent race conditions in CRWWebController. Added a new inttest, onload_replacestate_reload.html that triggers 3 separate latent race conditions in CRWWebController: 1. crbug.com/788452 : JS completion block from |webViewURLDidChange| fires too late and a different-document provisional navigation already started. 2. crbug.com/788231 : JS completion block from |updateHTML5HistoryState| fires too late and clobbers _lastRegisteredRequestURL. 3. crbug.com/788464 : window.history.didReplaceState message arrives too early and updates _lastRegisteredRequestURL that conflicts later with |webView:didFinishNavigation|. The root cause is that the web process is separate from the UI process, so we cannot assume specific ordering between KVO notifications, JS completion blocks and webkit.postMessage messages. The fixes are not perfect. I tried to detect the race condition and abort the second half of the async processing. A better longer term fix after WKBasedNavigationManager launches is to simplify history state handling and reduces async use cases in CRWWebController. Bug: 788464 , 788231 , 788452 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I69d788fc040371247cfac29e02e80dadb7821148 Reviewed-on: https://chromium-review.googlesource.com/788185 Commit-Queue: Danyao Wang <danyao@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#519445} [modify] https://crrev.com/414a100e00163ba2ed3b66c0094ca52709ec3b24/ios/testing/BUILD.gn [add] https://crrev.com/414a100e00163ba2ed3b66c0094ca52709ec3b24/ios/testing/data/http_server_files/onload_replacestate_reload.html [add] https://crrev.com/414a100e00163ba2ed3b66c0094ca52709ec3b24/ios/testing/data/http_server_files/onload_replacestate_reload.js [modify] https://crrev.com/414a100e00163ba2ed3b66c0094ca52709ec3b24/ios/web/navigation/history_state_operations_inttest.mm [modify] https://crrev.com/414a100e00163ba2ed3b66c0094ca52709ec3b24/ios/web/web_state/ui/crw_web_controller.mm
,
Nov 28 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by danyao@chromium.org
, Nov 24 2017