949992 - Create ShumwayWorker, a specialized kind of worker thread only usable by Shumway

Status: RESOLVED WONTFIX

(Core :: DOM: Workers, defect)

Description

[Till Schneidereit [:till]]

Because we have to implement a legacy system, Shumway needs some capabilities that aren't fit for inclusion into the web platform. Most importantly, we need a sync messaging channel to and from workers, as tracked in bug 930908. There are some additional features we want to hang off of the thus-needed specialized worker kind, though, and these are tracked in the other blocked bugs.

A problem that has been raised is that, once we have this worker kind, people might abuse it for other things. An idea for preventing that might be to hard-code the source location for the ShumwayWorker. Where workers normally take the location as an argument, we'd just always load from, say, "resource://shumway/shumway.js". (Except that, for ease of development reasons, we might want to make this overridable with an env var.)

Comment 1

[Till Schneidereit [:till]]

Attachment: Create ShumwayWorker

This at least works insofar as it installs a ShumwayWorker ctor on chrome globals, and creates a new worker thread with the hard-coded source path "resource://shumway/shumway.js" when that ctor is run.

My thinking is that I should merge the mIsChromeWorker flag and the WorkerType enum and create the following five enum values for the latter:
ContentWorkerTypeDedicated
ContentWorkerTypeShared
ChromeWorkerTypeDedicated
ChromeWorkerTypeShared
ShumwayWorkerType

Nothing about how the various states are queried would need to change, really, as these values are only used in Is*Worker methods on WorkerPrivateParent.

Ben, does this approach look good to you?

Comment 2

[Till Schneidereit [:till]]

Attachment: Merge mIsChromeWorker and mWorkerType fields

After doing the aforementioned merge, I'm not convinced anymore that it's a good idea. Maybe just introducing a new WorkerTypeShumway and leaving the isChromeWorker flag alone makes more sense. Putting this up mostly for posterity and as a backup.

Comment 3

[Masatoshi Kimura [:emk]]

Is Shumway going to be Firefox-specific?

Comment 4

[Till Schneidereit [:till]]

