Closed Bug 850253 Opened 7 years ago Closed 7 years ago

Remove Off-Main-Thread XPCWrappedJS refcounting from KeyGenRunnable

Categories

(Toolkit Graveyard :: Security, defect)

x86
macOS
defect
Not set

Tracking

(b2g18 fixed, b2g-v1.1hd fixed)

RESOLVED FIXED
mozilla23
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files)

No description provided.
Blocks: 773610
No longer blocks: 850252
The basic idea here is that it's no longer safe to addref/release XPCWrappedJS
objects off-main-thread, so we need to add a level of indirection with the
nsMainThreadPtr{Holder,Handle}. I'm happy to explain more if you want, since I
understand that this review request is coming out of nowhere. ;-)
Attachment #723993 - Flags: review?(jparsons)
Comment on attachment 723993 [details] [diff] [review]
Remove Off-Main-Thread XPCWrappedJS refcounting from KeyGenRunnable. v1

That makes sense to me. 

Thanks for fixing and submitting!
j
Attachment #723993 - Flags: review?(jparsons) → review+
Is there something blocking this from landing?
Not on my end.  Bobby, good to set checkin-needed?
Well, I was bisecting all these dethreading patches with try pushes to try to find out which one was responsible for the windows crashes. This was one of the two patches in the culprit push, but I think we've pretty clearly determined that the culprit is the other bug in that push (bug 850252). So we should get this checked in, now that we have other reasons to do so.
Keywords: checkin-needed
Here's a version that is rebased against landing, and with the commit message including the r=.  It is just a trivial rebased (conflicts in the include) but I put this here to make it easier if somebody lands it.  inbound is closed right now.
Thanks for the info, Bobby!
https://hg.mozilla.org/mozilla-central/rev/43acb3f9b06b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Blocks: 844088
FYI - See my comment on the blocking bug. You might want to ask for approval for this to uplift this to b2g18.
Depends on: 771074
This patch requires bug 771074, I believe.
(In reply to Andrew McCreight [:mccr8] from comment #12)
> This patch requires bug 771074, I believe.

Hmm...but that bug was fixed on mozilla16 though, so wouldn't that already be included on b2g18?
Ah, good point, I hadn't remembered that it was that old.
Andrew, can you please request approval for uplift?
Flags: needinfo?(continuation)
Comment on attachment 723993 [details] [diff] [review]
Remove Off-Main-Thread XPCWrappedJS refcounting from KeyGenRunnable. v1

I'm not sure why you needinfoed me, as I neither reviewed nor wrote this patch, but I can give it a go.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 753238
User impact if declined: sec problems (bug 844088)
Testing completed: it has been on central for quite some time, since Fx23
Risk to taking this patch (and alternatives if risky): very low
String or UUID changes made by this patch: none
Attachment #723993 - Flags: approval-mozilla-b2g18?
Flags: needinfo?(continuation)
Comment on attachment 723993 [details] [diff] [review]
Remove Off-Main-Thread XPCWrappedJS refcounting from KeyGenRunnable. v1

Approving as this needed for B2g due to 844088
Attachment #723993 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.