Make ChromeWorkers available to JSMs

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: philikon, Assigned: jorendorff)

Tracking

({dev-doc-complete})

unspecified
dev-doc-complete
Points:
---

Firefox Tracking Flags

(blocking2.0 beta8+, fennec2.0b3+)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 attachment, 3 obsolete attachments)

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

Comment 1

8 years ago
blocks a blocker -> requesting blocking
blocking2.0: --- → ?
tracking-fennec: --- → ?

Updated

8 years ago
blocking2.0: ? → beta8+
(Assignee)

Updated

8 years ago
Assignee: general → jorendorff
blocking2.0: beta8+ → ?

Updated

8 years ago
blocking2.0: ? → beta8+
tracking-fennec: ? → 2.0b3+
(Assignee)

Comment 2

8 years ago
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;
    }
(Assignee)

Comment 3

8 years ago
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?
(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.
(Assignee)

Comment 5

8 years ago
Created attachment 489224 [details] [diff] [review]
WIP 1

Here's my sketch of the first approach.
(Assignee)

Comment 6

8 years ago
Created attachment 489416 [details] [diff] [review]
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.
Attachment #489224 - Attachment is obsolete: true
Attachment #489416 - Flags: review?(bent.mozilla)
(Assignee)

Comment 7

8 years ago
(Emphasis on the word "sounds".)
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.
(Assignee)

Comment 9

8 years ago
Created attachment 490925 [details] [diff] [review]
v2
Attachment #489416 - Attachment is obsolete: true
Attachment #490925 - Flags: review?(bent.mozilla)
Attachment #489416 - Flags: review?(bent.mozilla)
(Assignee)

Comment 10

8 years ago
Created attachment 490930 [details] [diff] [review]
v3
Attachment #490925 - Attachment is obsolete: true
Attachment #490930 - Flags: review?(bent.mozilla)
Attachment #490925 - Flags: review?(bent.mozilla)
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!
Attachment #490930 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 12

8 years ago
http://hg.mozilla.org/tracemonkey/rev/f3cfb74fd413
Whiteboard: [fixed-in-tracemonkey]
blocking2.0: beta8+ → betaN+
tracking-fennec: 2.0b3+ → 2.0b4+
blocking2.0: betaN+ → beta8+
tracking-fennec: 2.0b4+ → 2.0b3+

Comment 13

8 years ago
http://hg.mozilla.org/mozilla-central/rev/f3cfb74fd413
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

8 years ago
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).
Keywords: dev-doc-needed
Nah, this is fine for now.
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.
You need to log in before you can comment on or make changes to this bug.