(In reply to Masatoshi Kimura [:emk] from comment #3)
> Is Shumway going to be Firefox-specific?

Shumway itself will continue working in other browsers, too. The feature worked on here is required for some fundamental optimizations, sadly, but not for Shumway working at all.

If we want to replace the Flash plugin with Shumway (which we do), we have to be competitive in terms of performance, responsiveness and memory usage when running multiple - potentially many - SWF files at the same time. For that, we need to share a runtime instead of creating a new one for each SWF. That means running in a worker. However, the semantics of Flash require the ability to synchronously communicate with the embedding web page, in both directions. This means blocking the main thread on the worker thread, and vice versa; a feature we don't want to have on the web. However, for web pages embedding Shumway, none of this is strictly necessary: they can control the number of instances, and can even still share a runtime among these instances, but do so on the main thread.

Comment 5

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

Comment on attachment 8347714 [details] [diff] [review]
Merge mIsChromeWorker and mWorkerType fields

Yeah, I'd prefer to keep the worker type (which determines the type of scope object) and the chrome flag (which determines whether the scope gets extra chrome features) separate.

Comment 6

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

Comment on attachment 8347712 [details] [diff] [review]
Create ShumwayWorker

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

This looks right to me!

::: dom/workers/WorkerPrivate.cpp
@@ +3436,5 @@
> +already_AddRefed<ShumwayWorkerPrivate>
> +ShumwayWorkerPrivate::Constructor(const GlobalObject& aGlobal,
> +                                 ErrorResult& aRv)
> +{
> +  return WorkerPrivate::Constructor(aGlobal, NS_LITERAL_STRING("resource://shumway/shumway.js"),

This should probably be a pref or something, right?

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 8347714 [details] [diff] [review]
Merge mIsChromeWorker and mWorkerType fields

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #6)
> > +  return WorkerPrivate::Constructor(aGlobal, NS_LITERAL_STRING("resource://shumway/shumway.js"),
> 
> This should probably be a pref or something, right?

Probably, yes. In that case, it should probably be read only once per browser session, though: we want to make it as hard as possible for addon authors to abuse the ShumwayWorker ...

Comment 8

[Till Schneidereit [:till]]

Attachment: Part 2: Install Shumway-only functions in the worker's global

To enable customizing the ShumwayWorker's global, I introduced WorkerPrivate::IsShumwayWorker(), and use that in WorkerPrivate::RegisterBindings to call the, also new, DefineShumwayWorkerFunctions.

Instead of adding a new bool aIsShumwayWorker to all the functions related to the creation of WorkerPrivate, I changed `bool aIsChromeWorker` to `WorkerPrivileges aWorkerPrivileges`, with the enum containing values for content, chrome, and shumway privileges.

Bent, do you think this is the right approach?

Comment 9

[Till Schneidereit [:till]]

Attachment: Part 2: Install Shumway-only functions in the worker's global. v2

Ugh, I just realized that I never added the new files to hg

Comment 10

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

Comment on attachment 8355815 [details] [diff] [review]
Part 2: Install Shumway-only functions in the worker's global. v2

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

This looks like it's on the right track!

::: dom/workers/RegisterBindings.cpp
@@ +52,5 @@
>        return false;
>      }
>    }
>  
> +  if (IsShumwayWorker()) {

This should be 'else if' since they're mutually exclusive

::: dom/workers/WorkerPrivate.h
@@ +178,5 @@
>      WorkerTypeDedicated,
>      WorkerTypeShared
>    };
>  
> +  enum WorkerPrivileges

I'm thinking we should make this more specific, something like 'WorkerScopeType' maybe. Privileges are determined by the principal anyway.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8355815 [details] [diff] [review]
Part 2: Install Shumway-only functions in the worker's global. v2

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

The way this should be done is to create a new WebIDL interface, ShumwayWorkerGlobalScope, and a new WorkerScope subclass that inherits from it.  You should declare your methods on the WebIDL.  The reason ChromeWorker doesn't do this is that it only needs to call JS_InitCTypesClass.  But we're trying to avoid manual JSAPI wherever possible.

Comment 12

[Till Schneidereit [:till]]

Attachment: Part 1: Create ShumwayWorker. v2

Added a pref for the worker script URI, as requested in comment 6.

Comment 13

[Till Schneidereit [:till]]

Attachment: Part 2: Install Shumway-only functions in the worker's global. v2

This changes ShumwayWorkerGlobalScope to a webIDL-based implementation. The implementation of `getWeakMapKeys` that I added as a test (but that we also need in Shumway) works as expected, so at least functionally, this seems to be ok.

Bent, I flagged you for comment because khuey is out, but if you want to punt on it, that's fine.

Comment 14

[Till Schneidereit [:till]]

Attachment: Part 2: Install Shumway-only functions in the worker's global. v3

Once more, with webidl file included

Comment 15

[Till Schneidereit [:till]]

Comment on attachment 8416873 [details] [diff] [review]
Part 2: Install Shumway-only functions in the worker's global. v3

Requesting review from khuey now that he's back from PTO.

Kyle, it'd be great if you could review this soon-ish, so we can land it preffed-off in Nightly (only).

Comment 16

[Till Schneidereit [:till]]

Comment on attachment 8416558 [details] [diff] [review]
Part 1: Create ShumwayWorker. v2

Oh, this part needs a proper review, too,

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8416558 [details] [diff] [review]
Part 1: Create ShumwayWorker. v2

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

::: dom/bindings/Bindings.conf
@@ +237,5 @@
> +'ShumwayWorker': {
> +    'headerFile': 'mozilla/dom/WorkerPrivate.h',
> +    'nativeType': 'mozilla::dom::workers::ShumwayWorkerPrivate',
> +    'customFinalize': True,
> +    'customWrapperManagement': True,

I assume you cargo-culted these two customFoo things, because we got rid of these a while ago, so they no longer do anything.

::: dom/workers/WorkerPrivate.cpp
@@ +3633,5 @@
> +already_AddRefed<ShumwayWorkerPrivate>
> +ShumwayWorkerPrivate::Constructor(const GlobalObject& aGlobal,
> +                                 ErrorResult& aRv)
> +{
> +  nsAutoString scriptURL(NS_LITERAL_STRING("resource://shumway/shumway.js"));

NS_NAMED_LITERAL_STRING?

@@ +3635,5 @@
> +                                 ErrorResult& aRv)
> +{
> +  nsAutoString scriptURL(NS_LITERAL_STRING("resource://shumway/shumway.js"));
> +  nsAdoptingString prefURL = Preferences::GetString("shumway.worker_uri");
> +  if (prefURL) {

I think you need to check IsEmpty here.  I'm pretty terrified that this automatically converts to a bool ...

@@ +3650,5 @@
> +{
> +  // Chrome is always allowed to use workers, and content is never allowed to
> +  // use ShumwayWorker, so all we have to check is the caller.
> +  return nsContentUtils::ThreadsafeIsCallerChrome();
> +}

Shouldn't this be main thread only?

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8416873 [details] [diff] [review]
Part 2: Install Shumway-only functions in the worker's global. v3

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

::: dom/workers/RuntimeService.cpp
@@ +2090,5 @@
>    JSContext* cx = aGlobal.GetContext();
>  
>    WorkerPrivate::LoadInfo loadInfo;
>    nsresult rv = WorkerPrivate::GetLoadInfo(cx, window, nullptr, aScriptURL,
> +                                           WorkerPrivate::WorkerScopeTypeContent, &loadInfo);

break this line at 80?

@@ +2120,5 @@
>  
>    if (!workerPrivate) {
>      ErrorResult rv;
>      workerPrivate =
> +      WorkerPrivate::Constructor(aGlobal, aScriptURL, WorkerPrivate::WorkerScopeTypeContent,

this too.

::: dom/workers/WorkerPrivate.h
@@ +204,5 @@
> +  {
> +    WorkerScopeTypeContent,
> +    WorkerScopeTypeChrome,
> +    WorkerScopeTypeShumway
> +  };

We should add assertions that these two enums are not combined in ways that don't work.

::: dom/workers/WorkerScope.cpp
@@ +369,5 @@
> +  JS::CompartmentOptions options;
> +  mWorkerPrivate->CopyJSCompartmentOptions(options);
> +
> +  // We're wrapping the global, so the scope is undefined.
> +  JS::Rooted<JSObject*> scope(aCx);

This isn't used ...

@@ +373,5 @@
> +  JS::Rooted<JSObject*> scope(aCx);
> +
> +  return ShumwayWorkerGlobalScopeBinding_workers::Wrap(aCx, this, this,
> +                                                         options,
> +                                                         GetWorkerPrincipal());

indentation

@@ +398,5 @@
> +    aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
> +    return nullptr;
> +  }
> +
> +  fprintf(stderr, "GetWeakMapKeys called\n");

Remove this.

::: dom/workers/WorkerScope.h
@@ +176,5 @@
> +
> +  void
> +  PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
> +              const Optional<Sequence<JS::Value>>& aTransferable,
> +              ErrorResult& aRv);

You didn't declare PostMessage in the IDL ... so this isn't actually hooked up, afaict.

Comment 19

[Till Schneidereit [:till]]

We decided that an e10s process is much better suited to our needs than a special snowflake of a worker just for us.