1122833 - Assertion failure: !isInterpretedLazy(), at jsfun.h or Assertion failure: hasScript(), at jsfun.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

// jsfunfuzz-generated code
s = newGlobal();
// Randomly chosen test: js/src/jit-test/tests/auto-regress/bug490191.js
evalcx("\
    (function() {\
        eval(\" \
            (function(){ \
                __defineGetter__(\\\"y\\\", function(){}) \
            }); \
        \")();\
    })()\
", s);
gc()
gc()
evalcx("\
    setJitCompilerOption('baseline.warmup.trigger', 1);\
    y;\
", s)

asserts js debug shell on m-c changeset 5438e3f74848 with --fuzzing-safe --no-threads --ion-eager --no-fpu at Assertion failure: !isInterpretedLazy(), at jsfun.h. During reduction, I also saw this: Assertion failure: hasScript(), at jsfun.h

Debug configure options:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

This was found by combining random js tests together with jsfunfuzz, the specific file(s) is/are:

http://hg.mozilla.org/mozilla-central/file/5438e3f74848/js/src/jit-test/tests/auto-regress/bug490191.js

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/b14f46d65f73
user:        Shu-yu Guo
date:        Wed Jan 14 15:18:42 2015 -0800
summary:     Bug 963879 - Part 1: Overhaul ScopeIter and StaticScopeIter to share iteration logic and to go through evals. (r=luke)

Setting s-s and assuming sec-critical because this seems to involve gc. Shu-yu, is bug 963879 a likely regressor?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x51dbe, 0x0002672f js-dbg-opt-32-dm-nsprBuild-darwin-5438e3f74848`JSFunction::isHeavyweight(this=<unavailable>) const + 207 at jsfun.h:94, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0002672f js-dbg-opt-32-dm-nsprBuild-darwin-5438e3f74848`JSFunction::isHeavyweight(this=<unavailable>) const + 207 at jsfun.h:94
    frame #1: 0x0082c0e8 js-dbg-opt-32-dm-nsprBuild-darwin-5438e3f74848`js::StaticScopeIter<(this=<unavailable>)0>::hasDynamicScopeObject() const + 104 at ScopeObject-inl.h:106
    frame #2: 0x007fd878 js-dbg-opt-32-dm-nsprBuild-darwin-5438e3f74848`AssertDynamicScopeMatchesStaticScope(cx=<unavailable>, script=<unavailable>, scope=<unavailable>) + 216 at Stack.cpp:147
    frame #3: 0x007fd3ba js-dbg-opt-32-dm-nsprBuild-darwin-5438e3f74848`js::InterpreterFrame::prologue(this=0x02856e10, cx=0x01d67b80) + 442 at Stack.cpp:212
    frame #4: 0x0077577b js-dbg-opt-32-dm-nsprBuild-darwin-5438e3f74848`Interpret(cx=<unavailable>, state=<unavailable>) + 971 at Interpreter.cpp:1508
(lldb)

Comment 3

[Shu-yu Guo [:shu]]

This is a relazification crash.

I'm not sure why my patch surfaced this, since the StaticScopeIter worked basically the same way before. The problem is that whether or not a JSFunction is heavyweight can only be checked on delazified JSFunctions. So SSI has to possibly delazify functions to check hasDynamicScopeChain. But there are several places that use StaticScopeIter<NoGC>, so I'm not sure how to fix...

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Bandaid to not relazify heavyweight functions.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Don't relazify scripts with direct eval.

Explanation of bug in the comment in the patch.

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 8550672 [details] [diff] [review]
Don't relazify scripts with direct eval.

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

Great catch! I wonder how this worked up until now ... It might well be the source of (some of) our various lazy script-related crashes.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Does the underlying defect affect older branches too?

Comment 9

[Till Schneidereit [:till]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> Does the underlying defect affect older branches too?

Hmm, yes it does.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Please don't use being a security bug that affects older branches as a reason not to land either this, some other workaround, or a backout on inbound/central -- debug builds are basically unusable for real web browsing right now, given that they crash on most google properties, new york times articles, etc. (bug 1122364).  That's an unacceptable state to leave central builds in.

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Till Schneidereit [:till] from comment #9)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> > Does the underlying defect affect older branches too?
> 
> Hmm, yes it does.

No, it doesn't affect older branches. Prior to my patch, functions with direct evals wouldn't be on their inner functions-by-way-of-eval's static scope chains.

Comment 12

[Till Schneidereit [:till]]

(In reply to Shu-yu Guo [:shu] from comment #11)
> No, it doesn't affect older branches. Prior to my patch, functions with
> direct evals wouldn't be on their inner functions-by-way-of-eval's static
> scope chains.

Oh, I see. There goes my hope regarding the other crashes, then.

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f0ab64c88102

Comment 14

[Luke Wagner [:luke]]

Is there any followup bug to land a test case and/or make isHeavyweight less of a relazification hazard (either renaming it to include the not-relazified assumption or, as discussed on irc on Friday, moving the heavyweight bit to JSFunction::flags or LazyScript)?  I didn't notice one, and it seems like we should do that given the super-tenuous current situation.

Comment 15

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #14)
> Is there any followup bug to land a test case and/or make isHeavyweight less
> of a relazification hazard (either renaming it to include the not-relazified
> assumption or, as discussed on irc on Friday, moving the heavyweight bit to
> JSFunction::flags or LazyScript)?  I didn't notice one, and it seems like we
> should do that given the super-tenuous current situation.

Not yet. The tricky thing is that even if we move it off the JSScript, it's not known until the first delazification. So really the bit still intrinsically lives on JSScript, but is cached elsewhere after it's been computed. I don't really feel great about that scheme, it doesn't fundamentally make the situation cleaner.

Comment 16

[Luke Wagner [:luke]]

(In reply to Shu-yu Guo [:shu] from comment #15)
> I don't really feel great about that scheme, it doesn't
> fundamentally make the situation cleaner.

I think what you're saying is that, even with this change, one could not access isHeavyweight in all states.  That makes sense.  I was thinking, though: isHeavyweight is defined in SharedContext.h to be the disjunction of a few simple conditions, all of which it seems like we can easily/cheaply determine during the syntax-only parse.  So on first glance it seems like we could determine isHeavyweight eagerly.

Comment 17

[Jan de Mooij [:jandem]]

FWIW I ran into a similar issue in bug 1123120 with the new relazifyFunctions() shell function and -D. There we have js_Disassemble1 -> ScopeCoordinateName -> ScopeCoordinateToStaticScopeShape -> StaticScopeIter -> isHeavyWeight.

The easiest fix there is to not relazify scripts with pc counts, but I agree it'd be nice if we could fix isHeavyweight.