Closed Bug 952818 Opened 6 years ago Closed 6 years ago

GenerationalGC: Assertion failure: isTenured(), at gc/Heap.h

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: gkw, Assigned: terrence)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(4 files, 2 obsolete files)

Attached file stack
(function() {
    eval("\
        (function() {\
            var f = function(){\
                f\
            }\
        })()\
    ")
})()

asserts js debug shell on m-c changeset 599100c4ebfe with --ion-parallel-compile=off --ion-eager --ion-check-thread-safety at Assertion failure: isTenured(), at gc/Heap.h

My configure flags are:

AR=ar sh ./configure --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-exact-rooting --enable-elf-hack --enable-stdcxx-compat --enable-warnings-as-errors --enable-signmar --disable-elf-hack --enable-js-diagnostics --with-intl-api=build --enable-ctypes --disable-shared-js --enable-jemalloc --enable-gcgenerational --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/06f844b81f3d
user:        Brian Hackett
date:        Wed Nov 27 16:28:57 2013 -0700
summary:     Bug 938124 - Add classes for use in IonBuilder thread safety analysis, r=jandem.

Bug 938124 was probably the changeset that added --ion-check-thread-safety, so I'm not sure if it's the regressor.
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)
Blocks: ggcfuzz
This reproduces in non-optimised debug builds also.

IonBuilder::hasStaticScopeObject() encounters a call object in the nursery when traversing the current script's function's scope chain.  I don't think this should be touching any nursery pointers here.

The CallObject::create() implementations tenure the object if treatAsRunOnce() is true for the script, and this is checked in hasStaticScopeObject().  However this is the outer script only - from what I can see the inner script can still have a nursery call object.

Brian, do you know how this is supposed to work?
Assignee: nobody → jcoppeard
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)
Flags: needinfo?(bhackett1024)
Attached patch patch (obsolete) — Splinter Review
hasStaticScopeObject doesn't work with generational GC, as there is no guarantee the scope objects being traversed are not in the nursery.  The attached patch restructures the addendums attached to type objects so that the type object for a run once function can hold pointers to the single call object for it.  This also uses the addendum for a type object's interpreted function, which allows cutting sizeof(TypeObject) by two words on 32 bit platforms.
Assignee: jcoppeard → bhackett1024
Attachment #8355549 - Flags: review?(jdemooij)
Flags: needinfo?(bhackett1024)
Assignee: bhackett1024 → terrence
Comment on attachment 8355549 [details] [diff] [review]
patch

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

Terrence, can you land this patch before it bitrots (more)? It would be good to check Octane just to be sure.

