608171 - Make ChromeWorkers available to JSMs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Philipp von Weitershausen [:philikon]]

We need ChromeWorkers available from JSMs in order to implement WeaveCrypto as a ChromeWorker.

Comment 1

[Dave Garrett]

blocks a blocker -> requesting blocking

Comment 2

[Jason Orendorff [:jorendorff]]

bent wants the API to look like this:

    var worker = Cc["@mozilla.org/threads/chromeworker;1"]
        .createInstance(Ci.nsIChromeWorker);
    worker.start(scriptURL);

That sounds fine to me too.  But in order to avoid immediately jumping in and rewriting parts of nsDOMWorker itself, I decided to start by implementing it this way:

    var factory = Cc["@mozilla.org/threads/workerfactory;1"]
        .createInstance(Ci.nsIWorkerFactory);
    var worker = factory.newChromeWorker(scriptURL);

This almost works, but there is a fatal flaw, namely that the nsDOMWorker class has a scriptable helper which refuses to let the nsIChromeWorker interface be exposed to script, even chrome. The relevant stack is:

    XPCWrappedNative::InitTearOff
    XPCWrappedNative::FindTearOff
    XPCWrappedNative::GetNewOrUsed
    XPCConvert::NativeInterface2JSObject
    XPCConvert::NativeData2JS

The check happens right after newChromeWorker returns. XPConnect is trying to wrap the return value.  The code in InitTearOff:

    // If the scriptable helper forbids us from reflecting additional
    // interfaces, then don't even try the QI, just fail.
    if(mScriptableInfo &&
       mScriptableInfo->GetFlags().ClassInfoInterfacesOnly() &&
       !mSet->HasInterface(aInterface) &&
       !mSet->HasInterfaceWithAncestor(aInterface))
    {
        return NS_ERROR_NO_INTERFACE;
    }

Comment 3

[Jason Orendorff [:jorendorff]]

Same deal if I try to implement it the other way, actually. I think I need chrome workers to return different classinfo. Is there any reason not to do that?

Comment 4

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

(In reply to comment #3)
> Same deal if I try to implement it the other way, actually. I think I need
> chrome workers to return different classinfo. Is there any reason not to do
> that?

Hm, yeah, maybe that's best.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

Here's my sketch of the first approach.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: v1 - nsIWorkerFactory interface and component

This one actually works.

Adding a start() method that is only exposed to chrome turns out to involve a surprising amount of extra pain. It would involve splitting ordinary Workers and ChromeWorkers into two separate C++ classes (related by inheritance) with different ClassInfo, changing the implementation to support exposing ChromeWorkers to script before they're initialized, and making the scriptable helpers work (bent says there's a twist to that, but I'm not sure what it is). None of that sounds hard, but I'd rather just land this.

Comment 7

[Jason Orendorff [:jorendorff]]

(Emphasis on the word "sounds".)

Comment 8

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

Comment on attachment 489416 [details] [diff] [review]
v1 - nsIWorkerFactory interface and component

>+#define NS_WORKERFACTORY_CID                         \
>+ {0x1295efb5, 0x8644, 0x42b2,                        \
>+  {0x8b, 0x8e, 0x80, 0xee, 0xf5, 0x6e, 0x42, 0x84} }

I'm ok with the contractID being in the IDL but I think you should move the CID to nsDOMWorker.h

>+  nsDOMWorker *worker = static_cast<nsDOMWorker *>(iworker.get());
>+  return worker->IsPrivileged() ? NS_SUCCESS_CHROME_ACCESS_ONLY : NS_OK;

Hm. Not sure I'm ok with this cast. Let me talk with jst about some other way to do this.

>+nsWorkerFactory::NewChromeWorker(nsIWorker **rval)

Nit: * on the left, param name should be "aRval".

And can you add an NS_IsMainThread assertion here?

>+  nsAXPCNativeCallContext *cc;

Nit: * on the left, here and elsewhere.

>+  nsDOMWorker *chromeWorker =
>+    static_cast<nsDOMWorker*>(static_cast<nsIWorker*>(newObject.get()));
>+  rv = chromeWorker->InitializeInternal(global, cx, globalobj, argc, argv);

Hm. Rather than this, just QI |newObject| to nsIJSNativeInitializer and call Initialize() like nsDOMClassInfo does.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: v2

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: v3

Comment 11

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

Comment on attachment 490930 [details] [diff] [review]
v3

>+  nsCOMPtr<nsIWorker> iworker(do_QueryInterface(aObject));
>+  if (!iworker) {
>+    return NS_OK;
>+  }
>+
>+  nsDOMWorker *worker = static_cast<nsDOMWorker *>(iworker.get());
>+  return worker->IsPrivileged() ? NS_SUCCESS_CHROME_ACCESS_ONLY : NS_OK;

Maybe simplify:

  if (iworker && static_cast<nsDOMWorker *>(iworker.get())->IsPrivileged()) {
    return NS_SUCCESS_CHROME_ACCESS_ONLY;
  }
  return NS_OK;

>+  // Create, initialize, and return the worker.
>+  nsRefPtr<nsDOMWorker> chromeWorker;
>+  rv = nsDOMWorker::NewChromeDOMWorker(getter_AddRefs(chromeWorker));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = chromeWorker->InitializeInternal(global, cx, globalobj, argc, argv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  chromeWorker.forget(_retval);
>+  return NS_OK;

Nit: Add a newline after each NS_ENSURE_SUCCESS

r=me with that!

Comment 12

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/f3cfb74fd413

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f3cfb74fd413

Comment 14

[Jason Orendorff [:jorendorff]]

I think this is dev-doc-needed, unless bent wants to make further changes to the API (or wrap it in some JS for convenience).

Comment 15

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

Nah, this is fine for now.

Comment 16

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/JavaScript_code_modules/Using_workers_in_JavaScript_code_modules
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIWorkerFactory

And linked to from Fx4 for developers.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Is this new service meant to work in a JSM?  Because it seems to assume the cx you're running on has an nsIScriptGlobalObject attached, which isn't true while executing a JSM's script....  See bug 615153.