945280 - GenerationalGC: Assertion failure: position_ == start(), at ../gc/Nursery.cpp:80

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central built with --enable-exact-rooting --enable-gcgenerational, revision 53d55d2d0a25 (run with --fuzzing-safe):


gczeal(7,1);
setObjectMetadataCallback(setObjectMetadataCallback);
gczeal(false);
var statusitems = [];

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug945280-disable

I like setObjectMetadataCallback(setObjectMetadataCallback)!

The problem here is that the store buffer is expected to be empty when it is re-enabled, but that conflicts with out generational zeal mode that doesn't reset the position to the start of the nursery after sweeping.

The fix is to rest the position in disable() if we're in this zeal mode.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8346574 [details] [diff] [review]
bug945280-disable

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

There are actually a bunch of other weaknesses around this area. A more comprehensive fix is in bug 945250. You should probably just import that into your queue while investigating these fuzz bugs, as I expect it fixes most of them. It's been a week since I posted that patch: I'll bug billm to review it today or punt it to you if he doesn't have time.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug945280-disable

Well that doesn't seem to fix this problem, but here is a better patch based on top of those changes.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8346659 [details] [diff] [review]
bug945280-disable

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

Ah, I indeed did not catch that particular transition. I am really starting to hate all this distributed state management. Coule we at least rephrase the jsgc.cpp changes a bit so that the enable and disable are adjacent? I think something like the below will work nicely.

--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -1016,25 +1016,28 @@ js::SetGCZeal(JSRuntime *rt, uint8_t zea
 {
     if (zeal == 0) {
         if (rt->gcVerifyPreData)
             VerifyBarriers(rt, PreBarrierVerifier);
         if (rt->gcVerifyPostData)
             VerifyBarriers(rt, PostBarrierVerifier);
     }
 
+#ifdef JSGC_GENERATIONAL
+    if (zeal == ZealGenerationalGCValue)
+        rt->gcNursery.enterZealMode();
+    else if (rt->gcZeal_ == ZealGenerationalGCValue)
+        rt->gcNursery.leaveZealMode();
+#endif
+
     bool schedule = zeal >= js::gc::ZealAllocValue;
     rt->gcZeal_ = zeal;
     rt->gcZealFrequency = frequency;
     rt->gcNextScheduled = schedule ? frequency : 0;
 
-#ifdef JSGC_GENERATIONAL
-    if (zeal == ZealGenerationalGCValue)
-        rt->gcNursery.enterZealMode();
-#endif
 }
 
 static bool
 InitGCZeal(JSRuntime *rt)
 {
     const char *env = getenv("JS_GC_ZEAL");
     if (!env)
         return true;

Comment 5

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fc4cf899a34

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #4)

So I pushed a slightly different tidyup to the one you suggested, before realising that your one would work just as well.

I did have to remove the |if (zeal == 0)| from around the previous block as this was causing test failures for the GGC analysis build.

Comment 7

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #6)
> (In reply to Terrence Cole [:terrence] from comment #4)
> 
> So I pushed a slightly different tidyup to the one you suggested, before
> realising that your one would work just as well.
> 
> I did have to remove the |if (zeal == 0)| from around the previous block as
> this was causing test failures for the GGC analysis build.

What you checked in looks great! I'll be the first to admit that though the logic I suggested was minimal, it was less obviously right than what you went with.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3fc4cf899a34