Closed Bug 1300521 Opened 3 years ago Closed 3 years ago

Assertion failure: shape->writable(), at js/src/vm/Interpreter.cpp:4400

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- fixed

People

(Reporter: decoder, Unassigned)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 401ea746b1a9 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

var NaN;
function NaN()
  function isFinite(x) {}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0000000000ae58c1 in js::DefFunOperation (cx=0x7ffff695f000, script=..., envChain=..., funArg=...) at js/src/vm/Interpreter.cpp:4400
#0  0x0000000000ae58c1 in js::DefFunOperation (cx=0x7ffff695f000, script=..., envChain=..., funArg=...) at js/src/vm/Interpreter.cpp:4400
#1  0x0000000000ad6bed in Interpret (cx=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:3444
[..]
#11 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7659
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/701075b5e63c
user:        Shu-yu Guo
date:        Fri Sep 02 15:30:48 2016 -0700
summary:     Bug 1298640 - Track top-level functions in eval/global bodies for all-or-nothing redeclaration checks. (r=Waldo)

This iteration took 247.614 seconds to run.
NI shu per comment 1.
Flags: needinfo?(shu)
Simpler testcase:

  var NaN; function NaN() {}

It's an almost-fix for Parser::tryDeclareVar to upgrade var declarations into body-level function declarations when it sees them, something like adding this to the if-declared consequent-statement:

    if (declaredKind == DeclarationKind::Var) {
        if (kind == DeclarationKind::BodyLevelFunction)
            p->value()->alterKind(kind);
    }

The problem is what a "var declaration" is.  |declaredKind == DeclarationKind::Var| covers most cases, but we need to handle the for-of case as well.  Modify to a for-of var, and we're back to square one:

js> for (var NaN of []); function NaN() {}
Assertion failure: shape->writable(), at /home/jwalden/moz/slots/js/src/vm/Interpreter.cpp:4400

Oh, hey, and Annex B vars too!

js> { function NaN() {} } function NaN() {}
Assertion failure: shape->writable(), at /home/jwalden/moz/slots/js/src/vm/Interpreter.cpp:4400

I think we're at the point where we need to change name-maps to map from name to something other than DeclarationKind directly, say, struct { DeclarationKind : 4; bool normalVar : 1; bool forOfVar : 1; bool bodyLevelFunction : 1; bool annexBVar: 1; }.  Or something like that.  ~~jazzhands~~  The pile of special-var cases grows, and it seems mostly impossible to keep them exclusive of each other, given these testcases.  Seem reasonable, shu?

Feh.
This language is a joke.
Flags: needinfo?(shu)
Comment on attachment 8788651 [details] [diff] [review]
Fix CanDeclareGlobalFunction checks for functions redeclaring vars in the same script.

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

::: js/src/frontend/Parser.cpp
@@ +1049,5 @@
> +                // Function [4]. CanDeclareGlobalFunction is strictly more
> +                // restrictive than CanDeclareGlobalVar, so record the more
> +                // restrictive kind. These semantics are implemented in
> +                // CheckCanDeclareGlobalBinding.
> +                //

I'd like to see a comment that the declaration type stored here is tested *only* for BodyLevelFunction, and that overwriting, say, ForOfVar with BodyLevelFunction won't discard information we need later.  Somehow or other.

It'd also be nice if this worldwide semantic were documented in some central location somewhere, but I'm not totally sure where that would be.

::: js/src/tests/ecma_6/LexicalEnvironment/nondefinable-function-same-script.js
@@ +1,4 @@
> +// |reftest| skip-if(!xulRuntime.shell)
> +
> +assertThrowsInstanceOf(() => evaluate(`var NaN; function NaN() {}`), TypeError);
> +assertThrowsInstanceOf(() => (1,eval)(`var NaN; function NaN() {}`), TypeError);

Add versions of this that hit the other var-types -- for-of vars, Annex B vars, &c.
Attachment #8788651 - Flags: review?(jwalden+bmo) → review+
Duplicate of this bug: 1301193
Should also add bug 1301193-style tests as well, to hit that separate assertion-case.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/99ab1f77deb7
Fix CanDeclareGlobalFunction checks for functions redeclaring vars in the same script. (r=Waldo)
https://hg.mozilla.org/mozilla-central/rev/99ab1f77deb7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.