Closed Bug 945280 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: decoder, Assigned: jonco)

References

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:ignore])

Attachments

(1 file, 1 obsolete file)

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 = [];
Attached patch bug945280-disable (obsolete) — Splinter Review
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.
Assignee: general → jcoppeard
Attachment #8346574 - Flags: review?(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.
Attachment #8346574 - Flags: review?(terrence) → review-
Well that doesn't seem to fix this problem, but here is a better patch based on top of those changes.
Attachment #8346574 - Attachment is obsolete: true
Attachment #8346659 - Flags: review?(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;
Attachment #8346659 - Flags: review?(terrence) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/3fc4cf899a34
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.