769072 - Assertion failure: !f.script()->strictModeCode, at methodjit/StubCalls.cpp:1347

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test asserts on mozilla-central revision be373f4b243f (options -m -n -a):


function f(g = delete Function)  {
    "use strict";
}
f();

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   95044:699a613bf616
user:        Benjamin Peterson
date:        Sat May 26 09:33:53 2012 -0400
summary:     Bug 757676 - Implement JS default parameters. r=jorendorff

Comment 2

[:Benjamin Peterson]

The problem is we don't detect strict mode until we get to the body, so we don't ever complain about violations in the defaults. The (not very appetizing options) are:

1. Queue strict mode errors while parsing argument lists. Then complain if it turns out strict mode was enabled.
2. Write some sort of lookahead into the function body for the "use strict";.

Comment 3

[:Benjamin Peterson]

Attachment: retroactively apply strict mode

Here's a patch implementing option 2. It queues a strict error until we can determine whether or not a function will be in strictmode. It refactors directive prologue parsing to use the tokenizer directly. I had to increase the tokenizer lookahead to do this. This also subsumes the current octal literal hack.

Comment 4

[:Benjamin Peterson]

(In reply to Benjamin Peterson from comment #3)
> Created attachment 637950 [details] [diff] [review]
> retroactively apply strict mode
> 
> Here's a patch implementing option 2.

Sorry, actually option 1.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 637950 [details] [diff] [review]
retroactively apply strict mode

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

Not very appetizing, indeed.

I haven't done a thorough review of this whole patch.  What I've seen so far is enough to set my spidey-sense tinging, and not in a good way :/  So I've given an f- instead of an r-.  I've marked a couple of things below that worry me.  Let's discuss!

BTW, are you certain that applying strict mode retroactively is the correct behaviour?  http://wiki.ecmascript.org/doku.php?id=harmony:parameter_default_values doesn't mention strict mode.  Implementing it is such a pain that I wonder if it's worth pushing for the language to be changed (if, indeed, the expected behaviour is even specified clearly).

::: js/src/frontend/BytecodeCompiler.cpp
@@ +128,3 @@
>          sc.setInStrictMode();
> +        tc.strictModeState = StrictMode::STRICT;
> +    }

We used to store the strict flag in both TokenStream and SharedContext.  In bug 754181 I removed the one in TokenStream.  And now you're adding a second strictness value to TreeContext, which is is closely related to TreeContext.  This doesn't feel right at all.

If you really need the three-valued STRICT/UNKNOWN/NOTSTRICT state, can that go in SharedContext and replace the boolean currently in cxFlags?

::: js/src/frontend/Parser.cpp
@@ +1801,5 @@
> +        }
> +        tc->strictModeState = StrictMode::STRICT;
> +
> +        // Retroactively recursively set strict mode on all the current children
> +        // (Basically functions in defaults).

Can you give an example of this?  Are you saying that if you have this:

  function f(x = (function(){ return 3; })) {
      "use strict";
  }

that the function in the default position inherits the strict mode?

::: js/src/frontend/TreeContext.h
@@ +206,5 @@
>  
>      // Return true if we need to check for conditions that elicit
> +    // JSOPTION_STRICT warnings or strict mode errors. If you are parsing, you
> +    // must pass a Parser. In the bytecode emitter, pass NULL.
> +    inline bool needStrictChecks(Parser *parser);

Why the two modes of behaviour?  BytecodeEmitter has a |parser| field, so it could pass it in, although I've been trying my hardest to remove the need for it.  This also feels wrong.

Comment 6

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> BTW, are you certain that applying strict mode retroactively is the correct
> behaviour? 
> http://wiki.ecmascript.org/doku.php?id=harmony:parameter_default_values
> doesn't mention strict mode.  Implementing it is such a pain that I wonder
> if it's worth pushing for the language to be changed (if, indeed, the
> expected behaviour is even specified clearly).

I discussed this a bit with dherman. I think it's the only reasonable interpretation if we take
function f(a=(function h(x) 42)) {}
to be roughtly
function f(a) {
    a = function h(x) 42; // if a was not passed
}

