618484 - Workers: Allow ChromeWorkers access to XPCOM objects

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

Attachment: Patch, v1

From an email thread, here's our plan for allowing ChromeWorkers more access to XPCOM:

1. Allow postMessage to pass XPCWrappedNatives to ChromeWorkers only, provided that the underlying object is marked with nsIClassInfo.THREADSAFE.

2. Expose a new "XPCOM" global object to ChromeWorkers only. This object will have "createInstance" and "getService" functions, taking a single contract id string argument. When these functions are called we will create/get the object on the worker thread. We will then QI to nsIClassInfo. If the QI fails or if the object is not marked with nsIClassInfo.THREADSAFE we will abort. Otherwise we will wrap the object and define all interfaces indicated by nsIClassInfo::GetInterfaces before returning it. We will not expose "Components" or "Components.interfaces" etc., so QI will not be available.

Patch attached that implements this.

This originally sprung up in response to bug 608142.

Comment 1

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

I think we want this to block.

Comment 2

[Johnny Stenback (:jst)]

Yup, this is effectively what will restore the ability to create XPCOM objects in JS off the main thread, which was possible (though unsafe) in 3.6.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 496975 [details] [diff] [review]
Patch, v1

r=me on the security manager changes.

Comment 4

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

Comment on attachment 496975 [details] [diff] [review]
Patch, v1

Johnny, can you review the dom/src/threads changes?

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 496975 [details] [diff] [review]
Patch, v1

Just as an API thing, I'd rather see a single extra argument everywhere of type JSStructuredCloneCallbacks *, rather than asking for the appropriate minimal subsets of the callbacks, in two or three arguments.

r=me for the js/src bits, with or without that change.

Comment 6

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

Attachment: Patch, v2

Ok, this patch fixes two things.

First, we no longer abort on trying to create non-threadsafe components. Johnny had the brilliant idea that we could just sync-wait for the main thread to make and test the instance for threadsafety before actually doing it on the worker. We keep a cache of threadsafe contract IDs too.

Second, I've changed the structured clone API a little more so that we can pass a void* to the callbacks. This lets us avoid leaks that could happen if we're serializing wrapped natives and AddRef'ing as we go. If something failed later, or if the buffer gets deleted without deserializing, then we'd leak all the wrapped natives. This way we can grab a list of all the wrapped natives and only AddRef if everything succeeds. Jorendorff, you ok with these changes?

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 498544 [details] [diff] [review]
Patch, v2

- In WriteStructuredClone():

+      if (NS_SUCCEEDED(classInfo->GetFlags(&flags)) &&
+          (flags & nsIClassInfo::THREADSAFE)) {
+        // Write the raw pointer into the stream, and add it to the list we're

Looks like that comment ends mid-sentence.

r=jst

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 498544 [details] [diff] [review]
Patch, v2

>+JS_WriteStructuredClone(JSContext *cx, jsval v, uint64 **bufp, size_t *nbytesp,
>+                        const JSStructuredCloneCallbacks *optionalCallbacks,
>+                        void *closure)

Well she ain't gonna win any beauty contests, that's for sure. ;)

r=me on the new js/src bits.

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/5e2045dddd67

Comment 10

[Eric Shepherd [:sheppy]]

These revisions to the ChromeWorker documentation have been made. Please take a look and let me know if I've missed anything.

https://developer.mozilla.org/en/DOM/ChromeWorker

Comment 11

[Andreas Gal :gal]

Creating a thread is a bad example for something to send to a worker imo, since there really isn't anything useful you can do with it. Can we use something else in the example?

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yeah, a really good example would be doing disk I/O of a large file, so that it doesn't block the UI.

Comment 13

[Eric Shepherd [:sheppy]]

I'll see if I can put together a more useful example. I used the tests as a basis for the one that's currently there. If anyone has some code I can use, awesome. Otherwise this'll take a bit of time. :)

Comment 14

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

I'll try to collaborate with bz today to get some file/stream example code hashed out.

Comment 15

[Eric Shepherd [:sheppy]]

bent: if you do that, you two will be my heroes. Thanks!

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This works fine with threadsafe xpcom objects (or objects marked as such to make it work).

What is still missing is a way to hand off an xpcom object to a different thread. It is a common task to prepare an object on a thread and then hand it off to where it came from. Any chance this will be possible in the 2.0 timeframe?

Comment 17

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

(In reply to comment #16)
> What is still missing is a way to hand off an xpcom object to a different
> thread. It is a common task to prepare an object on a thread and then hand it
> off to where it came from. Any chance this will be possible in the 2.0
> timeframe?

This is possible now (provided that the XPCOM object is marked threadsafe).

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #17)
> This is possible now (provided that the XPCOM object is marked threadsafe).
Thats exactly the point :-) The object I am working with is not marked threadsafe, and since its very complex it seems it cannot be marked threadsafe without some sort of modification (createInstance fails).

It would be good if I could create the non-threadsafe components in a worker so I can fill its info. Then I'd like to hand over the object and everything it accesses back to the main thread and end the worker.

Comment 19

[Boris Zbarsky [:bzbarsky]]

What object is this?  In general, if an object can't be marked threadsafe, then accessing it on multiple threads is rather unsafe, no?

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #19)
> What object is this?  In general, if an object can't be marked threadsafe, then
> accessing it on multiple threads is rather unsafe, no?

Its our implementation of calIEvent. I may be mistaking, but threadsafe means it can be accessed by multiple threads at the same time, right? I only need access from one thread at a time and after the object is initialized and sent to the main thread, I don't want to access it from the worker again.

Maybe a different scenario, but what if a developer wants to make use of core xpcom components that are not threadsafe, to initialize a different component that is marked threadsafe?

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Some objects are fine with at-most-one-thread-at-a-time; other code depends on being on the same thread, for TLS or JSThread or similar reasons.

The developer mooted in your second paragraph should initialize on the main thread and pass to the worker once it's done.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> but threadsafe means it can be accessed by multiple threads at the same time,
> right?

Yes, but perhaps not for all of the object's functionality.  There are "threadsafe" Gecko components that are not necessarily OK with being entered via the same API call on multiple threads at once...

Perhaps we need a better label for this sort of thing.

Comment 23

[Simon Kornblith]

Are there still plans for file/stream example code? I wanted to give this a try today, but I gave up when I realized that nsIScriptableInputStream doesn't appear to be thread-safe.