New issue
Advanced search Search tips

Issue 788464 link

Starred by 1 user
Status: Fixed
Owner:
danyao@chromium.org
Closed: Nov 2017
Cc:
eugene...@chromium.org
kkhorimoto@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Task

Blocked on:
issue 788231
issue 788452
Blocking:
issue 734150
issue 776438



Sign in to add a comment

replaceState triggers race condition in CRWWebController

Project Member Reported by danyao@chromium.org, Nov 24 2017 
The following example can trigger three separate race conditions in CRWWebController:

<html>
<head>
  <script>
    window.onload = function() {
      if (location.search == '') {
        window.history.replaceState({}, 'onReload', '?action=onReload');
        location.reload();
      } else if (location.search == '?action=onReload') {
        location.replace('pony.html');
      }
    };
  </script>
</head>
<body>
</body>
</html>

===

Race scenario #1:
This is the issue discovered in  crbug.com/788452 .

===

Race scenario #2:
This is the issue discovered in  crbug.com/788231 .

===

Race scenario #3:

Failure message: FATAL:crw_web_controller.mm(1998)] Check failed: (currentURL == _lastRegisteredRequestURL) || ![[_navigationStates lastAddedNavigation] isEqual:navigation] || (!_
lastRegisteredRequestURL.is_valid() && _documentURL.spec() == url::kAboutBlankURL) || (_lastRegisteredRequestURL.scheme() == url::kAboutScheme && currentURL.spec() == url::kAboutBlankURL).
currentURL = [http://127.0.0.1:50454/ios/testing/data/http_server_files/onload_replacestate_reload.html]
_lastRegisteredRequestURL = [http://127.0.0.1:50454/ios/testing/data/http_server_files/onload_replacestate_reload.html?action=onReload]

This is caused by the following interleaving of events:
1. didStartProvisionalNavigation sets _lastRegisteredRequestURL to onload_replacestate_reload.html
2. didCommitNavigation updates _documentURL to match _lastRegisteredRequestURL.
3. window.history.didReplaceState message is  and updates _lastRegisteredRequestURL to onload_replacestate_reload.html?action=onReload
4. didFinishNavigation for the original page is triggered, and fails the DCHECK because _documentURL does not match _lastRegisteredRequestURL.

Normally when replaceState happens, _documentURL is updated to match the replace state URL in one of the two KVO handlers: |webViewLoadingStateDidChange| or |URLDidChangeWithoutDocumentChange|. The race condition would not hidden if these KVO notifications arrive right after #3 and before #4.

The race condition is also hidden if window.history.didReplaceState message arrives after #4.

Comment 1 by danyao@chromium.org, Nov 24 2017

Blocking: 776438
Project Member

Comment 2 by bugdroid1@chromium.org, 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

Comment 3 by danyao@chromium.org, Nov 28 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment