Closed Bug 1056418 Opened 6 years ago Closed 6 years ago

testGCFinalizeCallback failure with no default compartment object on the context

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: bholley, Assigned: jonco)

References

Details

Attachments

(1 file)

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()
Flags: needinfo?(jcoppeard)
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.
Assignee: nobody → jcoppeard
Attachment #8476634 - Flags: review?(terrence)
Flags: needinfo?(jcoppeard)
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?
Attachment #8476634 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/359dfa6e0878
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.