936700 - WorkerPrivate should have a Preferences cache for WebIDL enabled checks

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

The following WebIDL preferences are read on worker threads and so need to have helper functions to read them out in RuntimeService:

dom.promise.enabled (Bug 915233)
dom.indexeddb.enabled (Bug 701634)
dom.mozTCPSocket.enabled (Bug 916199)

I'm sure there are more. It seems like a good time to abstract the mutex and preferences machinery.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

The new plan is to have WorkerPrivate have the cache so workers don't block on pref accesses.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Worker Preferences cache.

Bent, is this appropriate?
I'd like to land this over the Promises on workers bug to avoid the rebase.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8336423 [details] [diff] [review]
Worker Preferences cache.

Review of attachment 8336423 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good but I'd like to simplify things a bit.

First, I don't think we need to protect each worker's prefs with the mutex. We should be able to assert that they're only ever accessed on the worker thread construction time (where a worker inherits its parent's prefs, or the runtimeservice's copy in the case of top-level workers).

Second, I'd like to not re-get all the prefs that a worker cares about every time a single one of those prefs changes. The pref callback tells you which pref changed so we can just update that one.

Does that all make sense?

::: dom/promise/Promise.cpp
@@ +256,5 @@
>        return true;
>      }
>    } else {
> +    WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aCx);
> +    return workerPrivate->PromiseEnabled() || workerPrivate->IsChromeWorker();

Do you care about IsChromeWorker() or UsesSystemPrincipal() ? IsChromeWorker() really just tells you that the worker was created with the ChromeWorker constructor.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Worker Preferences cache.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8337049 [details] [diff] [review]
Worker Preferences cache.

Review of attachment 8337049 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really great! Several nits below, but please make sure to lose the strcmp:

::: dom/workers/RuntimeService.cpp
@@ +1712,2 @@
>                                                PREF_DOM_WINDOW_DUMP_ENABLED,
> +                                              nullptr)) ||

For all these cases just pass the enum value as the closure...

@@ +2385,5 @@
> +  AssertIsOnMainThread();
> +
> +  WorkerPreference key = WORKERPREF_LAST;
> +
> +  if (!strcmp(aPrefName, PREF_PROMISE_ENABLED)) {

... and then here you won't have to do the strcmp!

::: dom/workers/WorkerPrivate.h
@@ +1096,5 @@
>    bool
>    RegisterBindings(JSContext* aCx, JS::Handle<JSObject*> aGlobal);
>  
> +  bool
> +  DumpEnabled()

Nit: const, for PromiseEnabled too.

@@ +1188,5 @@
>                                uint64_t aMessagePortSerial,
>                                ErrorResult& aRv);
> +
> +  void
> +  CopyPreferences(bool aPreferences[WORKERPREF_LAST])

Nit: const, and maybe call this GetAllPreferences?

::: dom/workers/Workers.h
@@ +164,5 @@
>  
> +enum WorkerPreference
> +{
> +  WORKERPREF_DUMP = 0,
> +  WORKERPREF_PROMISE,

Nit: It would be nice to specify the prefs here:

   WORKERPREF_DUMP = 0, // browser.dom.window.dump.enabled
   WORKERPREF_PROMISE,  // dom.promise.enabled

@@ +165,5 @@
> +enum WorkerPreference
> +{
> +  WORKERPREF_DUMP = 0,
> +  WORKERPREF_PROMISE,
> +  WORKERPREF_LAST

Nit: WORKERPREF_COUNT is better!

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/185c12e7ba93

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/825162b7b8e0

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/185c12e7ba93
https://hg.mozilla.org/mozilla-central/rev/825162b7b8e0
https://hg.mozilla.org/mozilla-central/rev/14614046f0c9