Closed Bug 610381 Opened 9 years ago Closed 9 years ago

nsIThread#dispatch fails silently from JS

Categories

(Core :: JavaScript Engine, defect, major)

x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: kmag, Assigned: benjamin)

References

Details

Attachments

(2 files)

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
Duplicate of this bug: 610384
Andreas, I thought this was supposed to throw... is it not doing that?
I have the exact same problem with Firefox 4.0b7
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.
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.
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?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
blocking2.0: --- → ?
(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 !
> can you tell me if this is a recent modification

Yes.
Thanks.
Nothing more specific ?
I can't find a thing in the release notes about this change.
See bug 608142.  We do need to document/relnote this; added the relevant flags to that bug.
(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 on attachment 490599 [details] [diff] [review]
Propagate errors during synchronous runnable dispatch, rev. 1

r=me
Attachment #490599 - Flags: review?(bzbarsky) → review+
Xavier, do you mind emailing me? gal@mozilla.com 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).
Assignee: general → benjamin
Status: NEW → ASSIGNED
Attachment #490702 - Flags: review?(jst)
blocking2.0: ? → betaN+
(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
(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 ?
Attachment #490702 - Flags: review?(jst) → review+
You need to log in before you can comment on or make changes to this bug.