New issue
Advanced search Search tips

Issue 850786 link

Starred by 2 users
Status: Started
Owner:
lazyboy@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Extension SW: increment/decrement external request ref count is race-y

Project Member Reported by lazyboy@chromium.org, Jun 8 2018 
Because extension service workers can perform extension API call early enough before the worker reaches "activated" stage (typically I see that ServiceWorkerVersion::running_status() is EmbeddedWorkerStatus::STARTING), calls to ServiceWorkerVersion::Start/FinishExternalRequest before that will result in incorrect bookkeeping of external requests. This leads to bad_message from ExtensionServiceWorkerMessageFilter on decrement.

We should, instead, queue up external requests in ServiceWorkerVersion and let them through once the worker reaches running stage.
Project Member

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

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

commit 0880f959de5f682598d150b0d2998654bb9a26d6
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Fri Jun 08 09:51:36 2018

Extension SW: Fix race in Start/FinishExternalRequest.

Extension APIs can be called early before its Service Worker is in
running state. This CL queues up external requests from extension APIs
until the worker reaches EmbeddedWorkerStatus::RUNNING state.

Currently without the CL, FinishExternalRequest request before the
worker reaches RUNNING state results in killing the extension renderer process
from ExtensionServiceWorkerMessageListener. This is also responsible for
test flakiness

Bug: 850786
Change-Id: I353d72f7ce9b81354b29db050e1b69faa3d29841
Reviewed-on: https://chromium-review.googlesource.com/1088243
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565599}
[modify] https://crrev.com/0880f959de5f682598d150b0d2998654bb9a26d6/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/0880f959de5f682598d150b0d2998654bb9a26d6/content/browser/service_worker/service_worker_version.h
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
►

Sign in to add a comment