Closed Bug 958804 Opened 6 years ago Closed 6 years ago

Assertion failure: hasScript(), at js/src/jsfun.h:337

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: dbaron, Assigned: till)

References

Details

(Keywords: assertion, crash)

Attachments

(3 files, 1 obsolete file)

After updating my local debug build around noon pacific time today from mozilla-central, I crashed twice today with:

Assertion failure: hasScript(), at /home/dbaron/builds/ssd/mozilla-central/mozilla/js/src/jsfun.h:337

The second time was on Google Maps (classic).  I forgot what the first was on; it was during a meeting.

I presume this is a regression from bug 886193.
(crashed a third time while restoring my session, too)
Looks like I misunderstood the circumstances under which a function can be the scope object for another function. I thought that could only happen if the outer function contained the inner one as a child - in which case numInnerFunctions should be > 0.

Anyway, since we have a context just slightly higher up the stack here, this patch funnels it through to GenerateScopeChainGuard and makes that use fun->getOrCreateScript instead of nonLazyScript. r? instead of just landing because of the error handling stuff.
Attachment #8358788 - Flags: review?(jdemooij)
Assignee: nobody → till
Status: NEW → ASSIGNED
Comment on attachment 8358788 [details] [diff] [review]
Make sure functions are non-lazy when generating Ion scope chain guards

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

::: js/src/jit/IonCaches.cpp
@@ +3987,5 @@
>          // variables).
>          CallObject *callObj = &scopeObj->as<CallObject>();
>          if (!callObj->isForEval()) {
>              JSFunction *fun = &callObj->callee();
> +            JSScript *script = fun->getOrCreateScript(cx);

getOrCreateScript can GC I think, so you'd have to root scopeObj and probably other pointers on the stack.

However, we're only interested in script->funHasExtensibleScope() for a minor optimization (avoid a shape guard), so I think delazifying the whole script is probably not worth it and we can just check hasScript() before calling nonLazyScript().

It seems an edge case because: (1) we don't relazify if the compartment is active, so it's unlikely to hit any of our major benchmarks (2) apparently none of our tests hit this case (3) *NAME (not *GNAME) ops/ICs aren't used much since the aliased var ops (think slow stuff like eval, with etc).
Attachment #8358788 - Flags: review?(jdemooij)
Attachment #8358788 - Attachment is obsolete: true
Attachment #8358795 - Flags: review?(jdemooij) → review+
With the above patch, I crashed again on Google Maps.
I don't understand this crash at all. We're executing in the JIT, and have a JSContext in NameIC::attachReadSlot, so we're on the main thread. According to the documentation for compileLock[1], this means we can always read compilation data.

Jandem, can you shed some light on this and tell me how to fix it?


[1]: http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Runtime.h#816
Flags: needinfo?(jdemooij)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #6)
> With the above patch, I crashed again on Google Maps.

Jandem pointed out that, from the gdb output, it very much looks like the patch isn't applied. With the patch, the line number in frame #7 would have to be different, at the very least.

dbaron, can you check whether something went wrong with applying the patch (or updating from m-i, if that's what you did)?
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/b3ccaf008518
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(In reply to Till Schneidereit [:till] from comment #8)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #6)
> > With the above patch, I crashed again on Google Maps.
> 
> Jandem pointed out that, from the gdb output, it very much looks like the
> patch isn't applied. With the patch, the line number in frame #7 would have
> to be different, at the very least.
> 
> dbaron, can you check whether something went wrong with applying the patch
> (or updating from m-i, if that's what you did)?

Ah, I did a one-off import of the patch, but I think I might have then pulled and built again, and forgotten that I'd done that.  I'll update again now that it's on m-c.
Yeah, sorry, seems to be fixed now.
You need to log in before you can comment on or make changes to this bug.