Closed Bug 790424 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla18

People

(Reporter: gkw, Assigned: njn)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(3 files, 2 obsolete files)

Attached file 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
Assignee: general → benjamin
Attachment #660264 - Flags: review?(n.nethercote)
Forgot test.
Attachment #660264 - Attachment is obsolete: true
Attachment #660264 - Flags: review?(n.nethercote)
Attachment #660267 - Flags: review?(n.nethercote)
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?
Attachment #660267 - Flags: review?(n.nethercote)
Attachment #660267 - Flags: review?(jorendorff)
But if you can get rid of FunctionBox::{kids,siblings} and ParseContext::functionList as we discussed on IRC, this bug will become moot!
Attachment #660267 - Attachment is obsolete: true
Attachment #660267 - Flags: review?(jorendorff)
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.
Attachment #660656 - Flags: review?(benjamin)
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?
Attachment #660657 - Flags: review?(jimb)
Assignee: benjamin → n.nethercote
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.
Attachment #660656 - Flags: review?(benjamin) → 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?

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.
jimb: 6 day review ping.  It's a small patch that just removes dead code.  C'mon!
Attachment #660657 - Flags: review?(jimb) → review+
(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. :)
https://hg.mozilla.org/mozilla-central/rev/43ab91c9c8e1
https://hg.mozilla.org/mozilla-central/rev/65cf1362d58c
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Test was landed in https://hg.mozilla.org/mozilla-central/rev/43ab91c9c8e1
Status: RESOLVED → VERIFIED
Flags: in-testsuite- → in-testsuite+
> I assume you also meant to say the reviewer was me here. :)

Oh, yes.  Sorry!
You need to log in before you can comment on or make changes to this bug.