610381 - nsIThread#dispatch fails silently from JS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kris Maglione [:kmag]]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101107 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101107 Firefox/4.0b8pre                     

Dispatching a runnable to a nsIThread seems to fail silently no matter how it's invoked. No exception is thrown, and nothing appears in the error console, but execution continues as if the function had succeeded.

let t = Services.tm.newThread(0);
let a = {};
t.dispatch({ run: function () { a.b = 12; } }, t.DISPATCH_SYNC);
dump("a: " + Object.keys(a) + "\n"); // Prints: a: 


Reproducible: Always

Comment 2

[Boris Zbarsky [:bzbarsky]]

Andreas, I thought this was supposed to throw... is it not doing that?

Comment 3

[Xavier]

I have the exact same problem with Firefox 4.0b7

Comment 4

[Andreas Gal :gal]

Yes, this is supposed to throw. You can't dispatch a closure to a different thread any more. For the record, the test case above is not reliable, btw (DISPATCH_SYNC comes back as soon the event was dispatched on the other thread, that doesn't mean the closure has finished writing to a, even if we would support cross thread closure sending, which we do not any more). But thats irrelevant. We should never get to the dump clearly.

Comment 5

[Benjamin Smedberg]

dispatch_sync waits for a reply, that's what's makes it different from dispatch_async. Do we know *why* this doesn't throw?

FWIW, I wouldn't expect DISPATCH_ASYNC to throw, because the error doesn't occur until you try to use the runnable on the other thread.

Comment 6

[Kris Maglione [:kmag]]

For what it's worth, my impression is that it throws in the new background thread where the event is dispatched rather than in the calling thread, but doesn't appear in the console because such errors unfortunately tend not to. Perhaps nsIThread#dispatch needs a special-case test for the suitability of the nsIRunnable object and of the run method for compatibility against the target thread?

Comment 7

[Benjamin Smedberg]

Yes, it throws in the background thread. But with sync dispatch that failure should be propagated back. But I see it is not:

http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread.cpp#417

We should probably *also* put up an error from the misuse-of-jsobject code, but fixing the event-dispatch should be pretty trivial.

Comment 8

[Andreas Gal :gal]

bsmedberg, I am not particularly at home in this part of the code. Do you have someone who can fix this? We should definitely try to fix this for final. I don't think we have to hold b8 though.

Comment 9

[Xavier]

(In reply to comment #4)
> Yes, this is supposed to throw. You can't dispatch a closure to a different
> thread any more. For the record, the test case above is not reliable, btw
> (DISPATCH_SYNC comes back as soon the event was dispatched on the other thread,
> that doesn't mean the closure has finished writing to a, even if we would
> support cross thread closure sending, which we do not any more). But thats
> irrelevant. We should never get to the dump clearly.

Hi Andreas, when you say "You can't dispatch a closure to a different
thread any more.", can you tell me if this is a recent modification (like, it was possible in FF 4.0b6 but no longer, or was it much before) ?
Thanks !

Comment 10

[Boris Zbarsky [:bzbarsky]]

> can you tell me if this is a recent modification

Yes.

Comment 11

[Xavier]

Thanks.
Nothing more specific ?
I can't find a thing in the release notes about this change.

Comment 12

[Boris Zbarsky [:bzbarsky]]

See bug 608142.  We do need to document/relnote this; added the relevant flags to that bug.

Comment 13

[Kris Maglione [:kmag]]

http://blog.mozilla.com/addons/2010/11/11/making-add-on-compatible-firefox-4/

Comment 14

[Benjamin Smedberg]

Attachment: Propagate errors during synchronous runnable dispatch, rev. 1

Comment 15

[Xavier]

(In reply to comment #12)
> See bug 608142.  We do need to document/relnote this; added the relevant flags
> to that bug.

Wow, it looks like a rather dramatic change...
So I guess this example from the documentation is totally obsolete :
https://developer.mozilla.org/en/The_thread_manager

If you can point me to replacement solutions, that would be a tremendous help, because my extension is totally broken for now.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 490599 [details] [diff] [review]
Propagate errors during synchronous runnable dispatch, rev. 1

r=me

Comment 17

[Andreas Gal :gal]

Xavier, do you mind emailing me? [e-mail] I am looking to write a blog post on how to port threading use to chrome workers, and I am looking for a concrete code example (an actual extension).

Comment 18

[Benjamin Smedberg]

Attachment: Part B - report threading errors to the JS console as well, rev. 1

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #15)
> (In reply to comment #12)
> > See bug 608142.  We do need to document/relnote this; added the relevant flags
> > to that bug.
> 
> Wow, it looks like a rather dramatic change...
> So I guess this example from the documentation is totally obsolete :
> https://developer.mozilla.org/en/The_thread_manager
> 
> If you can point me to replacement solutions, that would be a tremendous help,
> because my extension is totally broken for now.

We may unbreak you via a "mostly (sanely) compatible" fix for bug 566951, which would not require you to rewrite to use ChromeWorkers (which might require us to add more APIs to ChromeWorker global objects, fix other bugs, etc.

In short, don't panic. We know that breaking thread manager usage from JS is a breaking change, and we are investigating fixes that may give short and longer term joy. See your code, in particular what global and outer-scope variables your closure(s) that run on other threads use, would be very helpful.

/be

Comment 20

[Xavier]

(In reply to comment #19)
> (In reply to comment #15)
> > (In reply to comment #12)
> > > See bug 608142.  We do need to document/relnote this; added the relevant flags
> > > to that bug.
> > 
> > Wow, it looks like a rather dramatic change...
> > So I guess this example from the documentation is totally obsolete :
> > https://developer.mozilla.org/en/The_thread_manager
> > 
> > If you can point me to replacement solutions, that would be a tremendous help,
> > because my extension is totally broken for now.
> 
> We may unbreak you via a "mostly (sanely) compatible" fix for bug 566951, which
> would not require you to rewrite to use ChromeWorkers (which might require us
> to add more APIs to ChromeWorker global objects, fix other bugs, etc.
> 
> In short, don't panic. We know that breaking thread manager usage from JS is a
> breaking change, and we are investigating fixes that may give short and longer
> term joy. See your code, in particular what global and outer-scope variables
> your closure(s) that run on other threads use, would be very helpful.
> 
> /be

Thanks !
As emailed to Andreas, we do sqlite requests in the background thread, using a sqlite database management object instanciated in the main thread, which I guess it no longer possible.

You mention global objects and outer scope variables, but I guess it includes objects that are properties of the runnable object dispatched to the background thread, if they were instanciated in the main thread ?

Comment 21

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/3862e43372ba
http://hg.mozilla.org/mozilla-central/rev/a86f1ab3f3db
http://hg.mozilla.org/mozilla-central/rev/5edec88ca944