New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 711809 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit 15 days ago
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 709179



Sign in to add a comment

Disallow Atomics in service workers

Project Member Reported by domenic@chromium.org, Apr 14 2017

Issue description

Service workers are not supposed to have any sync/blocking APIs, so in the spec being drafted at https://github.com/whatwg/html/pull/2521, we have set their [[CanBlock]] to false. Firefox implements this, but Chrome does not.

Tests available at https://github.com/w3c/web-platform-tests/pull/5569.
 
Components: Blink>ServiceWorker

Comment 2 by falken@chromium.org, Apr 17 2017

Blocking: 709179
Cc: -binji@chromium.org jochen@chromium.org nhiroki@chromium.org
Owner: binji@chromium.org
Status: Assigned (was: Untriaged)
binji: Would you be able to take this?

The WPT should soon appear in: 
external/wpt/html/webappapis/scripting/processing-model-2/integration-with-the-javascript-agent-formalism/canblock-serviceworker.html

I assume we need to do something like https://codereview.chromium.org/2660423003/ for the service worker thread.  Maybe WorkerThreadStartupData should have a bool for whether atomics are allowed, that gets plumbed to WorkerBackingThread::Initialize then to V8PerIsolateData::V8PerIsolateData. +nhiroki: do you have suggestions?

I think we should fix this before shipping Atomics.

Comment 3 by jochen@chromium.org, Apr 17 2017

won't that basically disable wasm on service workers (given that chrome still doesn't support nested workers)?

Comment 4 by falken@chromium.org, Apr 18 2017

Hey Jochen, this discussion is happening here: https://github.com/w3c/ServiceWorker/issues/1115

Does wasm require Atomics? I think we may be OK disabling wasm on service workers for now because service workers are not designed for synchronous heavy tasks. This may be another compelling reason for us to implement nested workers though.

Comment 5 by jochen@chromium.org, Apr 18 2017

Cc: bradnelson@chromium.org seththompson@chromium.org
Re c#2, falken@'s idea sounds good. WorkerV8Settings in WorkerThreadStartupData would be a good place to put such the flag.

Comment 7 by binji@chromium.org, Apr 18 2017

@falken: wasm doesn't have atomics currently, but will get them in the near future. The main issue is the "Atomics.wait" function (and its equivalent in wasm) is blocking. Without it, you can still use SharedArrayBuffers but you'll have to use lock-free algorithms to share data with another thread.

As for using wasm in general (without sharing memory), it should work anywhere the WebAssembly object is exposed (including ServiceWorker, I assume).

Comment 8 by binji@chromium.org, Apr 21 2017

Hm, taking a look at this it seems like it would be nicer if it wasn't necessary to plumb the value through to the constructor. I'm going to take a look at adding a setter on the v8::Isolate, which will make it work more like WorkerV8Settings::heap_limit_mode_ (which ends up calls v8::Isolate::IncreaseHeapLimitForDebugging).
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/3cc88456804ebe0f989f12353202b9dc768f6669

commit 3cc88456804ebe0f989f12353202b9dc768f6669
Author: binji <binji@chromium.org>
Date: Mon Apr 24 19:08:22 2017

Add setter to Isolate for allowing Atomics.wait

This makes it easier to set the value for embedders where it is
difficult to plumb through to the Isolate constructor.

BUG= chromium:711809 
R=jochen@chromium.org

Review-Url: https://codereview.chromium.org/2829223002
Cr-Commit-Position: refs/heads/master@{#44813}

[modify] https://crrev.com/3cc88456804ebe0f989f12353202b9dc768f6669/include/v8.h
[modify] https://crrev.com/3cc88456804ebe0f989f12353202b9dc768f6669/src/api.cc
[modify] https://crrev.com/3cc88456804ebe0f989f12353202b9dc768f6669/test/cctest/test-api.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 29 2017

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

commit a82bef3834cf1550ed4e4108ded9795a2f9a4d34
Author: binji <binji@chromium.org>
Date: Sat Apr 29 00:14:15 2017

Disallow Atomics.wait in ServiceWorker, allow in SharedWorker

Atomics.wait is a blocking call, and is only allowed on certain workers. See
the new web platform tests here:
https://github.com/w3c/web-platform-tests/pull/5569

BUG= chromium:711809 

Review-Url: https://codereview.chromium.org/2841003002
Cr-Commit-Position: refs/heads/master@{#468188}

[modify] https://crrev.com/a82bef3834cf1550ed4e4108ded9795a2f9a4d34/third_party/WebKit/Source/bindings/core/v8/WorkerV8Settings.h
[modify] https://crrev.com/a82bef3834cf1550ed4e4108ded9795a2f9a4d34/third_party/WebKit/Source/core/workers/DedicatedWorkerMessagingProxy.cpp
[modify] https://crrev.com/a82bef3834cf1550ed4e4108ded9795a2f9a4d34/third_party/WebKit/Source/core/workers/DedicatedWorkerMessagingProxy.h
[modify] https://crrev.com/a82bef3834cf1550ed4e4108ded9795a2f9a4d34/third_party/WebKit/Source/core/workers/DedicatedWorkerTest.cpp
[modify] https://crrev.com/a82bef3834cf1550ed4e4108ded9795a2f9a4d34/third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp
[modify] https://crrev.com/a82bef3834cf1550ed4e4108ded9795a2f9a4d34/third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h
[modify] https://crrev.com/a82bef3834cf1550ed4e4108ded9795a2f9a4d34/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/a82bef3834cf1550ed4e4108ded9795a2f9a4d34/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp

Status: Fixed (was: Assigned)
Labels: M-60

Sign in to add a comment