Flakey LoadIfNecessaryTest.DisableAndReenableWebUsage with WKBasedNavigationManager |
|||||
Issue descriptionThis test sometimes fails the DCHECK in [CRWWebController URLDidChangeWithoutDocumentChange] when used with WKBasedNavigationManager: FATAL:crw_web_controller.mm(5074)] Check failed: navigationContext->IsSameDocument(). 0 ios_web_unittests 0x000000010316634d base::debug::StackTrace::StackTrace(unsigned long) + 157 1 ios_web_unittests 0x000000010316638d base::debug::StackTrace::StackTrace(unsigned long) + 29 2 ios_web_unittests 0x00000001031647fc base::debug::StackTrace::StackTrace() + 28 3 ios_web_unittests 0x00000001031c21bf logging::LogMessage::~LogMessage() + 479 4 ios_web_unittests 0x00000001031bfb65 logging::LogMessage::~LogMessage() + 21 5 ios_web_unittests 0x0000000102e4e01d -[CRWWebController URLDidChangeWithoutDocumentChange:] + 4173 6 ios_web_unittests 0x0000000102e4cb02 __39-[CRWWebController webViewURLDidChange]_block_invoke + 914 It seems that WKBasedNavigationManager's restore_session.html triggers the following race condition: 1. While the page is still loading, the replaceState call triggers |-webViewURLDidChange| to execute the window.location.href JavaScript 2. Before the JavaScript completion block is called, the page calls location.reload on the replaced state. This triggers |-webView:didStartProvisionalNavigation| because the document is about to be updated. 3. When JavaScript completion block fires, it attempts to complete the same-document URL change processing with the pending navigation, which is actually for a different document. This triggers the DCHECK. The key for triggering this case seems to be calling replaceState and location.reload immediately after within the onload() handler.
,
Nov 24 2017
,
Nov 24 2017
,
Nov 24 2017
,
Nov 27 2017
Note for posterity: just using [_navigationStates lastAddedNavigation] to check if a new navigation has started between the original executeJavaScript call and when the JS completion block fires is not sufficient. We also need to check [_navigationStates stateForNavigation:navigation] is STARTED or later. This edge case is exposed by NavigationAndLoadCallbacksTest.UserInitiatedHashChangeNavigation. What is happening is this: if user enters a URL that differs from the previous one only in the fragment (hash), [WKWebView -loadRequest] will happily return a new WKNavigation*, but will never trigger any WKNavigationDelegate callbacks. The state of this WKNavigation* will never change from REQUESTED to STARTED. In this case, the JS completion block still needs to run.
,
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