850253 - Remove Off-Main-Thread XPCWrappedJS refcounting from KeyGenRunnable

Status: RESOLVED FIXED

(Toolkit Graveyard :: Security, defect)

Comment 1

[Bobby Holley (:bholley)]

Attachment: Remove Off-Main-Thread XPCWrappedJS refcounting from KeyGenRunnable. v1

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. ;-)

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Andrew McCreight [:mccr8]]

Is there something blocking this from landing?

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Not on my end.  Bobby, good to set checkin-needed?

Comment 6

[Bobby Holley (:bholley)]

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.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: "rebased" for landing

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.

Comment 8

[Andrew McCreight [:mccr8]]

Thanks for the info, Bobby!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43acb3f9b06b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43acb3f9b06b

Comment 11

[Jason Smith [:jsmith]]

FYI - See my comment on the blocking bug. You might want to ask for approval for this to uplift this to b2g18.

Comment 12

[Andrew McCreight [:mccr8]]

This patch requires bug 771074, I believe.

Comment 13

[Jason Smith [:jsmith]]

(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?

Comment 14

[Andrew McCreight [:mccr8]]

Ah, good point, I hadn't remembered that it was that old.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Andrew, can you please request approval for uplift?

Comment 16

[Andrew McCreight [:mccr8]]

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

Comment 17

[bhavana bajaj [:bajaj]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2d1fe68319cc

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/bb1ba0f59b31