790424 - "Assertion failure: strictModeState != StrictMode::UNKNOWN,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

function p(N = ((function(){})for(u in e)) )t

asserts js debug shell on m-c changeset 1d4fc0c60063 without any CLI argument at Assertion failure: strictModeState != StrictMode::UNKNOWN,

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   101716:84739a192aa9
user:        Benjamin Peterson
date:        Tue Aug 07 13:41:31 2012 -0700
summary:     Bug 780405 - Set the funbox kids in LeaveFunction, so generator expression boxes don't lose them. r=jorendorff

Comment 1

[:Benjamin Peterson]

Attachment: transplant functionList to functionList, not to kids

Comment 2

[:Benjamin Peterson]

Attachment: transplant functionList to functionList, not to kids

Forgot test.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 660267 [details] [diff] [review]
transplant functionList to functionList, not to kids

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

Sorry, I'm not comfortable reviewing this.  jorendorff or jimb, perhaps?

Comment 4

[Nicholas Nethercote [inactive]]

But if you can get rid of FunctionBox::{kids,siblings} and ParseContext::functionList as we discussed on IRC, this bug will become moot!

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Set strict mode on functions in default arguments more lazily. code=benjamin,nnethercote.

This is a cleaned-up version of a patch Benjamin posted on IRC yesterday.
It delays until emitting the setting of strictModeState for functions in
defaults.  I also added a little bit of new explanation and assertion
checking in setStrictMode().

This fixes the original assertion failure, and paves the way for the
subsequent clean-up patch.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove ParseContext::functionList and FunctionBox::{siblings,kids}.

ParseContext::functionList and FunctionBox::{siblings,kids} are now dead and
can be removed.  

jimb, I'm asking you for review because of the comment that I removed above
ParseNodeAllocator::freeTree() -- is removing that entire comment the right
thing to do?  Are there any follow-on simplifications that can happen with
ParseContext::functionList gone?

Also, can CompExprTransplanter be simplified any further?

Comment 7

[:Benjamin Peterson]

Comment on attachment 660656 [details] [diff] [review]
(part 1) - Set strict mode on functions in default arguments more lazily.  code=benjamin,nnethercote.

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

I'm happy to trade mutating the parse tree during compilation to kill siblings/kids/functionList. Do you know if this will confuse Reflect.parse, though?

::: js/src/frontend/Parser.cpp
@@ +1828,5 @@
>              return false;
>          }
>          pc->sc->strictModeState = StrictMode::STRICT;
> +    } else {
> +        if (!pc->parent || pc->parent->sc->strictModeState == StrictMode::NOTSTRICT) {

I don't think this whole block needs to be reindented.

::: js/src/frontend/SharedContext.h
@@ +156,5 @@
>      //
> +    // When parsing is done, no contexts can be in the UNKNOWN state, with the
> +    // exception of functions defined in default expressions.  Their state is
> +    // changed to STRICT/NOTSTRICT when the context starts being used in
> +    // BytecodeEmitter (see EmitFunc()).

State that the rule is that they simply inherit the strictness of their parents.

Comment 8

[Nicholas Nethercote [inactive]]

> I'm happy to trade mutating the parse tree during compilation to kill
> siblings/kids/functionList. Do you know if this will confuse Reflect.parse,
> though?

jsreflect.cpp's only uses of FunctionBox are funbox->fun() and funbox->isGenerator(), so it should be fine.


> @@ +1828,5 @@
> >              return false;
> >          }
> >          pc->sc->strictModeState = StrictMode::STRICT;
> > +    } else {
> > +        if (!pc->parent || pc->parent->sc->strictModeState == StrictMode::NOTSTRICT) {
> 
> I don't think this whole block needs to be reindented.

I did it so that the strict/non-strict cases are obvious.

Comment 9

[Nicholas Nethercote [inactive]]

jimb: 6 day review ping.  It's a small patch that just removes dead code.  C'mon!

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43ab91c9c8e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/65cf1362d58c

Comment 11

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/43ab91c9c8e1

I assume you also meant to say the reviewer was me here. :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43ab91c9c8e1
https://hg.mozilla.org/mozilla-central/rev/65cf1362d58c

Comment 13

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

Test was landed in https://hg.mozilla.org/mozilla-central/rev/43ab91c9c8e1

Comment 14

[Nicholas Nethercote [inactive]]

> I assume you also meant to say the reviewer was me here. :)

Oh, yes.  Sorry!