S13nServiceWorker Flaky. controller_service_worker_connector.cc Check failed: State::kNoContainerHost != state_ |
||||||
Issue description
Repro steps:
(1) Add a delay in SendSetControllerServiceWorker().
example: https://chromium-review.googlesource.com/897629
Then several tests will crash with the network service enabled.
For instance: external/wpt/service-workers/service-worker/controller-on-disconnect.https.html
Stack trace:
```
[1:1:0201/175238.742269:FATAL:controller_service_worker_connector.cc(77)] Check failed: State::kNoContainerHost != state_ (3 vs. 3)
#0 0x7fc2cf9cfc2d base::debug::StackTrace::StackTrace()
#1 0x7fc2cf9ce11c base::debug::StackTrace::StackTrace()
#2 0x7fc2cfa5682a logging::LogMessage::~LogMessage()
#3 0x7fc2d38a0174 content::ControllerServiceWorkerConnector::ResetControllerConnection()
#4 0x7fc2d38e982e content::ServiceWorkerProviderContext::SetController()
#5 0x7fc2d13cd811 content::mojom::ServiceWorkerContainerStubDispatch::Accept()
```
I found it will trying to apply https://docs.google.com/document/d/1dvvksKQYWEnUdOcjkyreaTOxJD9L4Gybszb5mxrSzHk/
with NavigationMojoResponse. I have the same DCHECK triggered and that's the last thing I need to fix before publishing my CL.
I will work on it tomorrow, but if you already understand the issue today, let me know ;-)
,
Feb 7 2018
,
Feb 12 2018
I would like to merge in M65 two patches together: * https://crbug.com/705744 (https://crrev.com/535339) * https://crbug.com/808071 (https://crrev.com/534706) <- Here in comment 1. They have been on M66 canary for a few days in 66.0.3344.0 (Feb 8) and 66.0.3342.0 (Feb 6). They are fixing a potential race condition issue between Service workers and NavigationMojoResponse. NavigationMojoResponse is disabled by default and finch is used. There is a 50%/50% experiment on M65 Canary. I would like to merge these two patches in order to move the experiment from Canary to Dev and get some performance statistics. I do not plan to continue the experiment further than beta in M65. Risk is low because these two patches are modifying a code path that is not used by default (NavigationMojoResponse and/or S13nServiceWorker). Finch can be used to disable the experiment on Canary/Dev if needed. FYI: NavigationMojoResponse launch bug: https://crbug.com/805851.
,
Feb 12 2018
This bug requires manual review: M65 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e65a98e0c0fd06e32226c03a4057d6dc8c3c2cd4 commit e65a98e0c0fd06e32226c03a4057d6dc8c3c2cd4 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Mon Feb 12 18:41:48 2018 ControllerServiceWorkerConnector: Remove DCHECK. All of this happens when S13nServiceWorker is enabled. The removed DCHECK was here to enforce that the ControllerServiceWorker is not updated after container host connection has been closed. A race condition can happen and triggers this DCHECK. After CommitNavigation(), if the browser process is slow enough, a javascript call can delete the iframe before the OnProviderCreated/SetController round trip between the browser and the renderer has been made. Deleting the iframe cause OnContainerHostConnectionClose() to be called. It happens before receiving the SetController call. ┌───────┐ ┌────────┐ │Browser│ │Renderer│ └───┬───┘ └───┬────┘ │CommitNavigation() │ │──────────────────>│ (ResetController()) │ │ │OnProviderCreated()│ │<──────────────────│ │─┐ │─┐ │ │ │ │ Remove the iframe. │ │ │ │ (OnContainerHostConnectionClose()) │ │ Sleep... │<┘ │ │ │ │<┘ │ │ │ │ SetController() │ │──────────────────>│ (ResetController()) ┌───┴───┐ ┌───┴────┐ │Browser│ │Renderer│ └───────┘ └────────┘ Note: The CL... https://chromium-review.googlesource.com/897629 ...shows adding a delay in the browser process causes several service worker tests to fail. Note: Doing SetController() on response to OnProviderCreated() is no really necessary since thttps://goo.gl/UztxVi was implemented by https://chromium-review.googlesource.com/742961. This was kept to update UseCounter. Bug: 808071 Change-Id: I46c4fd8e4db7eb4ed5e304bb1989d36d552567ad Reviewed-on: https://chromium-review.googlesource.com/899143 Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#534706}(cherry picked from commit 4b597a8953893e33a4c76b987093655a71c87a2a) Reviewed-on: https://chromium-review.googlesource.com/913953 Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#427} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/e65a98e0c0fd06e32226c03a4057d6dc8c3c2cd4/content/renderer/service_worker/controller_service_worker_connector.cc
,
Feb 12 2018
Was this cl merged without merge approval?
,
Feb 12 2018
cmasso@, CL listed at #5 was approved for merge at - https://bugs.chromium.org/p/chromium/issues/detail?id=705744#c59.
,
Feb 13 2018
This is already merged to M65 at comment #5. If nothing is pending for M65, pls remove "Merge-Review-65" label. Thank you.
,
Feb 14 2018
Everything has been merged. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by bugdroid1@chromium.org
, Feb 6 2018