Jeff Walden told me webkit solves this by reparsing from the beginning of the function when "use strict" is discovered.

> 
> ::: js/src/frontend/BytecodeCompiler.cpp
> @@ +128,3 @@
> >          sc.setInStrictMode();
> > +        tc.strictModeState = StrictMode::STRICT;
> > +    }
> 
> We used to store the strict flag in both TokenStream and SharedContext.  In
> bug 754181 I removed the one in TokenStream.  And now you're adding a second
> strictness value to TreeContext, which is is closely related to TreeContext.
> This doesn't feel right at all.
> 
> If you really need the three-valued STRICT/UNKNOWN/NOTSTRICT state, can that
> go in SharedContext and replace the boolean currently in cxFlags?

What I would really like is for the BytecodeEmitter to only use SharedContext.inStrictMode() and for the parser to only use strictModeState. However, there's no BytecodeEmitter-specific structure like TreeContext for the parser.

> 
> ::: js/src/frontend/Parser.cpp
> @@ +1801,5 @@
> > +        }
> > +        tc->strictModeState = StrictMode::STRICT;
> > +
> > +        // Retroactively recursively set strict mode on all the current children
> > +        // (Basically functions in defaults).
> 
> Can you give an example of this?  Are you saying that if you have this:
> 
>   function f(x = (function(){ return 3; })) {
>       "use strict";
>   }
> 
> that the function in the default position inherits the strict mode?

Exactly.

> 
> ::: js/src/frontend/TreeContext.h
> @@ +206,5 @@
> >  
> >      // Return true if we need to check for conditions that elicit
> > +    // JSOPTION_STRICT warnings or strict mode errors. If you are parsing, you
> > +    // must pass a Parser. In the bytecode emitter, pass NULL.
> > +    inline bool needStrictChecks(Parser *parser);
> 
> Why the two modes of behaviour?  BytecodeEmitter has a |parser| field, so it
> could pass it in, although I've been trying my hardest to remove the need
> for it.  This also feels wrong.

This is related to the strictModeState vs. inStrictMode() mess you noted above. Since we are done parsing while emitting bytecode, strict mode is always a binary state there. During parsing, we need to do strict mode checks both when in strict mode and when strict mode is not known yet. Hence, needStrictChecks() takes a Parser argument to get at the TreeContext. Removing the parser attribute on BytecodeEmitter would actually help enforce this distinction.

Comment 7

[Nicholas Nethercote [inactive]]

> > If you really need the three-valued STRICT/UNKNOWN/NOTSTRICT state, can that
> > go in SharedContext and replace the boolean currently in cxFlags?
> 
> What I would really like is for the BytecodeEmitter to only use
> SharedContext.inStrictMode() and for the parser to only use strictModeState.
> However, there's no BytecodeEmitter-specific structure like TreeContext for
> the parser.

BytecodeEmitter is the equivalent of TreeContext -- there's one per level of script nesting.  On the bytecode side, there's no equivalent to Parser, i.e. a single object holding bytecode-wide info.  I have plans to rename BytecodeEmitter as BytecodeContext, and then create a new class BytecodeEmitter, of which there would be a single instance.  And also to rename TreeContext as ParseContext.  But I haven't got there yet.

> Since we are done parsing while emitting bytecode, strict mode is
> always a binary state there. During parsing, we need to do strict mode
> checks both when in strict mode and when strict mode is not known yet.
> Hence, needStrictChecks() takes a Parser argument to get at the TreeContext.
> Removing the parser attribute on BytecodeEmitter would actually help enforce
> this distinction.

Again: can you put the ternary state in SharedContext?  When accessed from BytecodeEmitter it would never return UNKNOWN, but that would be fine.  (You could assert this is true if you wanted.)  I suspect that would avoid the duplication and this odd Parser argument.

Comment 8

[:Benjamin Peterson]

Attachment: retroactively apply strict mode to defaults

