New issue
Advanced search Search tips

Issue 850792 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Extension SW: Race in worker registration and extension event dispatch

Project Member Reported by lazyboy@chromium.org, Jun 8 2018

Issue description

Currently, for extension event dispatch: ServiceWorkerTaskQueue starts the extension's worker and sends IPC to dispatch the event. The //content API used (ServiceWorkerContext::StartActiveWorkerForPattern) only looks for an "active" worker. If the event was dispatched early before the worker reached "activated" stage, we'll fail DCHECK in ServiceWorkerTaskQueue::DidStartActiveWorkerFail. In this case, the SW registration will only have installing_version(), but no waiting/active version.

My understanding is that we should also look at SWRegistration::installing_version in this case.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 11 2018

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

commit 0c08a3486bad77be27a74b3e19cc16cb67d26767
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Mon Jun 11 05:13:05 2018

Fix race/incorrectness in looking up worker while dispatching Extension events.

Extension event dispatch code currently assumes that there will always
be an active service worker that it can "Start" and then dispatch
events. This isn't true if the extension's service worker is still
installing. Consider registration's installing_version() as well to
avoid the race.

This CL renames ServiceWorkerContext's StartActiveWorkerForPattern to
StartWorkerForPattern and changes its behavior to also consider
ServiceWorkerRegistration::installing_version(). If there isn't any
waiting or active worker in the registration, the change would
attempt to start/run the installing worker.

This change should improve test failures that see
ServiceWorkerTaskQueue::DidStartActiveWorkerFail in this scenario.

Subsequent CLs will also exercise early event dispatch to make this
CL a necessary fix.

Bug: 850792
Change-Id: If73c36f0f13a3a6a19ae510d4d06b1f602185175
Reviewed-on: https://chromium-review.googlesource.com/1088306
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565929}
[modify] https://crrev.com/0c08a3486bad77be27a74b3e19cc16cb67d26767/content/browser/service_worker/service_worker_context_wrapper.cc
[modify] https://crrev.com/0c08a3486bad77be27a74b3e19cc16cb67d26767/content/browser/service_worker/service_worker_context_wrapper.h
[modify] https://crrev.com/0c08a3486bad77be27a74b3e19cc16cb67d26767/content/public/browser/service_worker_context.h
[modify] https://crrev.com/0c08a3486bad77be27a74b3e19cc16cb67d26767/content/public/test/fake_service_worker_context.cc
[modify] https://crrev.com/0c08a3486bad77be27a74b3e19cc16cb67d26767/content/public/test/fake_service_worker_context.h
[modify] https://crrev.com/0c08a3486bad77be27a74b3e19cc16cb67d26767/extensions/browser/service_worker_task_queue.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 19 2018

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

commit ccb444028112108d32f945c03f1cb16ac0b27b7c
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Tue Jun 19 02:11:12 2018

Beginnings of SW based background page.

Add manifest key ("service_worker_script") to specify a script
for SW based background page.
Registers the provided script as a worker.
Add extension registry observers to observe the begin/finish of the
SW registration.
Change ServiceWorkerTaskQueue to support queueing tasks. Queueing is
done if the SW registration is pending.

Add a browser_test to load such extension, that doesn't use any
background page.

Doc: https://goo.gl/j4ca5d
Bug: 853378, 850792, 850786
Change-Id: I11c0bdbbbdb6e2ca1e7423330ebd455bdd45fb11
Reviewed-on: https://chromium-review.googlesource.com/1088162
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568300}
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/chrome/browser/extensions/service_worker_apitest.cc
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/chrome/common/extensions/manifest_tests/extension_manifests_background_unittest.cc
[add] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/chrome/test/data/extensions/api_test/service_worker/worker_based_background/basic/manifest.json
[add] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/chrome/test/data/extensions/api_test/service_worker/worker_based_background/basic/service_worker_background.js
[add] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/chrome/test/data/extensions/manifest_tests/service_worker_based_background.json
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/extensions/browser/extension_registrar.cc
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/extensions/browser/process_manager.cc
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/extensions/browser/service_worker_task_queue.cc
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/extensions/browser/service_worker_task_queue.h
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/extensions/browser/service_worker_task_queue_factory.cc
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/extensions/common/api/_manifest_features.json
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/extensions/common/manifest_constants.cc
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/extensions/common/manifest_constants.h
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/extensions/common/manifest_handlers/background_info.cc
[modify] https://crrev.com/ccb444028112108d32f945c03f1cb16ac0b27b7c/extensions/common/manifest_handlers/background_info.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 20 2018

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

commit 771aa8a29cbcdd820bd797532780c788cee481ad
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Wed Jun 20 23:40:53 2018

Extension SW: Exercise early extension event dispatch right after registration.

This CL adds a test that sends an extension event early in the
extension lifetime, specifically after EventRouter sees a listener
registration, shortly after Service Worker is being registered.
This demonstrates that event dispatch works correctly before SW
registration contains an active worker. The registration would contain
installing_worker() in scenario.

This is a regression test for change
0c08a3486bad77be27a74b3e19cc16cb67d26767

Bug: 850792
Change-Id: Ifab379cf60a105c806280fc5c27972f919632eac
Reviewed-on: https://chromium-review.googlesource.com/1091705
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569087}
[modify] https://crrev.com/771aa8a29cbcdd820bd797532780c788cee481ad/chrome/browser/extensions/service_worker_apitest.cc
[add] https://crrev.com/771aa8a29cbcdd820bd797532780c788cee481ad/chrome/test/data/extensions/api_test/service_worker/worker_based_background/early_event_dispatch/manifest.json
[add] https://crrev.com/771aa8a29cbcdd820bd797532780c788cee481ad/chrome/test/data/extensions/api_test/service_worker/worker_based_background/early_event_dispatch/service_worker_background.js

Sign in to add a comment