::: js/src/jit/Ion.cpp
@@ +1611,4 @@
>  {
>      JS_ASSERT(obj->hasSingletonType());
>  
> +    // Keep track of 

This should probably be: "Keep track of run-once call objects." or something.

@@ +1614,5 @@
> +    // Keep track of 
> +    JS_ASSERT(obj->callee().hasSingletonType());
> +    types::TypeObject *type = obj->callee().getType(cx);
> +    if (type && !type->hasRunOnceCallObject())
> +        type->initAddendum(types::TypeObject::RunOnceCallObject, obj);

It would be nice to assert either here or in initAddendum that obj is not allocated in the nursery.

::: js/src/jsinfer.cpp
@@ +3011,5 @@
> +    JS_ASSERT(!(flags() & OBJECT_FLAG_ADDENDUM_MASK));
> +    JS_ASSERT(addendum);
> +
> +    JS_ASSERT(kind <= OBJECT_FLAG_ADDENDUM_MASK >> OBJECT_FLAG_ADDENDUM_SHIFT);
> +    this->flags_ |= (kind << OBJECT_FLAG_ADDENDUM_SHIFT);

Nit: add a JS_ASSERT(addendumKind() == kind); after this line -- it's too easy to break this when moving flags around etc.
Attachment #8355549 - Flags: review?(jdemooij) → review+
Comment on attachment 8355549 [details] [diff] [review]
patch

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

We need this patch to happen and Brian is out until February, so I am going to drive it forward. I've read it over carefully now and I think it's a great cleanup that we should get in asap.

This probably still deserves a look from Jan, so I'll upload a rebased version in a bit with, the test, and my own review commentary included.

::: js/src/jit/Ion.cpp
@@ +1611,4 @@
>  {
>      JS_ASSERT(obj->hasSingletonType());
>  
> +    // Keep track of 

Looks like this comment got chopped off. It's pretty clear what's happening here, so I don't think a comment is really even needed.

::: js/src/jit/IonBuilder.cpp
@@ +9398,5 @@
>      // so there will be only one call object and the aliased var access can be
> +    // compiled in the same manner as a global access. Look for the call
> +    // object, which was hopefully installed by TrackCallObjectProperties.
> +
> +    *pcall = funType->runOnceCallObject();

\o/ Nice!

::: js/src/jsinfer.cpp
@@ +3012,5 @@
> +    JS_ASSERT(addendum);
> +
> +    JS_ASSERT(kind <= OBJECT_FLAG_ADDENDUM_MASK >> OBJECT_FLAG_ADDENDUM_SHIFT);
> +    this->flags_ |= (kind << OBJECT_FLAG_ADDENDUM_SHIFT);
> +    this->addendum = addendum;

This needs a post-barrier barrier or an assertion that addendum is not in the nursery.

@@ +3028,5 @@
>      }
>  
>      /*
> +     * It is possible for the object to not have a new script addendum yet, but
> +     * but to have one added in the future. When analyzing properties of new

Remove the second "but".
Attachment #8355549 - Flags: review?(jdemooij)
Attachment #8355549 - Flags: review+
Having mid-aired with Jan in reviewing this, I'm a bit terrified at how identical our reviews are.
Attachment #8355549 - Attachment is obsolete: true
Attachment #8355549 - Flags: review?(jdemooij)
Attachment #8356740 - Flags: review+
I think awfy doesn't really like this commit. I see a 27% decrease on octane-typearray...
And backed out for adding 3 new hazards.
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e682c34d75

(In reply to Hannes Verschore [:h4writer], pto 13th - 17th of January from comment #7)
> I think awfy doesn't really like this commit. I see a 27% decrease on
> octane-typearray...

Ouch! I'll take a look at that too.
(In reply to Terrence Cole [:terrence] from comment #8)
> And backed out for adding 3 new hazards.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/23e682c34d75
> 
> (In reply to Hannes Verschore [:h4writer], pto 13th - 17th of January from
> comment #7)
> > I think awfy doesn't really like this commit. I see a 27% decrease on
> > octane-typearray...
> 
> Ouch! I'll take a look at that too.

Looks like it also broke m4: https://tbpl.mozilla.org/php/getParsedLog.php?id=32657616&tree=Mozilla-Inbound
And m-oth: https://tbpl.mozilla.org/php/getParsedLog.php?id=32658511&tree=Mozilla-Inbound
The perf problem is that we are not performing the new analysis during non-compilation analysis or before inlining. Fixing those two oversights results in a consistent 1% speed boost; I have no idea why this is significantly faster as it's just a refactoring. Still, I'll take it.
This is still "crashing" with signal 11 in the middle of browser tests. I've reprod, but I can't figure out what's going on: the code continues fine and seems to pass the test if I just ignore the signal by continuing. I managed to get TBPL to crash in a shell test as well, but I haven't been able to repro that locally yet.

https://tbpl.mozilla.org/?tree=Try&rev=24b7a152cb7e
Since Try ate my patch, let's re-upload it to the bug.
Attachment #8356740 - Attachment is obsolete: true
Attachment #8370199 - Flags: feedback?(bhackett1024)
The reason that mochitest 4 and other were crashing is that TypeObject::sizeOfExcludingThis() was passing non-malloc-allocated addendum pointers to the mallocSizeOf function.  I changed this to return zero when then addendum is a GC thing, as in that case the memory will be accounted for elsewhere.

I put back in most of the commented-out optimisations, but two of them cause a problem.  Calling TrackPropertiesForSingletonScopes() can acquire the compilation lock, so calling this from places in IonBuilder where the lock is already taken is a problem.  This happens in hasStaticScopeObject() and canInlineTarget().

The comments say the lock is not reentrant.  I don't understand the locking scheme enough to know how to proceed here.

Without these two optimisations however the mochitests are green.
Comment on attachment 8370199 [details] [diff] [review]
do_runonce_computation_on_main_thread-v2.diff

This doesn't reproduce without off-main-thread ionbuilder.
Attachment #8370199 - Flags: feedback?(bhackett1024)
Let's add the regression test from this bug so that this doesn't get dropped when we re-enable threading in IonBuilder.
Attachment #8379150 - Flags: review?(sphink)
Attachment #8379150 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/a444cf2a9b09
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.