GC: Recalculation of GC fullness in EndSweepPhase broken

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

({csectype-uaf, regression, sec-critical})

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 unaffected, firefox19 unaffected, firefox20+ fixed, firefox21+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main20-])

Attachments

(1 attachment, 4 obsolete attachments)

Bug 790338 re-broke calculation of whether the GC is full vs. compartmental in EndSweepPhase (I can't find the original defect right now).
That seems like it could do bad things to how we schedule GC and CC.
Posted patch Proposed fix (obsolete) — Splinter Review
Fix the issue and add regression tests.
Group: core-security
Ah, yes, this problem. Bug 801957 is the one you were thinking of. Thanks for writing regression tests, as this is clearly a touchy area of the code. :)
Assignee: general → jcoppeard
Posted patch Proposed fix (obsolete) — Splinter Review
Attachment #697909 - Attachment is obsolete: true
Attachment #698758 - Flags: review?(wmccloskey)
Comment on attachment 698758 [details] [diff] [review]
Proposed fix

Review of attachment 698758 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TestingFunctions.cpp
@@ +1072,5 @@
>  bool
>  js::DefineTestingFunctions(JSContext *cx, HandleObject obj)
>  {
> +#ifdef JS_GC_ZEAL
> +    JS_SetFinalizeCallback(cx->runtime, TestFinalizeCallback);

This code can execute from the browser, in which case it will conflict with the browser's finalize callback.

I think this might work better as a JSAPI test. Can you try that instead?

::: js/src/jsgc.cpp
@@ +4327,5 @@
>      AssertCanGC();
>      Collect(rt, true, SliceBudget::Unlimited, gckind, reason);
>  }
>  
> +static bool CompartmentsSelected(JSRuntime *rt)

Need a newline between bool and the function name.
Attachment #698758 - Flags: review?(wmccloskey)
Blocks: 826673
Posted patch Fix with updated test code (obsolete) — Splinter Review
Attachment #698758 - Attachment is obsolete: true
Attachment #699312 - Flags: review?(wmccloskey)
Comment on attachment 699312 [details] [diff] [review]
Fix with updated test code

Review of attachment 699312 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi-tests/testGCFinalizeCallback.cpp
@@ +16,5 @@
> +
> +static void
> +FinalizeCallback(JSFreeOp *fop, JSFinalizeStatus status, JSBool isCompartmentGC)
> +{
> +	if (FinalizeCalls < BufferSize) {

It looks like some tab characters snuck in here. Please replace with spaces.

@@ +97,5 @@
> +    CHECK(checkFinalizeIsCompartmentGC(true));
> +
> +#ifdef JS_GC_ZEAL
> +
> +	/* Full GC with reset due to new compartment, becoming compartment GC. */

Another tab here.
Attachment #699312 - Flags: review?(wmccloskey) → review+
Attachment #699312 - Attachment is obsolete: true
Attachment #701064 - Attachment is obsolete: true
Comment on attachment 701106 [details] [diff] [review]
Also fix whitespace issues in patch

Please take the printf out of the test.
Attachment #701106 - Flags: review+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/31f4f50447e8

Note wrong bug number in commit message!
You should nominate the patch for Aurora approval, if you think it is ready to land there.
Flags: in-testsuite+
Target Milestone: --- → mozilla21
Comment on attachment 701106 [details] [diff] [review]
Also fix whitespace issues in patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 790338
User impact if declined: Potential to cause crashes, hence security issue
Testing completed (on m-c, etc.): This patch has been on m-c for about a week
Risk to taking this patch (and alternatives if risky): low risk, restores original behaviour before regression
String or UUID changes made by this patch: none
Attachment #701106 - Flags: approval-mozilla-aurora?
Comment on attachment 701106 [details] [diff] [review]
Also fix whitespace issues in patch

Approving on aurora as it has a low risk patch restoring original behavior, which led to this regression. Tested on m-c for a week.
Attachment #701106 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.