826649 - GC: Recalculation of GC fullness in EndSweepPhase broken

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Bug 790338 re-broke calculation of whether the GC is full vs. compartmental in EndSweepPhase (I can't find the original defect right now).

Comment 1

[Andrew McCreight [:mccr8]]

That seems like it could do bad things to how we schedule GC and CC.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Proposed fix

Fix the issue and add regression tests.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Proposed fix

Comment 5

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

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.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Fix with updated test code

Comment 7

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

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.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Fix for test failures in opt builds

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Also fix whitespace issues in patch

Comment 10

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

Comment on attachment 701106 [details] [diff] [review]
Also fix whitespace issues in patch

Please take the printf out of the test.

Comment 11

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31f4f50447e8

Comment 12

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/mozilla-central/rev/31f4f50447e8

Note wrong bug number in commit message!

Comment 13

[Andrew McCreight [:mccr8]]

You should nominate the patch for Aurora approval, if you think it is ready to land there.

Comment 14

[Jon Coppeard (:jonco)]

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

Comment 15

[bhavana bajaj [:bajaj]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/07e1d48fd8f7

(With bug # fixed)