Last Comment Bug 758471 - Don't sweep native interfaces during compartment GC
: Don't sweep native interfaces during compartment GC
Status: RESOLVED FIXED
[advisory-tracking+][qa?]
: sec-critical
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: [PTO to Dec5] Bill McCloskey (:billm)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-24 18:09 PDT by [PTO to Dec5] Bill McCloskey (:billm)
Modified: 2012-07-20 18:25 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed


Attachments
patch (9.11 KB, patch)
2012-05-24 18:09 PDT, [PTO to Dec5] Bill McCloskey (:billm)
bobbyholley: review+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description [PTO to Dec5] Bill McCloskey (:billm) 2012-05-24 18:09:06 PDT
Created attachment 627060 [details] [diff] [review]
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) (busy with Stylo) 2012-05-25 09:38:44 PDT
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 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-25 11:07:15 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a172444cca11
Comment 3 Ed Morley [:emorley] 2012-05-26 05:27:26 PDT
https://hg.mozilla.org/mozilla-central/rev/a172444cca11
Comment 4 Daniel Veditz [:dveditz] 2012-06-22 10:40:48 PDT
(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 [PTO to Dec5] Bill McCloskey (:billm) 2012-06-22 11:11:07 PDT
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 [PTO to Dec5] Bill McCloskey (:billm) 2012-06-22 11:16:24 PDT
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 2012-06-22 15:35:22 PDT
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 9 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-12 15:11:13 PDT
Is there something QA can do to verify this fix?

Note You need to log in before you can comment on or make changes to this bug.