I killed the strictMode context flag in favor of using SharedContext::strictModeState everywhere. (This is much nicer; thanks for the idea.) There's still duplication between SharedContext and FunctionBox but that's unavoidable with the current parser/emitter architecture.

Comment 9

[Nicholas Nethercote [inactive]]

> There's still duplication between SharedContext and FunctionBox
> but that's unavoidable with the current parser/emitter architecture.

That duplication is much less obnoxious because the copies aren't live at the same time -- SharedContext effectively passes ownership of the flag to FunctionBox, which then passes it onto another SharedContext.

Comment 10

[:Benjamin Peterson]

Attachment: retroactively apply strict mode to defaults

A few tweaks on the last patch.

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 639207 [details] [diff] [review]
retroactively apply strict mode to defaults

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

I've reviewed all of this except TokenStream.cpp and Parser.cpp, the bits I've looked at so far are good.

In TokenStream.cpp I think you've essentially done two changes -- refactored reportCompileErrorNumberVA() to introduce the CompileError type, and then the strict mode changes.  Would it be possible to do the refactoring as a preliminary patch?  It's difficult to review that file's changes because so much code is moving around.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4738,5 @@
>          sc.cxFlags = funbox->cxFlags;
>          if (bce->sc->funMightAliasLocals())
>              sc.setFunMightAliasLocals();  // inherit funMightAliasLocals from parent
>          sc.bindings.transfer(&funbox->bindings);
> +        JS_ASSERT_IF(bce->sc->inStrictMode(), sc.inStrictMode());

LOL, that wasn't adding much value.

