1056418 - testGCFinalizeCallback failure with no default compartment object on the context

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

This seems to be the only remaining failure blocking my patches in bug 981218 to remove default compartment objects (which would be super-duper-awesome). I discussed it with terrence and bill, and they suggested that I file a bug and NI jonco.

Jonco, is there any chance you can look at this before thursday afternoon? I'm leaving on PTO tomorrow afternoon (thursday) and it would be awesome to get bug 981218 landed before I leave (and thus for this cycle). No worries if not though - it can probably wait until the next cycle.

To reproduce, apply this patch:

diff --git a/js/src/jsapi-tests/tests.h b/js/src/jsapi-tests/tests.h
index dfe0023..004c85c 100644
--- a/js/src/jsapi-tests/tests.h
+++ b/js/src/jsapi-tests/tests.h
@@ -306,6 +306,7 @@ class JSAPITest
         if (!cx)
             return nullptr;
         JS_SetErrorReporter(cx, &reportError);
+        JS::ContextOptionsRef(cx).setNoDefaultCompartmentObject(true);
         return cx;
     }

And run jsapi-tests testGCFinalize. Output:

TEST-UNEXPECTED-FAIL | testGCFinalizeCallback | CHECK failed: FinalizeCalls % 2 == 1CHECK failed: checkMultipleGroups()

Comment 1

[Jon Coppeard (:jonco)]

Attachment: finalize-callback-patch

It took me a while to see what was happening but it seems it's just the test code at fault here.

Calling JSAPITest::createGlobal() did more than I expected and overwrote the objects main global root with the new object, unrooting the original global.  This can prompt the GC run a second collection to clean up the compartment which it thinks should die as it has no roots or incoming pointers to it, and this in turn confuses the test by calling the finalize callback more than the expected number of times.

The fix is just to not overwrite JSAPITest::global when creating new globals inside the test.

I also factored out add/remove of the finalize callback into init()/uninit() overrides so that if the test fails then the callback is removed.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8476634 [details] [diff] [review]
finalize-callback-patch

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

::: js/src/jsapi-tests/testGCFinalizeCallback.cpp
@@ +37,5 @@
>      CHECK(global1);
>      CHECK(global2);
>      CHECK(global3);
>  
> +

Extra \n?

Comment 3

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/359dfa6e0878

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/359dfa6e0878