Closed Bug 1282113 Opened 3 years ago Closed 3 years ago

Crashes involving setGCCallback or Assertion failure: typeDescrObjects.empty(), at js/src/gc/Zone.cpp:61

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox50 --- verified

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update][adv-main50-])

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 939ecc4e9d05 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

Object.getOwnPropertyNames(this);
setGCCallback({
    action: "majorGC",
    phases: "begin"
});
selectforgc(this);


Backtrace:

0   js-dbg-64-dm-clang-darwin-939ecc4e9d05	0x000000010a1fcdce JS::Zone::~Zone() + 462 (Zone.cpp:61)
1   js-dbg-64-dm-clang-darwin-939ecc4e9d05	0x0000000109d44997 js::gc::GCRuntime::finish() + 391 (Utility.h:256)
2   js-dbg-64-dm-clang-darwin-939ecc4e9d05	0x0000000109f80878 JSRuntime::~JSRuntime() + 1032 (Runtime.cpp:467)
3   js-dbg-64-dm-clang-darwin-939ecc4e9d05	0x0000000109c8b156 JS_DestroyRuntime(JSRuntime*) + 22 (Utility.h:256)
4   js-dbg-64-dm-clang-darwin-939ecc4e9d05	0x00000001096ec181 main + 13249 (js.cpp:7453)
5   js-dbg-64-dm-clang-darwin-939ecc4e9d05	0x00000001096e6934 start + 52
/snip

For detailed crash information, see attachment.

Setting [fuzzblocker] because this is happening frequently and also s-s as a start because GC is in the testcase.
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160622003502" and the hash "e3c00a0692367cc1ea6cf6f40813abfaa74eba8a".
The "bad" changeset has the timestamp "20160622005003" and the hash "0ca871e39a20d94c5c8948beb41867d679f3709e".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e3c00a0692367cc1ea6cf6f40813abfaa74eba8a&tochange=0ca871e39a20d94c5c8948beb41867d679f3709e

Jan, is bug 1279295 a likely regressor?
Blocks: 1279295
Flags: needinfo?(jdemooij)
Basically, setGCCallback is virtually broken as it is causing various assertion failures and crashes including this one.
Summary: Assertion failure: typeDescrObjects.empty(), at js/src/gc/Zone.cpp:61 → Crashes involving setGCCallback or Assertion failure: typeDescrObjects.empty(), at js/src/gc/Zone.cpp:61
Attached patch PatchSplinter Review
We no longer do the DESTROY_CONTEXT GC, I think that exposed this somehow.

While we do a DESTROY_RUNTIME GC, we trigger the GC callback and it starts a major GC. This nested GC then resets some global state, so the DESTROY_RUNTIME GC no longer cleans up everything.

This patch disallows non-shutdown GCs while we're destroying the runtime. We're going to destroy everything anyway.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8765369 - Flags: review?(terrence)
See also bug 1277833, an intermittent with the same signature. It started 3 weeks ago so it's definitely unrelated to bug 1279295.

I wonder if it could be related.
See Also: → 1277833
Comment on attachment 8765369 [details] [diff] [review]
Patch

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

::: js/public/GCAPI.h
@@ -53,5 @@
>      /* Reasons internal to the JS engine */     \
>      D(API)                                      \
>      D(EAGER_ALLOC_TRIGGER)                      \
>      D(DESTROY_RUNTIME)                          \
> -    D(DESTROY_CONTEXT)                          \

If you just delete this, it will throw off all of the reason telemetry buckets. Please instead replace D(DESTROY_CONTEXT) with D(UNUSED0).

::: js/src/jsgc.cpp
@@ +6282,5 @@
>  
> +    // Only allow shutdown GCs when we're destroying the runtime. This keeps
> +    // the GC callback from triggering a nested GC and resetting global state.
> +    if (rt->isBeingDestroyed() && !IsShutdownGC(reason))
> +        return false;

Ugh. It's absolutely awful that GC an recurse at all, but that's the status quo right now.
Attachment #8765369 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/bc6e009a06d2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Group: javascript-core-security → core-security-release
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main50-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.