758471 - Don't sweep native interfaces during compartment GC

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This is the one aspect of bug 715761 that was correctness-related. Right now, we mark native interfaces when we mark XPCWrappedNativeProto and XPCWrappedNative. We won't mark an XPCWrappedNativeProto or XPCWrappedNative if it's not in a compartment that's being collected. However, we'll still sweep their native interfaces. That's bad.

This patch skips the sweeping if we aren't collecting all compartments. This is kind of an imperfect solution, because most GCs now are compartment GCs. However, it's hard for me to think of a situation where we'll accumulate tons of native interfaces in a short period of time. So I think it's okay that they have to wait until a full GC to be swept.

Eventually it would be nice if the maps for native interfaces were per-compartment rather than per-runtime. Then we could sweep only the ones that were marked. But that's a bigger change than I have time for right now.

The motivation for this is that I saw a stack pop on tryserver today with native interface sweeping at the top. So it seems like we might be hitting this. Recent patches have made it a lot more likely.

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 627060 [details] [diff] [review]
patch

>+    if (arg == UNMARK_ONLY)
>+        return JS_DHASH_NEXT;
>+

Please add a big comment here explaining why this is necessary (especially the part how we can't just skip marking them because they get marked when marking WNs and protos, which we _do_ want to do for compartmental GC).

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a172444cca11

Comment 3

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a172444cca11

Comment 4

[Daniel Veditz [:dveditz]]

(In reply to Bill McCloskey (:billm) from comment #0)
> This is the one aspect of bug 715761 that was correctness-related.

"Correctness" in GC meaning a potential security problem? Does this affect ESR or Beta (fx14) and need to be fixed on those branches?

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This goes back to when compartment GC was introduced in Firefox 4. When I noticed the problem in bug 715761, it seemed like a theoretical problem. Lately we've been doing more compartment GCs and I started noticing crashes on tryserver that seemed like they might be related. The patch in this bug seemed to make them go away.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 627060 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Low likelihood of crashes in xpconnect, possible exploitation.
Fix Landed on Version: 15
Risk to taking this patch (and alternatives if risky): Patch will require some rebasing that may introduce bugs. Otherwise low risk, although there's a theoretical possibility that we'll take longer to free some memory.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 605662
User impact if declined: Low likelihood of crashes in xpconnect, possible exploitation.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low risk, although there's a theoretical possibility that we'll take longer to free some memory.
String or UUID changes made by this patch: None

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 627060 [details] [diff] [review]
patch

approving for Beta and ESR but if this fix landed on 15 then we don't need it for Aurora.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a272c966d0ca
https://hg.mozilla.org/releases/mozilla-esr10/rev/814e0e9a8618

Comment 9

[u279076]

Is there something QA can do to verify this fix?