775435 - GC hazard with cross-compartment sNPObjectMemberClass::npobjWrapper

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I was reading over my patches for bug 771202, and realized that I screwed this one up. :-(

GetNPObjectWrapper currently returns an unwrapped JSObject*, since its primary consumer is GetNPObject, which just grabs the private (an NPObject*). But it's _also_ used in CreateNPObjectMember when storing |npobjWrapper|:

https://hg.mozilla.org/mozilla-central/rev/0d561abceeb3#l1.234

I thought this was ok at the time, but sNPObjectMemberClass actually has a trace hook NPObjectMember_Trace, which means that npobjWrapper very much needs to be same-compartment with the NPObjectMember. Patch coming right up.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Wrap-by-default in GetNPObjectWrapper. v1

Attaching a patch. Flagging bsmedberg for review.

Comment 2

[Bobby Holley (:bholley)]

I guess this is probably s-s.

Comment 3

[Benjamin Smedberg]

Comment on attachment 643765 [details] [diff] [review]
Wrap-by-default in GetNPObjectWrapper. v1

Yeah. Damn this is complicated.

Comment 4

[Bobby Holley (:bholley)]

Pushed to try here:
https://tbpl.mozilla.org/?tree=Try&rev=83cf9dab23ac

Looks green modulo infra orange, so pushing to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6bff6810a82e

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6bff6810a82e

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 643765 [details] [diff] [review]
Wrap-by-default in GetNPObjectWrapper. v1

Waiting to hear whether this reduced crashes over in bug 774052, but we should take this on m-c and m-b regardless, because it's an obvious GC hazard. Requesting approval.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 771202
User impact if declined: Potential crashes and security bugs
Testing completed (on m-c, etc.): baked on m-c over the weekend. The crash this may fix is being tracked in bug 774052.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None

Comment 7

[Alex Keybl [:akeybl]]

Comment on attachment 643765 [details] [diff] [review]
Wrap-by-default in GetNPObjectWrapper. v1

[Triage Comment]
Cleanup for uplifted patch, and low risk. Let's take these on branches.

Comment 8

[Bobby Holley (:bholley)]

Pushed to m-a:
http://hg.mozilla.org/releases/mozilla-aurora/rev/da5ea41d129d

Comment 9

[Bobby Holley (:bholley)]

Pushed to m-b:
http://hg.mozilla.org/releases/mozilla-beta/rev/7bf668f1e547

Comment 10

[Manuela Muntean [Away]]

What is the visible impact while testing this on a build that reproduces this issue?