Add runtime abort for using an XPCWrappedJS off the main thread

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Depends on 1 bug)

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Depends on: 771074
Depends on: 773610
No longer depends on: 771074
Depends on: 840592
All of the deps here have now landed, so this should be good to go.

https://tbpl.mozilla.org/?tree=Try&rev=348dbf349cd4
This test used to check that we'd error-out properly, but now it'll just cause us to abort.
Attachment #737017 - Flags: review?(mrbkap)
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?
Flags: needinfo?(josh)
Yes, I'll try to take some time this week to dig in some more.
Flags: needinfo?(josh)
Awesome, thanks JDM! :-)
Attachment #737017 - Flags: review?(mrbkap) → review+
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.
Attachment #737018 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/1bc459138617
https://hg.mozilla.org/mozilla-central/rev/6be352955252
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 870782
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla23 → ---
Depends on: 870916
Depends on: 871125
Depends on: 870887
No longer depends on: 870887
No longer depends on: 870916, 871125
Giving try another shot before landing: https://tbpl.mozilla.org/?tree=Try&rev=274e9ae31511
https://hg.mozilla.org/mozilla-central/rev/593e06824257
https://hg.mozilla.org/mozilla-central/rev/fca5e32db9d7
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
\o/

Thanks a million, jdm.
Depends on: 882196
Depends on: 882200
Depends on: 882125
Depends on: 882424
Depends on: 882637
Depends on: 882328
Depends on: 883495
Depends on: 884200
Depends on: 884792
Depends on: 885292
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...
I recommend writing a debug C++ component that proxies the callback to a main thread listener, if possible.
(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.
Depends on: 887191
Depends on: 889885
Depends on: 892340
Depends on: 1205205
You need to log in before you can comment on or make changes to this bug.