770840 - Add runtime abort for using an XPCWrappedJS off the main thread

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Looks like this actually happens in a few places. :-(

Currently it gets caught by various belt-and-suspenders checking, so it never manages to actually hit our abort. We should fix this code to just runtime-abort.

Comment 1

[Bobby Holley (:bholley)]

All of the deps here have now landed, so this should be good to go.

https://tbpl.mozilla.org/?tree=Try&rev=348dbf349cd4

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove XPConnect test that explicitly runs JS on a seperate thread. v1

This test used to check that we'd error-out properly, but now it'll just cause us to abort.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add Runtime aborts when using XPCWrappedJS off-main-thread. v2

Comment 4

[Bobby Holley (:bholley)]

Looks like jdm's nsSocketTransport2 patch didn't catch all of the cases where we addref/release XPCWrappedJS off-main-thread. See the crashes in the comment 1 try push.

JDM, is this something you think you can fix?

Comment 5

[Josh Matthews [:jdm]]

Yes, I'll try to take some time this week to dig in some more.

Comment 6

[Bobby Holley (:bholley)]

Awesome, thanks JDM! :-)

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 737018 [details] [diff] [review]
Part 2 - Add Runtime aborts when using XPCWrappedJS off-main-thread. v2

r=me assuming this is green.

Comment 8

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc459138617
https://hg.mozilla.org/integration/mozilla-inbound/rev/6be352955252

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1bc459138617
https://hg.mozilla.org/mozilla-central/rev/6be352955252

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 870782.
https://hg.mozilla.org/mozilla-central/rev/2673016e7df4

This also caused c-c bustage, fwiw.
https://tbpl.mozilla.org/php/getParsedLog.php?id=22823512&tree=Thunderbird-Trunk

Comment 11

[Josh Matthews [:jdm]]

Try run at https://tbpl.mozilla.org/?tree=Try&rev=12be5cb02e9d.

Comment 12

[Josh Matthews [:jdm]]

Giving try another shot before landing: https://tbpl.mozilla.org/?tree=Try&rev=274e9ae31511

Comment 13

[Josh Matthews [:jdm]]

It's super effective!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/593e06824257
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fca5e32db9d7

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/593e06824257
https://hg.mozilla.org/mozilla-central/rev/fca5e32db9d7

Comment 15

[Bobby Holley (:bholley)]

\o/

Thanks a million, jdm.

Comment 16

[Honza Bambas (:mayhemer)]

This completely destroys possibility to write xpcshell tests that have callbacks on non-main thread :(((

Coming at the right moment we have successfully written tests for the new http cache...

Comment 17

[Josh Matthews [:jdm]]

I recommend writing a debug C++ component that proxies the callback to a main thread listener, if possible.

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to Josh Matthews [:jdm] from comment #17)
> I recommend writing a debug C++ component that proxies the callback to a
> main thread listener, if possible.

Yes, that is obvious.  But I need a result.