::: js/src/frontend/Parser.cpp
@@ +534,5 @@
>              if (!ReportBadParameter(cx, parser, name, JSMSG_BAD_BINDING))
>                  return false;
>          }
>  
> +        if (FindKeyword(name->charsZ(), name->length())) {

Why did you remove the sc->inStrictMode() test here?

@@ +540,5 @@
>               * JSOPTION_STRICT is supposed to warn about future keywords, too,
>               * but we took care of that in the scanner.
>               */
> +            if (!ReportBadParameter(cx, parser, name, JSMSG_RESERVED_ID))
> +                return false;

Removing the JS_ALWAYS_TRUE is good -- it doesn't make sense because we could OOM within ReportBadParameter.  But notice that you've changed the return behaviour -- the old code always returned false here, the new code will fall through unless ReportBadParameter() failed.

@@ +596,5 @@
>          JS_ASSERT(JS_HAS_EXPR_CLOSURES);
> +
> +        // There are no directives to parse, so set strict mode if that's needed.
> +        if (!setStrictMode(false))
> +            return NULL;

The phrase "set strict mode" makes me think you're setting it to true.  Reword?  Also, why is the "if that's needed" there?

@@ -1544,5 @@
>      if (!funtc.init())
>          return NULL;
>  
> -    if (outertc->sc->inStrictMode())
> -        funsc.setInStrictMode();    // inherit strict mode from parent

Nice;  setting this via the constructor is much better.

::: js/src/frontend/Parser.h
@@ -229,5 @@
>      ParseNode *identifierName(bool afterDoubleDot);
>  
>  #if JS_HAS_XML_SUPPORT
>      // True if E4X syntax is allowed in the current syntactic context.
> -    bool allowsXML() const { return !tc->sc->inStrictMode() && tokenStream.allowsXML(); }

Why is the inStrictMode() call removed here?

::: js/src/frontend/TokenStream.h
@@ +429,5 @@
> +    void throwError();
> +};
> +
> +
> +namespace StrictMode {

A whole namespace for this is overkill.  jorendorff is using the |frontend| namespace more consistently in bug 770854, that's good enough for this type.

@@ +450,5 @@
>  // TokenStream needs to see it.
>  //
>  // This class constitutes a tiny back-channel from TokenStream to the strict
>  // mode flag that avoids exposing the rest of SharedContext to TokenStream.  
>  // get() is implemented in Parser.cpp.

Please update this comment to mention the queued error as well as the flag.

@@ +610,5 @@
>      }
>  
>      TokenKind peekToken() {
>          if (lookahead != 0) {
> +            JS_ASSERT(lookahead <= 2);

Can you explain why this is necessary?  I don't understand it.

::: js/src/frontend/TreeContext-inl.h
@@ -84,5 @@
>  }
>  
> -// For functions the tree context is constructed and destructed a second
> -// time during code generation. To avoid a redundant stats update in such
> -// cases, we store UINT16_MAX in maxScopeDepth.

Good -- I've been meaning to remove that comment for a while :)

@@ +111,5 @@
> +    if (queuedStrictModeError) {
> +        if (parent && parent->sc->strictModeState == StrictMode::UNKNOWN &&
> +            !parent->queuedStrictModeError)
> +            parent->queuedStrictModeError = queuedStrictModeError;
> +        else

A brief comment explaining this would be helpful.

::: js/src/frontend/TreeContext.h
@@ +140,5 @@
> +    // strictModeState tracks the strictness of this context. Normally, it
> +    // should be STRICT or NOTSTRICT. However, it can be UNKNOWN when parsing
> +    // code for which the strictness has not yet been determined. This happens
> +    // when parsing function defaults and non-"use strict" directive prologue
> +    // strings.

The last sentence is difficult to parse, partly because my brain wants to intepret "defaults" as a verb.  Maybe "a function's defaults".

What other kind of directive prologue strings are there?  I thought "use strict" was the only one.

@@ +229,5 @@
>                                         be an error if we turn out to be inside a
>                                         generator expression */
>      FunctionBox     *functionList;
>  
> +    CompileError    *queuedStrictModeError;

You have such a nice comment on |strictModeState| above, it'd be to have one for this too.

Comment 12

[:Benjamin Peterson]

Attachment: increase tokenizer lookahead

Comment 13

[:Benjamin Peterson]

Attachment: create a CompileError class

Comment 14

[:Benjamin Peterson]

Attachment: retroactively apply strict mode to defaults

Comment 15

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #11)
> Comment on attachment 639207 [details] [diff] [review]
> retroactively apply strict mode to defaults
> 
> Review of attachment 639207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've reviewed all of this except TokenStream.cpp and Parser.cpp, the bits
> I've looked at so far are good.
> 
> In TokenStream.cpp I think you've essentially done two changes -- refactored
> reportCompileErrorNumberVA() to introduce the CompileError type, and then
> the strict mode changes.  Would it be possible to do the refactoring as a
> preliminary patch?  It's difficult to review that file's changes because so
> much code is moving around.

Will do.

> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +4738,5 @@
> >          sc.cxFlags = funbox->cxFlags;
> >          if (bce->sc->funMightAliasLocals())
> >              sc.setFunMightAliasLocals();  // inherit funMightAliasLocals from parent
> >          sc.bindings.transfer(&funbox->bindings);
> > +        JS_ASSERT_IF(bce->sc->inStrictMode(), sc.inStrictMode());
> 
> LOL, that wasn't adding much value.

If you look closely, you'll notice that bce->sc is the parent's context.

> 
> ::: js/src/frontend/Parser.cpp
> @@ +534,5 @ t
> >              if (!ReportBadParameter(cx, parser, name, JSMSG_BAD_BINDING))
> >                  return false;
> >          }
> >  
> > +        if (FindKeyword(name->charsZ(), name->length())) {
> 
> Why did you remove the sc->inStrictMode() test here?

I'll change it to a sc->needStrictChecks() to avoid the keyword lookup when possible.

> 
> @@ +540,5 @@
> >               * JSOPTION_STRICT is supposed to warn about future keywords, too,
> >               * but we took care of that in the scanner.
> >               */
> > +            if (!ReportBadParameter(cx, parser, name, JSMSG_RESERVED_ID))
> > +                return false;
> 
> Removing the JS_ALWAYS_TRUE is good -- it doesn't make sense because we
> could OOM within ReportBadParameter.  But notice that you've changed the
> return behaviour -- the old code always returned false here, the new code
> will fall through unless ReportBadParameter() failed.

Strict errors need to go through the tokenizer error reporter whenever the strictModeState is not NOTSTRICT. The tokenizer error handler can return true if it's just queueing the error.

> ::: js/src/frontend/Parser.h
> @@ -229,5 @@
> >      ParseNode *identifierName(bool afterDoubleDot);
> >  
> >  #if JS_HAS_XML_SUPPORT
> >      // True if E4X syntax is allowed in the current syntactic context.
> > -    bool allowsXML() const { return !tc->sc->inStrictMode() && tokenStream.allowsXML(); }
> 
> Why is the inStrictMode() call removed here?

TokenStream::allowsXML() checks it.

> 
> ::: js/src/frontend/TokenStream.h
> @@ +429,5 @@
> > +    void throwError();
> > +};
> > +
> > +
> > +namespace StrictMode {
> 
> A whole namespace for this is overkill.  jorendorff is using the |frontend|
> namespace more consistently in bug 770854, that's good enough for this type.

I was trying to namespace the enum members, but I suppose it doesn't matter terribly since they're always put into a member called strictModeState on SharedContext.

> 
> @@ +450,5 @@
> >  // TokenStream needs to see it.
> >  //
> >  // This class constitutes a tiny back-channel from TokenStream to the strict
> >  // mode flag that avoids exposing the rest of SharedContext to TokenStream.  
> >  // get() is implemented in Parser.cpp.
> 
> Please update this comment to mention the queued error as well as the flag.
> 
> @@ +610,5 @@
> >      }
> >  
> >      TokenKind peekToken() {
> >          if (lookahead != 0) {
> > +            JS_ASSERT(lookahead <= 2);
> 
> Can you explain why this is necessary?  I don't understand it.

I had to increase the lookahead for the Parser::processDirectives().

> 
> ::: js/src/frontend/TreeContext.h
> @@ +140,5 @@
> > +    // strictModeState tracks the strictness of this context. Normally, it
> > +    // should be STRICT or NOTSTRICT. However, it can be UNKNOWN when parsing
> > +    // code for which the strictness has not yet been determined. This happens
> > +    // when parsing function defaults and non-"use strict" directive prologue
> > +    // strings.
> 
> The last sentence is difficult to parse, partly because my brain wants to
> intepret "defaults" as a verb.  Maybe "a function's defaults".
> 
> What other kind of directive prologue strings are there?  I thought "use
> strict" was the only one.

There are no other official ones, but there's the idea of a "directive prologue" which can contain any number of compiler directives. For example, the following function would be strict mode:

function f() {
    "blah, blah, blah, blah";
    "funky bunnies";
    "use strict";
}

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 639578 [details] [diff] [review]
create a CompileError class

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

Much easier to read -- thanks!

::: js/src/frontend/TokenStream.cpp
@@ +442,5 @@
> +void
> +CompileError::clear()
> +{
> +    if (report.uclinebuf)
> +        cx->free_((void*)report.uclinebuf);

You can pass NULL to free_(), but I'm not much fussed either way.

@@ +461,5 @@
> +    PodZero(&report);
> +
> +    if (message)
> +        cx->free_(message);
> +    message = NULL;

Move this up next to the freeing of the other buffers?

@@ +480,5 @@
>          warning = false;
>      }
>  
> +    CompileError normalError(cx);
> +    CompileError *err = &normalError;

Why not rename |normalError| as |err| and then write |err.foo| throughout instead of |err->foo|?

::: js/src/frontend/TokenStream.h
@@ +421,5 @@
> +    CompileError(JSContext *cx)
> +     : cx(cx), message(NULL), hasCharArgs(false)
> +    {
> +        PodZero(&report);
> +        clear();

Why bother calling clear() here?  It won't do anything because everything is zeroed.  And after removing this call, you'll be able to inline clear() into the destructor.

Comment 17

[Nicholas Nethercote [inactive]]

> > +    CompileError normalError(cx);
> > +    CompileError *err = &normalError;
> 
> Why not rename |normalError| as |err| and then write |err.foo| throughout
> instead of |err->foo|?

Never mind, I see this is useful for the following patch.

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 639579 [details] [diff] [review]
retroactively apply strict mode to defaults

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

I've looked at TokenStream.cpp.  Unfortunately it's 5pm on Friday here and I won't have a chance to look at Parser.cpp until Monday morning.

::: js/src/frontend/TokenStream.cpp
@@ +395,5 @@
>  bool
>  TokenStream::reportStrictModeErrorNumberVA(ParseNode *pn, unsigned errorNumber, va_list args)
>  {
>      /* In strict mode code, this is an error, not merely a warning. */
> +    unsigned flags = JSREPORT_STRICT;

I don't understand how this change preserves existing behaviour.  Or was the old code wrong?  It's certainly odd that the old code didn't set the STRICT flag.  How did it ever work?

@@ +471,5 @@
>  {
>      bool strict = JSREPORT_IS_STRICT(flags);
>      bool warning = JSREPORT_IS_WARNING(flags);
>  
> +    if (strict && warning && !cx->hasStrictOption())

Same here -- the behaviour has changed, AFAICT, but the new behaviour seems more correct...

@@ +476,3 @@
>          return true;
>  
> +    if (warning && errorNumber != JSMSG_STRICT_CODE_WITH && cx->hasWErrorOption()) {

The JSMSG_STRICT_CODE_WITH is mysterious.  I guess the comment in withStatement() explains it?  Please add a comment pointing to that.

Hmm, if errorNumber==JSMSG_STRICT_CODE_WITH, won't |warning| always be false?  Maybe you could assert that and remove JSMSG_STRICT_CODE_WITH from the condition?

@@ +483,5 @@
>      CompileError normalError(cx);
>      CompileError *err = &normalError;
> +    if (strict && !warning && strictModeState() == StrictMode::UNKNOWN) {
> +        if (strictModeGetter->queuedStrictModeError()) {
> +            if (cx->hasStrictOption() && errorNumber != JSMSG_STRICT_CODE_WITH) {

Again, won't |errorNumber != JSMSG_STRICT_CODE_WITH| always be true here?

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 639579 [details] [diff] [review]
retroactively apply strict mode to defaults

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

::: js/src/frontend/Parser.cpp
@@ +724,5 @@
>           * to make the arguments object reflect initial parameter values prior
>           * to any mutation we create it eagerly whenever parameters are (or
>           * might, in the case of calls to eval) be assigned.
>           */
> +        if (tc->sc->strictModeState != StrictMode::NOTSTRICT) {

Is it worth adding a helper function sc->mightBeStrict()?  I find the double negative a little hard to read... not sure, you decide.

@@ +1553,5 @@
>      RootedFunction fun(context, newFunction(outertc, funName, kind));
>      if (!fun)
>          return NULL;
>  
>      /* Create box for fun->object early to protect against last-ditch GC. */

Can you add the "inherit strict mode from parent" comment back in here?

@@ +1784,5 @@
> +            kid->recursivelySetStrictMode(strictness);
> +    }
> +}
> +
> +bool

A comment explaining the meaning of the return value would be very helpful here.  More comments in the body would also be helpful;  the level of checking is great but it's a bit mesmerizing without comments.

Comment 20

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #19)
> Comment on attachment 639579 [details] [diff] [review]
> retroactively apply strict mode to defaults
> 
> Review of attachment 639579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/Parser.cpp
> @@ +724,5 @@
> >           * to make the arguments object reflect initial parameter values prior
> >           * to any mutation we create it eagerly whenever parameters are (or
> >           * might, in the case of calls to eval) be assigned.
> >           */
> > +        if (tc->sc->strictModeState != StrictMode::NOTSTRICT) {
> 
> Is it worth adding a helper function sc->mightBeStrict()?  I find the double
> negative a little hard to read... not sure, you decide.

I've switched it to SharedContext::needStrictChecks().

Comment 21

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #18)
> Comment on attachment 639579 [details] [diff] [review]
> retroactively apply strict mode to defaults
> 
> Review of attachment 639579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've looked at TokenStream.cpp.  Unfortunately it's 5pm on Friday here and I
> won't have a chance to look at Parser.cpp until Monday morning.
> 
> ::: js/src/frontend/TokenStream.cpp
> @@ +395,5 @@
> >  bool
> >  TokenStream::reportStrictModeErrorNumberVA(ParseNode *pn, unsigned errorNumber, va_list args)
> >  {
> >      /* In strict mode code, this is an error, not merely a warning. */
> > +    unsigned flags = JSREPORT_STRICT;
> 
> I don't understand how this change preserves existing behaviour.  Or was the
> old code wrong?  It's certainly odd that the old code didn't set the STRICT
> flag.  How did it ever work?

I don't think JSREPORT_STRICT changes any behavior in error reporting code, so it probably doesn't make a difference.

> 
> @@ +476,3 @@
> >          return true;
> >  
> > +    if (warning && errorNumber != JSMSG_STRICT_CODE_WITH && cx->hasWErrorOption()) {
> 
> The JSMSG_STRICT_CODE_WITH is mysterious.  I guess the comment in
> withStatement() explains it?  Please add a comment pointing to that.
> 
> Hmm, if errorNumber==JSMSG_STRICT_CODE_WITH, won't |warning| always be
> false?  Maybe you could assert that and remove JSMSG_STRICT_CODE_WITH from
> the condition?

Parser::withStatement() always reports a strict mode error, so its up to this code to reject it.

> 
> @@ +483,5 @@
> >      CompileError normalError(cx);
> >      CompileError *err = &normalError;
> > +    if (strict && !warning && strictModeState() == StrictMode::UNKNOWN) {
> > +        if (strictModeGetter->queuedStrictModeError()) {
> > +            if (cx->hasStrictOption() && errorNumber != JSMSG_STRICT_CODE_WITH) {
> 
> Again, won't |errorNumber != JSMSG_STRICT_CODE_WITH| always be true here?

No, it can arrive as stated above.

Comment 22

[:Benjamin Peterson]

Attachment: part 1: create a CompileError class

Comment 23

[:Benjamin Peterson]

Attachment: part 2: increase tokenizer lookahead

Comment 24

[:Benjamin Peterson]

Attachment: part 3: retroactively apply strict mode to defaults

Comment 25

[:Benjamin Peterson]

Attachment: part 3: retroactively apply strict mode to defaults

Comment 26

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50ab0f0736c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6332579d7623
https://hg.mozilla.org/integration/mozilla-inbound/rev/418ad69cfe52

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6332579d7623
https://hg.mozilla.org/mozilla-central/rev/418ad69cfe52

Comment 28

[:Benjamin Peterson]

Also:

https://hg.mozilla.org/mozilla-central/rev/50ab0f0736c8

Comment 29

[Justin Wood (:Callek)]

This bug is my current suspect for a SeaMonkey trunk crash-on-startup (Windows): (e.g. http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1341965780.1341967097.28879.gz&fulltext=1#err0 )

Assertion failure: length != 0, at e:/builds/slave/comm-cen-trunk-w32-dbg/build/mozilla/js/src/frontend/TokenStream.cpp:63

Followed by a crash.

We do set javascript.options.strict, true, (which should be different than what you touch here) and Firefox has javascript.options.strict, false.

Any chance we can revert this for a little bit and see if it is the cause and/or you see something obviously wrong here based on the comments.

More current builds listed at http://tbpl.drapostles.org

Comment 30

[Justin Wood (:Callek)]

I'm running through try a patch that toggles that pref for m-c-based applications, as a test of that theory.

https://tbpl.mozilla.org/?tree=Try&rev=a406aaac051b (I set --post-to-bugzilla, but I forget if that is broken or not)

Comment 31

[:Benjamin Peterson]

Let's track this in bug 772751.