Closed
Bug 958804
Opened 11 years ago
Closed 11 years ago
Assertion failure: hasScript(), at js/src/jsfun.h:337
Categories
(Core :: JavaScript Engine, defect)
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.
Reporter | ||
Comment 1•11 years ago
|
||
(crashed a third time while restoring my session, too)
Assignee | ||
Comment 2•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: nobody → till
Status: NEW → ASSIGNED
Comment 3•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8358788 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8358795 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Reporter | ||
Comment 6•11 years ago
|
||
With the above patch, I crashed again on Google Maps.
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
(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)
Comment 9•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Reporter | ||
Comment 10•11 years ago
|
||
(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.
Reporter | ||
Comment 11•11 years ago
|
||
Yeah, sorry, seems to be fixed now.
You need to log in
before you can comment on or make changes to this bug.
Description
•