514585 - ES5 strict mode: recognize "use strict" directives in Directive Prologue

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

The parser should recognize ES5 Directive Prologues when they appear at the top of global code, eval code, or a function body, recognize a Use Strict Directive within such a prologue, and set an appropriate flag in the JSTreeContext structure that enables the errors required for strict mode.

Comment 1

[Jim Blandy :jimb]

Attachment: Use bitfields in JSScript instead of a flag word.

This is behavior-neutral cleanup in preparation for the real patch.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 401941 [details] [diff] [review]
Use bitfields in JSScript instead of a flag word.

>@@ -506,7 +506,9 @@ WrapEscapingClosure(JSContext *cx, JSSta
>      * Fill in the rest of wscript. This means if you add members to JSScript
>      * you must update this code. FIXME: factor into JSScript::clone method.
>      */
>-    wscript->flags = script->flags;
>+    wscript->noScriptRval = script->noScriptRval;
>+    wscript->savedCallerFun = script->savedCallerFun;
>+    wscript->hasSharps = script->hasSharps;

Here is the only place going to bitfields loses slightly. Worth it for readability and (more significant?) gdb-ability (without archer).

>+            if (down && down->script && (down->script->hasSharps)) {

Don't overparenthesize -> expressions.

>+            if ((script->savedCallerFun) &&

Ditto.

Glad to see some bitfields, even though the inlined clone case in WrapEscapingClosure makes me very slightly sad.

/be

Comment 3

[Jim Blandy :jimb]

Attachment: Use bitfields in JSScript instead of a flag word. [landed]

Address nits.  Should have noticed the gratuitous parens --- thanks.

Comment 4

[Jeff Walden [:Waldo]]

(In reply to comment #2)
> (From update of attachment 401941 [details] [diff] [review])
> >@@ -506,7 +506,9 @@ WrapEscapingClosure(JSContext *cx, JSSta
> >      * Fill in the rest of wscript. This means if you add members to JSScript
> >      * you must update this code. FIXME: factor into JSScript::clone method.
> >      */
> >-    wscript->flags = script->flags;
> >+    wscript->noScriptRval = script->noScriptRval;
> >+    wscript->savedCallerFun = script->savedCallerFun;
> >+    wscript->hasSharps = script->hasSharps;
> 
> Here is the only place going to bitfields loses slightly. Worth it for
> readability and (more significant?) gdb-ability (without archer).

We could enclose bitfields in struct { /* ... */ }; bits if we wanted, to make bitfield copying easy.  You can assign structs in C++ even if they don't have operator=, can't you?

Comment 5

[Jim Blandy :jimb]

(In reply to comment #4)
> We could enclose bitfields in struct { /* ... */ }; bits if we wanted, to make
> bitfield copying easy.  You can assign structs in C++ even if they don't have
> operator=, can't you?

Sure, we could do that.  It feels a bit heavy to me: a new type, "x.flag" -> "x.bits.flag".  But it would avoid "forgot to copy that bit" bugs.  If others think that would be nice, I'm game.

Comment 6

[Jim Blandy :jimb]

Attachment: Move 'grouped' to explicit param of JS_NEW_PRINTER.

Make all the JSPrinter status bits explicit flags to JS_NEW_PRINTER.
Only in public-facing functions like JS_DecompileFunction should we be
peeling bits out of indent values and turning them into flags;
internally, we can change interfaces to make more sense.

This is preparation for adding another flag to JS_NEW_PRINTER.

Comment 7

[Jim Blandy :jimb]

Attachment: Recognize "use strict" directives.

Comment 8

[Jim Blandy :jimb]

Attachment: Machinery for reporting ES5 strict mode errors.

At compile-time, we must consult the current JSTreeContext to decide
whether to issue an ES5 strict mode error; at run-time, we need to
check the strictness of the currently executing script.  Both cases
also check the context options.  The design is supposed to make it
easy to follow the principle that conditions treated as errors in ES5
strict mode are a subset of those warned about by JSOPTION_STRICT.

This patch removes report flag handling from js_ExpandErrorArguments,
which is used for both compile-time and run-time errors.  At run-time,
the new checkReportFlags handles the checks.  At compile-time, we need
different checks depending on the situation, so the checks are done in
js_ReportES5StrictError, js_ReportCompileErrorNumber, and the new
ReportCompileErrorNumberVA.

Comment 9

[Jim Blandy :jimb]

Igor --- I hope you don't mind my dumping this series on you; I thought it would be best for a single reviewer to do all the patches related to the fundamental setup.  The actual strict mode checks will be in other bugs, with different reviewers, because they just check the strict mode flags and are otherwise independent.  Thanks for your time!

Comment 10

[Igor Bukanov]

Comment on attachment 404893 [details] [diff] [review]
Move 'grouped' to explicit param of JS_NEW_PRINTER.

>@@ -4887,7 +4887,8 @@ JS_DecompileScript(JSContext *cx, JSScri
>     CHECK_REQUEST(cx);
>     jp = JS_NEW_PRINTER(cx, name, NULL,
>                         indent & ~JS_DONT_PRETTY_PRINT,
>-                        !(indent & JS_DONT_PRETTY_PRINT));
>+                        !(indent & JS_DONT_PRETTY_PRINT),
>+                        JS_FALSE);

Nit: use bool/true/false here and everywhere.

>+JSString *
>+js_DecompileToString(JSContext *cx, const char *name, JSFunction *fun,
>+                     uintN indent, JSBool pretty, JSBool grouped,
>+                     JSBool (*decompiler)(JSPrinter *jp))
> #ifdef JS_ARENAMETER
>-# define JS_NEW_PRINTER(cx, name, fun, indent, pretty)                        \
>-    js_NewPrinter(cx, name, fun, indent, pretty)
>+# define JS_NEW_PRINTER(cx, name, fun, indent, pretty, grouped)               \
>+    js_NewPrinter(cx, name, fun, indent, pretty, grouped)
> #else
>-# define JS_NEW_PRINTER(cx, name, fun, indent, pretty)                        \
>-    js_NewPrinter(cx, fun, indent, pretty)
>+# define JS_NEW_PRINTER(cx, name, fun, indent, pretty, grouped)               \
>+    js_NewPrinter(cx, fun, indent, pretty, grouped)
> #endif

extern JSPrinter *
JS_NEW_PRINTER(JSContext *cx, const char *name, JSFunction *fun,
               uintN indent, JSBool pretty);

Nit: the name parameter is useless unless debug-only JS_ARENAMETER is defined. To address this and pre-existing all-caps JS_NEW_PRINTER ugliness define them as:

#ifndef JS_ARENAMETER
#define js_DecompileToString(cx, name, fun, indent, pretty, grouped, jp) \
    js_DecompileToString (cx, fun, indent, pretty, grouped, jp)
   
#define js_NewPrinter(cx, name, fun, indent, pretty, grouped) \
     js_NewPrinter (cx, fun, indent, pretty, grouped, jp)
#endif

extern JSString *
js_DecompileToString(JSContext *cx, const char *name, JSFunction *fun,
                     uintN indent, JSBool pretty, JSBool grouped,
                     JSBool (*decompiler)(JSPrinter *jp));

extern JSPrinter *
js_NewPrinter(JSContext *cx, const char *name, JSFunction *fun,
               uintN indent, JSBool pretty);


That is, use that after #define x() the decompiler expands only x(), not x<blank>()

Comment 11

[Igor Bukanov]

Comment on attachment 404899 [details] [diff] [review]
Recognize "use strict" directives.

> struct JSPrinter {
>     Sprinter        sprinter;       /* base class state */
>     JSArenaPool     pool;           /* string allocation pool */
>     uintN           indent;         /* indentation in spaces */
>     JSPackedBool    pretty;         /* pretty-print: indent, use newlines */
>     JSPackedBool    grouped;        /* in parenthesized expression context */
>+    JSPackedBool    strict;         /* in code marked strict */

Nit: Replace JSPackedBool with bool.

>+    JSBool inDirectivePrologue;

Nit: again, use bool here.

> #ifdef METER_PARSENODES
>     void *sbrk(ptrdiff_t), *before = sbrk(0);
> #endif
> 
>     JS_ASSERT(!(tcflags & ~(TCF_COMPILE_N_GO | TCF_NO_SCRIPT_RVAL)));
> 
>     /*
>      * The scripted callerFrame can only be given for compile-and-go scripts
>@@ -878,32 +883,37 @@ JSCompiler::compileScript(JSContext *cx,
>     onlyXML = true;
> #endif
> 
>     CG_SWITCH_TO_PROLOG(&cg);
>     if (js_Emit1(cx, &cg, JSOP_TRACE) < 0)
>         goto out;
>     CG_SWITCH_TO_MAIN(&cg);
> 
>+    inDirectivePrologue = JS_TRUE;
>     for (;;) {
>         jsc.tokenStream.flags |= TSF_OPERAND;
>         tt = js_PeekToken(cx, &jsc.tokenStream);
>         jsc.tokenStream.flags &= ~TSF_OPERAND;
>         if (tt <= TOK_EOF) {
>             if (tt == TOK_EOF)
>                 break;
>             JS_ASSERT(tt == TOK_ERROR);
>             goto out;
>         }
> 
>         pn = Statement(cx, &jsc.tokenStream, &cg);
>         if (!pn)
>             goto out;
>         JS_ASSERT(!cg.blockNode);
> 
>+        if (inDirectivePrologue)
>+            inDirectivePrologue
>+                = RecognizeDirectivePrologue(cx, &jsc.tokenStream, &cg, pn);
>+
>         if (!js_FoldConstants(cx, pn, &cg))
>             goto out;
> 
>         if (cg.functionList) {
>             if (!jsc.analyzeFunctions(cg.functionList, cg.flags))
>                 goto out;
>             cg.functionList = NULL;
>         }
>@@ -2860,41 +2870,80 @@ FunctionDef(JSContext *cx, JSTokenStream
>         pn->pn_body = body;
>     }
> 
>     pn->pn_blockid = tc->blockid();
> 
>     if (!LeaveFunction(pn, &funtc, tc, funAtom, lambda))
>         return NULL;
> 
>+    // If the surrounding function is not strict code, reset the lexer.
>+    if (!(tc->flags & TCF_ES5_STRICT_MODE))
>+        ts->flags &= ~TSF_ES5_STRICT_MODE;
>+
>     return result;
> }
> 
> static JSParseNode *
> FunctionStmt(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc)
> {
>     return FunctionDef(cx, ts, tc, 0);
> }
> 
> static JSParseNode *
> FunctionExpr(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc)
> {
>     return FunctionDef(cx, ts, tc, JSFUN_LAMBDA);
> }
> 
> /*
>+ * Recognize Directive Prologue members and directives.  Assuming PN
>+ * is a candidate for membership in a directive prologue, return
>+ * JS_TRUE if it is in fact a member.  Recognize directives and set
>+ * TC's flags accordingly.
>+ *
>+ * Note that the following is a strict mode function:
>+ *
>+ * function foo() {
>+ *   "blah" // inserted semi colon
>+ *        "blurgh"
>+ *   "use\x20loose"
>+ *   "use strict"
>+ * }
>+ *
>+ * That is, a statement can be a Directive Prologue member, even
>+ * if it can't possibly be a directive, now or in the future.
>+ */
>+static JSBool
>+RecognizeDirectivePrologue(JSContext *cx, JSTokenStream *ts,
>+                           JSTreeContext *tc, JSParseNode *pn)
>+{
>+    if (!pn->isDirectivePrologueMember())
>+        return JS_FALSE;
>+    if (pn->isDirective()) {
>+        JSAtom *directive = pn->pn_kid->pn_atom;
>+        if (directive == cx->runtime->atomState.useStrictAtom) {
>+            tc->flags |= TCF_ES5_STRICT_MODE;
>+            ts->flags |= TSF_ES5_STRICT_MODE;

Nit: remove the redundant line.

>+    /*
>+     * True if this node, known to be a Directive Prologue member,
>+     * could be a directive itself.
>+     */
>+    bool isDirective() const {
>+        JS_ASSERT(isDirectivePrologueMember());
>+        JSParseNode *kid = pn_kid;
>+        JSString *str = ATOM_TO_STRING(kid->pn_atom);
>+        /* Directives must contain no EscapeSequences or
>+           LineContinuations.  If the string's length in the source
>+           code is its length as a value, accounting for the quotes,
>+           then it qualifies.  */

Nit: use the proper comment style here like:
/*
 *
 */

Comment 12

[Jim Blandy :jimb]

(In reply to comment #10)
> (From update of attachment 404893 [details] [diff] [review])
> >@@ -4887,7 +4887,8 @@ JS_DecompileScript(JSContext *cx, JSScri
> >     CHECK_REQUEST(cx);
> >     jp = JS_NEW_PRINTER(cx, name, NULL,
> >                         indent & ~JS_DONT_PRETTY_PRINT,
> >-                        !(indent & JS_DONT_PRETTY_PRINT));
> >+                        !(indent & JS_DONT_PRETTY_PRINT),
> >+                        JS_FALSE);
> 
> Nit: use bool/true/false here and everywhere.

Will do --- thanks.

> >+JSString *
> >+js_DecompileToString(JSContext *cx, const char *name, JSFunction *fun,
> >+                     uintN indent, JSBool pretty, JSBool grouped,
> >+                     JSBool (*decompiler)(JSPrinter *jp))
> > #ifdef JS_ARENAMETER
> >-# define JS_NEW_PRINTER(cx, name, fun, indent, pretty)                        \
> >-    js_NewPrinter(cx, name, fun, indent, pretty)
> >+# define JS_NEW_PRINTER(cx, name, fun, indent, pretty, grouped)               \
> >+    js_NewPrinter(cx, name, fun, indent, pretty, grouped)
> > #else
> >-# define JS_NEW_PRINTER(cx, name, fun, indent, pretty)                        \
> >-    js_NewPrinter(cx, fun, indent, pretty)
> >+# define JS_NEW_PRINTER(cx, name, fun, indent, pretty, grouped)               \
> >+    js_NewPrinter(cx, fun, indent, pretty, grouped)
> > #endif
> 
> extern JSPrinter *
> JS_NEW_PRINTER(JSContext *cx, const char *name, JSFunction *fun,
>                uintN indent, JSBool pretty);
> 
> Nit: the name parameter is useless unless debug-only JS_ARENAMETER is defined.
> To address this and pre-existing all-caps JS_NEW_PRINTER ugliness define them
> as:
> 
> #ifndef JS_ARENAMETER
> #define js_DecompileToString(cx, name, fun, indent, pretty, grouped, jp) \
>     js_DecompileToString (cx, fun, indent, pretty, grouped, jp)
> 
> #define js_NewPrinter(cx, name, fun, indent, pretty, grouped) \
>      js_NewPrinter (cx, fun, indent, pretty, grouped, jp)
> #endif
> 
> extern JSString *
> js_DecompileToString(JSContext *cx, const char *name, JSFunction *fun,
>                      uintN indent, JSBool pretty, JSBool grouped,
>                      JSBool (*decompiler)(JSPrinter *jp));
> 
> extern JSPrinter *
> js_NewPrinter(JSContext *cx, const char *name, JSFunction *fun,
>                uintN indent, JSBool pretty);
> 
> 
> That is, use that after #define x() the decompiler expands only x(), not
> x<blank>()

You mean the preprocessor, right?  Actually, it does expand x<blank>().  It won't expand something that would introduce infinite recursion, though.  So:

$ cat self.c
#define X(a,b) X(a,b)
#define Y(a) y a y
#define Z(a) Y (a)

X: X(1,2)
Z1: Z(a)
Z2: Z (a)
$ gcc -E self.c
# 1 "self.c"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "self.c"




X: X(1,2)
Z1: y a y
Z2: y a y
$ 

So your suggestion could be even simpler.

But I have to say, that seems extremely confusing.  If I were dealing with js_NewPrinter but wasn't aware of the macro, I think I would tear my hair out trying to figure out why js_NewPrinter didn't have the 'name' argument I'd written in the source code, since there would be no indication that there is a macro involved.

How about something like this:

In jsarena.h:
#ifdef JS_ARENAMETER
#define ARENAMETER_ONLY(x) x
#else
#define ARENAMETER_ONLY(x)
#endif

In jsopcodes.h:
extern JSPrinter *
js_NewPrinter(JSContext *cx, ARENAMETER_ONLY(const char *name,) JSFunction *fun, ...);

... and obvious concomitant changes in jsopcodes.cpp.

Would that be better?

Comment 13

[Jim Blandy :jimb]

Based on Brendan's comments (bug 514567 comment 8; bug 514559 comment 5), I've made the following substitutions throughout the patch series:

    -e 's/TCF_ES5_STRICT_MODE/TCF_STRICT_MODE_CODE/g' \
    -e 's/TSF_ES5_STRICT_MODE/TSF_STRICT_MODE_CODE/g' \
    -e 's/TCF_ES5_STRICT_MODE/TCF_STRICT_MODE_CODE/g' \
    -e 's/es5StrictMode/strictModeCode/g' \
    -e 's/JSREPORT_ES5_STRICT_ERROR/JSREPORT_STRICT_MODE_ERROR/g' \
    -e 's/JSREPORT_IS_ES5_STRICT_ERROR/JSREPORT_IS_STRICT_MODE_ERROR/g' \
    -e 's/js_ReportES5StrictError/js_ReportStrictModeError/g' \

Comment 14

[Jim Blandy :jimb]

(In reply to comment #12)
> How about something like this:
> 
> In jsarena.h:
> #ifdef JS_ARENAMETER
> #define ARENAMETER_ONLY(x) x
> #else
> #define ARENAMETER_ONLY(x)
> #endif
> 
> In jsopcodes.h:
> extern JSPrinter *
> js_NewPrinter(JSContext *cx, ARENAMETER_ONLY(const char *name,) JSFunction
> *fun, ...);
> 
> ... and obvious concomitant changes in jsopcodes.cpp.
> 
> Would that be better?

What's ugly about this is that every call would need to use ARENAMETER_ONLY, too.  The macro has the advantage of letting one write as if the name parameter is always passed.  However, ARENAMETER_ONLY does seem to have the advantage of being less magical.

It would be nice to apply the same solution to both this and JS_InitArenaPool, which uses the same kludge.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 404899 [details] [diff] [review]
Recognize "use strict" directives.

>@@ -5048,19 +5052,32 @@ js_DecompileFunction(JSPrinter *jp)
> #if JS_HAS_DESTRUCTURING
>         jp->script = NULL;
>         JS_ARENA_RELEASE(&jp->sprinter.context->tempPool, mark);
> #endif
>         if (!ok)
>             return JS_FALSE;
>         if (fun->flags & JSFUN_EXPR_CLOSURE) {
>             js_printf(jp, ") ");
>+            if (fun->u.i.script->es5StrictMode && !jp->strict) {
>+                /*
>+                 * We have no syntax for strict function expressions;
>+                 * at least give a hint.
>+                 */
>+                js_printf(jp, "\t/* use strict */ \n");
>+                jp->strict = JS_TRUE;
>+            }

We could decompile as a function expression instead. It would stabilize after one round trip. Hmm.

/be

Comment 16

[Brendan Eich [:brendan]]

For JS_NewPrinter, why not always take the name parameter? Is that going to get us an unused parameter warning?

/be

Comment 17

[Igor Bukanov]

(In reply to comment #16)
> For JS_NewPrinter, why not always take the name parameter? Is that going to get
> us an unused parameter warning?

What about just dropping that parameter and passing "jsprinter" or something JS_INIT_ARENA_POOL?

Comment 18

[Jim Blandy :jimb]

Attachment: Remove macro wrappers for js_NewPrinter and JS_InitArenaPool. [committed]

How about just clearing all that crud out?

These macros were intended to help avoid passing unused parameters
when JS_ARENAMETER is not defined, but they're not worth the trouble.
The unused parameter doesn't elicit warnings, and the performance
impact of passing the unneeded argument is negligible.

Comment 19

[Brendan Eich [:brendan]]

Yes, just always pass the name. If it is worthless remove it, but at this point the simpler step (and it helps for arena metering) is to make name unconditional. Thanks,

/be

Comment 20

[Jim Blandy :jimb]

I've used bool/true/false instead of JSBool/JSPackedBool/JS_TRUE/JS_FALSE throughout, and fixed comment styles.

(In reply to comment #11)
> >+    if (pn->isDirective()) {
> >+        JSAtom *directive = pn->pn_kid->pn_atom;
> >+        if (directive == cx->runtime->atomState.useStrictAtom) {
> >+            tc->flags |= TCF_ES5_STRICT_MODE;
> >+            ts->flags |= TSF_ES5_STRICT_MODE;
> 
> Nit: remove the redundant line.

Do you mean the last one quoted?  That's adjusting the token stream's state, which is separate from the tree context's state.

Comment 21

[Jim Blandy :jimb]

Attachment: Recognize "use strict" directives.

Revised per Igor's comments.

Comment 22

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/20b808b1575c

Comment 23

[Jim Blandy :jimb]

Only one patch landed out of five.

Comment 24

[Jim Blandy :jimb]

Attachment: Move 'grouped' to explicit param of JS_NEW_PRINTER. r=igor [committed]

Revised version of already-approved patch.

Comment 25

[Jim Blandy :jimb]

Hi, Igor.  Review ping on the patches here.  I believe all the patches you've commented on have been updated.

Comment 26

[Jim Blandy :jimb]

Hi, Igor.  Is there anything I can do to help out with this?  Should I r? brendan instead?

Comment 27

[David Mandelin [:dmandelin]]

I just backed out 723a2622ad4a from TM because it broke the build on tinderbox:

changeset:   34244:723a2622ad4a
user:        Jim Blandy <[e-mail]>
date:        Tue Oct 27 16:38:09 2009 -0700
summary:     Bug 514585: Move 'grouped' to explicit param of JS_NEW_PRINTER. r=igor

Comment 28

[Jim Blandy :jimb]

(In reply to comment #27)
> I just backed out 723a2622ad4a from TM because it broke the build on tinderbox:

Thanks, David.  The problem is that js/jsd #includes jsopcodes.h as C code, where one needs to #include <stdbool.h> to get the 'bool' type.

Comment 29

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/723a2622ad4a

Comment 30

[Jim Blandy :jimb]

Hi, Igor.  There are still two patches waiting for your review here.

Comment 31

[Igor Bukanov]

Comment on attachment 404902 [details] [diff] [review]
Machinery for reporting ES5 strict mode errors.

>diff --git a/js/src/jsapi.h b/js/src/jsapi.h

>+/* This condition is an error in ES5 strict mode code, a warning if
>+   JS_HAS_STRICT_OPTION(cx), and otherwise should not be reported at
>+   all.  We check the strictness of the context's top frame's script;
>+   where that isn't appropriate, the caller should do the right checks
>+   itself instead of using this flag.  */

Nit: use here and *everywhere* SpiderMonkey comment style.

>+#define JSREPORT_ES5_STRICT_ERROR 0x8
>+
> /*
>  * If JSREPORT_EXCEPTION is set, then a JavaScript-catchable exception
>  * has been thrown for this runtime error, and the host should ignore it.
>@@ -2691,6 +2698,8 @@ struct JSErrorReport {
> #define JSREPORT_IS_WARNING(flags)      (((flags) & JSREPORT_WARNING) != 0)
> #define JSREPORT_IS_EXCEPTION(flags)    (((flags) & JSREPORT_EXCEPTION) != 0)
> #define JSREPORT_IS_STRICT(flags)       (((flags) & JSREPORT_STRICT) != 0)
>+#define JSREPORT_IS_ES5_STRICT_ERROR(flags) \
>+                                   (((flags) & JSREPORT_ES5_STRICT_ERROR) != 0)

Nit: SpiderMonkey style is to put \ that continue the line at the column 78. Also use the following indent here:

#define JSREPORT_IS_ES5_STRICT_ERROR(flags) (((flags) &                      \
                                             JSREPORT_ES5_STRICT_ERROR) != 0)

>diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
>--- a/js/src/jscntxt.cpp
>+++ b/js/src/jscntxt.cpp
>@@ -1368,6 +1368,37 @@ js_ReportAllocationOverflow(JSContext *c
>     JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_ALLOC_OVERFLOW);
> }
> 
>+/*
>+ * Given FLAGS and the state of CX, decide whether we should report an
>+ * error, a warning, or just continue execution normally.  Return
>+ * JS_TRUE if we should continue normally, without reporting anything;
>+ * otherwise, adjust *FLAGS as appropriate and return JS_FALSE.
>+ */

Nit: use low-case flags and cx names when referring to arguments.

>+static JSBool
>+checkReportFlags(JSContext *cx, uintN *flags)
>+{
>+    if (JSREPORT_IS_ES5_STRICT_ERROR(*flags)) {
>+        // Error in strict code; warning with strict option; okay otherwise.

Nit: use /* comments here and *everywhere*.

>+        JS_ASSERT(JS_IsRunning(cx));
>+        if (cx->fp->script->es5StrictMode)
>+            *flags &= ~JSREPORT_WARNING;
>+        else if (JS_HAS_STRICT_OPTION(cx))
>+            *flags |= JSREPORT_WARNING;
>+        else
>+            return JS_TRUE;

Nit: use true/false in any new code. Also, what stops checkReportFlags from be a bool, not JSBool?

R+ with this addressed

Comment 32

[Brendan Eich [:brendan]]

(In reply to comment #31)
> Nit: SpiderMonkey style is to put \ that continue the line at the column 78.

79, numbering from 1.

/be

Comment 33

[Brendan Eich [:brendan]]

Why is this bug RESOLVED/FIXED?

/be

Comment 34

[Robert Sayre]

(In reply to comment #33)
> Why is this bug RESOLVED/FIXED?
> 
> /be

my fault, I must have marked it by reflex when I landed the first patch on mozilla-central.

Comment 35

[Igor Bukanov]

(In reply to comment #34)
> (In reply to comment #33)
> > Why is this bug RESOLVED/FIXED?
> > 
> > /be
> 
> my fault, I must have marked it by reflex when I landed the first patch on
> mozilla-central.

This is going to be messy. Perhaps the remaining patches should go to a separated bug with this one remained fixed?

Comment 36

[Jim Blandy :jimb]

Attachment: Machinery for reporting ES5 strict mode errors. [committed]

Revised per Igor's comments.

(In reply to comment #31)
> Nit: use here and *everywhere* SpiderMonkey comment style.

Okay.  I've done this throughout the patch series.

> Nit: use low-case flags and cx names when referring to arguments.

I'll do this throughout the series.

> >+        JS_ASSERT(JS_IsRunning(cx));
> >+        if (cx->fp->script->es5StrictMode)
> >+            *flags &= ~JSREPORT_WARNING;
> >+        else if (JS_HAS_STRICT_OPTION(cx))
> >+            *flags |= JSREPORT_WARNING;
> >+        else
> >+            return JS_TRUE;
> 
> Nit: use true/false in any new code. Also, what stops checkReportFlags from be
> a bool, not JSBool?

You mentioned this a while back; I'd fixed my copy, but not updated the attachment in bugzilla.  Sorry about that.

Comment 37

[Jim Blandy :jimb]

Attachment: Recognize "use strict" directives. [committed]

Post latest revision of patch.

Comment 38

[Jim Blandy :jimb]

(In reply to comment #34)
> my fault, I must have marked it by reflex when I landed the first patch on
> mozilla-central.

This causes Rob so much trouble --- I'll stick to one-bug-per-patch in the future.

Comment 39

[Igor Bukanov]

Comment on attachment 412750 [details] [diff] [review]
Recognize "use strict" directives. [committed]

>Bug 514585: Recognize "use strict" directives.
>
>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -4909,7 +4909,7 @@ JS_DecompileScript(JSContext *cx, JSScri
>     jp = js_NewPrinter(cx, name, NULL,
>                        indent & ~JS_DONT_PRETTY_PRINT,
>                        !(indent & JS_DONT_PRETTY_PRINT),
>-                       false);
>+                       false, false);
>     if (!jp)
>         return NULL;
>     if (js_DecompileScript(jp, script))
>@@ -4927,7 +4927,7 @@ JS_DecompileFunction(JSContext *cx, JSFu
>     return js_DecompileToString(cx, "JS_DecompileFunction", fun,
>                                 indent & ~JS_DONT_PRETTY_PRINT,
>                                 !(indent & JS_DONT_PRETTY_PRINT),
>-                                false, js_DecompileFunction);
>+                                false, false, js_DecompileFunction);
> }
> 
> JS_PUBLIC_API(JSString *)
>@@ -4937,7 +4937,7 @@ JS_DecompileFunctionBody(JSContext *cx, 
>     return js_DecompileToString(cx, "JS_DecompileFunctionBody", fun,
>                                 indent & ~JS_DONT_PRETTY_PRINT,
>                                 !(indent & JS_DONT_PRETTY_PRINT),
>-                                false, js_DecompileFunctionBody);
>+                                false, false, js_DecompileFunctionBody);
> }
> 
> JS_PUBLIC_API(JSBool)
>diff --git a/js/src/jsatom.cpp b/js/src/jsatom.cpp
>--- a/js/src/jsatom.cpp
>+++ b/js/src/jsatom.cpp
>@@ -162,6 +162,7 @@ const char *const js_common_atom_names[]
>     js_configurable_str,        /* configurableAtom             */
>     js_writable_str,            /* writableAtom                 */
>     js_value_str,               /* valueAtom                    */
>+    "use strict",               /* useStrictAtom                */
> 
> #if JS_HAS_XML_SUPPORT
>     js_etago_str,               /* etagoAtom                    */
>diff --git a/js/src/jsatom.h b/js/src/jsatom.h
>--- a/js/src/jsatom.h
>+++ b/js/src/jsatom.h
>@@ -264,6 +264,7 @@ struct JSAtomState {
>     JSAtom              *configurableAtom;
>     JSAtom              *writableAtom;
>     JSAtom              *valueAtom;
>+    JSAtom              *useStrictAtom;
> 
> #if JS_HAS_XML_SUPPORT
>     JSAtom              *etagoAtom;
>diff --git a/js/src/jsemit.h b/js/src/jsemit.h
>--- a/js/src/jsemit.h
>+++ b/js/src/jsemit.h
>@@ -234,11 +234,6 @@ struct JSTreeContext {              /* t
>     bool ensureSharpSlots();
> };
> 
>-/*
>- * Flags to propagate out of the blocks.
>- */
>-#define TCF_RETURN_FLAGS        (TCF_RETURN_EXPR | TCF_RETURN_VOID)
>-
> #define TCF_COMPILING           0x01 /* JSTreeContext is JSCodeGenerator */
> #define TCF_IN_FUNCTION         0x02 /* parsing inside function body */
> #define TCF_RETURN_EXPR         0x04 /* function has 'return expr;' */
>@@ -283,6 +278,19 @@ struct JSTreeContext {              /* t
> #define TCF_NEED_MUTABLE_SCRIPT 0x20000
> 
> /*
>+ * This function/global/eval code body contained a Use Strict
>+ * Directive.  Treat certain strict warnings as errors, and forbid
>+ * the use of 'with'.  See also TSF_STRICT_MODE_CODE,
>+ * JSScript::strictModeCode, and JSREPORT_STRICT_ERROR.
>+ */
>+#define TCF_STRICT_MODE_CODE       0x40000
>+
>+/*
>+ * Flags to propagate out of the blocks.
>+ */
>+#define TCF_RETURN_FLAGS        (TCF_RETURN_EXPR | TCF_RETURN_VOID)
>+
>+/*
>  * Sticky deoptimization flags to propagate from FunctionBody.
>  */
> #define TCF_FUN_FLAGS           (TCF_FUN_SETS_OUTER_NAME |                    \
>@@ -291,7 +299,8 @@ struct JSTreeContext {              /* t
>                                  TCF_FUN_HEAVYWEIGHT     |                    \
>                                  TCF_FUN_IS_GENERATOR    |                    \
>                                  TCF_FUN_USES_OWN_NAME   |                    \
>-                                 TCF_HAS_SHARPS)
>+                                 TCF_HAS_SHARPS          |                    \
>+                                 TCF_STRICT_MODE_CODE)
> 
> /*
>  * Span-dependent instructions are jumps whose span (from the jump bytecode to
>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>--- a/js/src/jsfun.cpp
>+++ b/js/src/jsfun.cpp
>@@ -475,6 +475,7 @@ WrapEscapingClosure(JSContext *cx, JSSta
>     wscript->noScriptRval = script->noScriptRval;
>     wscript->savedCallerFun = script->savedCallerFun;
>     wscript->hasSharps = script->hasSharps;
>+    wscript->strictModeCode = script->strictModeCode;
>     wscript->version = script->version;
>     wscript->nfixed = script->nfixed;
>     wscript->filename = script->filename;
>diff --git a/js/src/jsopcode.cpp b/js/src/jsopcode.cpp
>--- a/js/src/jsopcode.cpp
>+++ b/js/src/jsopcode.cpp
>@@ -724,8 +724,9 @@ struct JSPrinter {
>     Sprinter        sprinter;       /* base class state */
>     JSArenaPool     pool;           /* string allocation pool */
>     uintN           indent;         /* indentation in spaces */
>-    JSPackedBool    pretty;         /* pretty-print: indent, use newlines */
>-    JSPackedBool    grouped;        /* in parenthesized expression context */
>+    bool            pretty;         /* pretty-print: indent, use newlines */
>+    bool            grouped;        /* in parenthesized expression context */
>+    bool            strict;         /* in code marked strict */
>     JSScript        *script;        /* script being printed */
>     jsbytecode      *dvgfence;      /* DecompileExpression fencepost */
>     jsbytecode      **pcstack;      /* DecompileExpression modeled stack */
>@@ -735,7 +736,7 @@ struct JSPrinter {
> 
> JSPrinter *
> js_NewPrinter(JSContext *cx, const char *name, JSFunction *fun,
>-              uintN indent, JSBool pretty, JSBool grouped)
>+              uintN indent, JSBool pretty, JSBool grouped, JSBool strict)
> {
>     JSPrinter *jp;
> 
>@@ -747,6 +748,7 @@ js_NewPrinter(JSContext *cx, const char 
>     jp->indent = indent;
>     jp->pretty = pretty;
>     jp->grouped = grouped;
>+    jp->strict = strict;
>     jp->script = NULL;
>     jp->dvgfence = NULL;
>     jp->pcstack = NULL;
>@@ -2244,7 +2246,8 @@ Decompile(SprintStack *ss, jsbytecode *p
>                   do_function:
>                     js_puts(jp, "\n");
>                     jp2 = js_NewPrinter(cx, "nested_function", fun,
>-                                        jp->indent, jp->pretty, jp->grouped);
>+                                        jp->indent, jp->pretty, jp->grouped,
>+                                        jp->strict);
>                     if (!jp2)
>                         return NULL;
>                     ok = js_DecompileFunction(jp2);
>@@ -4144,8 +4147,9 @@ Decompile(SprintStack *ss, jsbytecode *p
>                      * that checks for JSOP_LAMBDA.
>                      */
>                     bool grouped = !(fun->flags & JSFUN_EXPR_CLOSURE);
>+                    bool strict = jp->script->strictModeCode;
>                     str = js_DecompileToString(cx, "lambda", fun, 0, 
>-                                               false, grouped,
>+                                               false, grouped, strict,
>                                                js_DecompileFunction);
>                     if (!str)
>                         return NULL;
>@@ -4902,13 +4906,13 @@ js_DecompileScript(JSPrinter *jp, JSScri
> 
> JSString *
> js_DecompileToString(JSContext *cx, const char *name, JSFunction *fun,
>-                     uintN indent, JSBool pretty, JSBool grouped,
>+                     uintN indent, JSBool pretty, JSBool grouped, JSBool strict,
>                      JSBool (*decompiler)(JSPrinter *jp))
> {
>     JSPrinter *jp;
>     JSString *str;
> 
>-    jp = js_NewPrinter(cx, name, fun, indent, pretty, grouped);
>+    jp = js_NewPrinter(cx, name, fun, indent, pretty, grouped, strict);
>     if (!jp)
>         return NULL;
>     if (decompiler(jp))
>@@ -5064,9 +5068,22 @@ js_DecompileFunction(JSPrinter *jp)
>             return JS_FALSE;
>         if (fun->flags & JSFUN_EXPR_CLOSURE) {
>             js_printf(jp, ") ");
>+            if (fun->u.i.script->strictModeCode && !jp->strict) {
>+                /*
>+                 * We have no syntax for strict function expressions;
>+                 * at least give a hint.
>+                 */
>+                js_printf(jp, "\t/* use strict */ \n");
>+                jp->strict = true;
>+            }
>+            
>         } else {
>             js_printf(jp, ") {\n");
>             jp->indent += 4;
>+            if (fun->u.i.script->strictModeCode && !jp->strict) {
>+                js_printf(jp, "\t'use strict';\n");
>+                jp->strict = true;
>+            }
>         }
> 
>         len = script->code + script->length - pc;
>@@ -5314,7 +5331,7 @@ DecompileExpression(JSContext *cx, JSScr
> 
>     name = NULL;
>     jp = js_NewPrinter(cx, "js_DecompileValueGenerator", fun, 0,
>-                       false, false);
>+                       false, false, false);
>     if (jp) {
>         jp->dvgfence = end;
>         jp->pcstack = pcstack;
>diff --git a/js/src/jsopcode.h b/js/src/jsopcode.h
>--- a/js/src/jsopcode.h
>+++ b/js/src/jsopcode.h
>@@ -273,11 +273,16 @@ js_QuoteString(JSContext *cx, JSString *
>  * JSPrinter operations, for printf style message formatting.  The return
>  * value from js_GetPrinterOutput() is the printer's cumulative output, in
>  * a GC'ed string.
>+ *
>+ * strict is true if the context in which the output will appear has
>+ * already been marked as strict, thus indicating that nested
>+ * functions need not be re-marked with a strict directive.  It should
>+ * be false in the outermost printer.
>  */
> 
> extern JSPrinter *
> js_NewPrinter(JSContext *cx, const char *name, JSFunction *fun,
>-              uintN indent, JSBool pretty, JSBool grouped);
>+              uintN indent, JSBool pretty, JSBool grouped, JSBool strict);
> 
> extern void
> js_DestroyPrinter(JSPrinter *jp);
>@@ -421,7 +426,7 @@ js_DecompileFunction(JSPrinter *jp);
> 
> extern JSString *
> js_DecompileToString(JSContext *cx, const char *name, JSFunction *fun,
>-                     uintN indent, JSBool pretty, JSBool grouped,
>+                     uintN indent, JSBool pretty, JSBool grouped, JSBool strict,
>                      JSBool (*decompiler)(JSPrinter *jp));
> 
> /*
>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
>--- a/js/src/jsparse.cpp
>+++ b/js/src/jsparse.cpp
>@@ -147,6 +147,9 @@ static JSMemberParser  MemberExpr;
> static JSPrimaryParser PrimaryExpr;
> static JSParenParser   ParenExpr;
> 
>+static bool RecognizeDirectivePrologue(JSContext *cx, JSTokenStream *ts,
>+                                         JSTreeContext *tc, JSParseNode *pn);
>+
> /*
>  * Insist that the next token be of type tt, or report errno and return null.
>  * NB: this macro uses cx and ts from its lexical environment.
>@@ -306,7 +309,8 @@ JSCompiler::newFunctionBox(JSObject *obj
>         }
>     }
>     funbox->level = tc->staticLevel;
>-    funbox->tcflags = TCF_IN_FUNCTION | (tc->flags & TCF_COMPILE_N_GO);
>+    funbox->tcflags = (TCF_IN_FUNCTION
>+                       | (tc->flags & (TCF_COMPILE_N_GO | TCF_STRICT_MODE_CODE)));
>     return funbox;
> }
> 
>@@ -797,6 +801,7 @@ JSCompiler::compileScript(JSContext *cx,
>     JSParseNode *pn;
>     uint32 scriptGlobals;
>     JSScript *script;
>+    bool inDirectivePrologue;
> #ifdef METER_PARSENODES
>     void *sbrk(ptrdiff_t), *before = sbrk(0);
> #endif
>@@ -883,6 +888,7 @@ JSCompiler::compileScript(JSContext *cx,
>         goto out;
>     CG_SWITCH_TO_MAIN(&cg);
> 
>+    inDirectivePrologue = true;
>     for (;;) {
>         jsc.tokenStream.flags |= TSF_OPERAND;
>         tt = js_PeekToken(cx, &jsc.tokenStream);
>@@ -899,6 +905,10 @@ JSCompiler::compileScript(JSContext *cx,
>             goto out;
>         JS_ASSERT(!cg.blockNode);
> 
>+        if (inDirectivePrologue)
>+            inDirectivePrologue
>+                = RecognizeDirectivePrologue(cx, &jsc.tokenStream, &cg, pn);
>+
>         if (!js_FoldConstants(cx, pn, &cg))
>             goto out;
> 
>@@ -2865,6 +2875,10 @@ FunctionDef(JSContext *cx, JSTokenStream
>     if (!LeaveFunction(pn, &funtc, tc, funAtom, lambda))
>         return NULL;
> 
>+    /* If the surrounding function is not strict code, reset the lexer. */
>+    if (!(tc->flags & TCF_STRICT_MODE_CODE))
>+        ts->flags &= ~TSF_STRICT_MODE_CODE;
>+
>     return result;
> }
> 
>@@ -2881,6 +2895,40 @@ FunctionExpr(JSContext *cx, JSTokenStrea
> }
> 
> /*
>+ * Recognize Directive Prologue members and directives.  Assuming PN
>+ * is a candidate for membership in a directive prologue, return
>+ * true if it is in fact a member.  Recognize directives and set
>+ * TC's flags accordingly.
>+ *
>+ * Note that the following is a strict mode function:
>+ *
>+ * function foo() {
>+ *   "blah" // inserted semi colon
>+ *        "blurgh"
>+ *   "use\x20loose"
>+ *   "use strict"
>+ * }
>+ *
>+ * That is, a statement can be a Directive Prologue member, even
>+ * if it can't possibly be a directive, now or in the future.
>+ */
>+static bool
>+RecognizeDirectivePrologue(JSContext *cx, JSTokenStream *ts,
>+                           JSTreeContext *tc, JSParseNode *pn)
>+{
>+    if (!pn->isDirectivePrologueMember())
>+        return false;
>+    if (pn->isDirective()) {
>+        JSAtom *directive = pn->pn_kid->pn_atom;
>+        if (directive == cx->runtime->atomState.useStrictAtom) {
>+            tc->flags |= TCF_STRICT_MODE_CODE;
>+            ts->flags |= TSF_STRICT_MODE_CODE;
>+        }
>+    }
>+    return true;
>+}
>+
>+/*
>  * Parse the statements in a block, creating a TOK_LC node that lists the
>  * statements' trees.  If called from block-parsing code, the caller must
>  * match { before and } after.
>@@ -2890,6 +2938,7 @@ Statements(JSContext *cx, JSTokenStream 
> {
>     JSParseNode *pn, *pn2, *saveBlock;
>     JSTokenType tt;
>+    bool inDirectivePrologue = tc->atTopLevel();
> 
>     JS_CHECK_RECURSION(cx, return NULL);
> 
>@@ -2921,6 +2970,19 @@ Statements(JSContext *cx, JSTokenStream 
>             return NULL;
>         }
> 
>+        if (inDirectivePrologue) {
>+            if (RecognizeDirectivePrologue(cx, ts, tc, pn2)) {
>+                /* 
>+                 * A Directive Prologue member is dead code.  Omit it from the
>+                 * statement list.
>+                 */
>+                RecycleTree(pn2, tc);
>+                continue;
>+            } else {
>+                inDirectivePrologue = false;
>+            }
>+        }
>+
>         if (pn2->pn_type == TOK_FUNCTION) {
>             /*
>              * PNX_FUNCDEFS notifies the emitter that the block contains top-
>diff --git a/js/src/jsparse.h b/js/src/jsparse.h
>--- a/js/src/jsparse.h
>+++ b/js/src/jsparse.h
>@@ -478,6 +478,39 @@ struct JSParseNode {
>                (PN_TYPE(this) == TOK_PRIMARY && PN_OP(this) != JSOP_THIS);
>     }
> 
>+    /* 
>+     * True if this statement node could be a member of a Directive
>+     * Prologue.  Note that the prologue may contain strings that
>+     * cannot themselves be directives; that's a stricter test.
>+     * If Statement begins to simplify trees into this form, then
>+     * we'll need additional flags that we can test here.
>+     */
>+    bool isDirectivePrologueMember() const {
>+        if (PN_TYPE(this) == TOK_SEMI &&
>+            pn_arity == PN_UNARY) {
>+            JSParseNode *kid = pn_kid;
>+            return kid && PN_TYPE(kid) == TOK_STRING && !kid->pn_parens;
>+        }
>+        return false;
>+    }
>+
>+    /*
>+     * True if this node, known to be a Directive Prologue member,
>+     * could be a directive itself.
>+     */
>+    bool isDirective() const {
>+        JS_ASSERT(isDirectivePrologueMember());
>+        JSParseNode *kid = pn_kid;
>+        JSString *str = ATOM_TO_STRING(kid->pn_atom);
>+        /* 
>+         * Directives must contain no EscapeSequences or LineContinuations.
>+         * If the string's length in the source code is its length as a value,
>+         * accounting for the quotes, then it qualifies.
>+         */
>+        return (pn_pos.begin.lineno == pn_pos.end.lineno &&
>+                pn_pos.begin.index + str->length() + 2 == pn_pos.end.index);
>+    }
>+
>     /*
>      * Compute a pointer to the last element in a singly-linked list. NB: list
>      * must be non-empty for correct PN_LAST usage -- this is asserted!
>diff --git a/js/src/jsscan.h b/js/src/jsscan.h
>--- a/js/src/jsscan.h
>+++ b/js/src/jsscan.h
>@@ -345,6 +345,9 @@ struct JSTokenStream {
> /* Ignore keywords and return TOK_NAME instead to the parser. */
> #define TSF_KEYWORD_IS_NAME 0x4000
> 
>+/* Tokenize as appropriate for strict mode code.  */
>+#define TSF_STRICT_MODE_CODE 0x8000
>+
> /* Unicode separators that are treated as line terminators, in addition to \n, \r */
> #define LINE_SEPARATOR  0x2028
> #define PARA_SEPARATOR  0x2029
>diff --git a/js/src/jsscript.cpp b/js/src/jsscript.cpp
>--- a/js/src/jsscript.cpp
>+++ b/js/src/jsscript.cpp
>@@ -413,7 +413,7 @@ static const jsbytecode emptyScriptCode[
> 
> /* static */ const JSScript JSScript::emptyScriptConst = {
>     const_cast<jsbytecode*>(emptyScriptCode),
>-    1, JSVERSION_DEFAULT, 0, 0, 0, 0, 0, 1, 0, 0,
>+    1, JSVERSION_DEFAULT, 0, 0, 0, 0, 0, true, true, true, false,
>     const_cast<jsbytecode*>(emptyScriptCode),
>     {0, NULL}, NULL, 0, 0, 0, NULL
> };
>@@ -1639,6 +1639,8 @@ js_NewScriptFromCG(JSContext *cx, JSCode
>         script->noScriptRval = true;
>     if (cg->hasSharps())
>         script->hasSharps = true;
>+    if (cg->flags & TCF_STRICT_MODE_CODE)
>+        script->strictModeCode = true;
> 
>     if (cg->upvarList.count != 0) {
>         JS_ASSERT(cg->upvarList.count <= cg->upvarMap.length);
>diff --git a/js/src/jsscript.h b/js/src/jsscript.h
>--- a/js/src/jsscript.h
>+++ b/js/src/jsscript.h
>@@ -119,6 +119,7 @@ struct JSScript {
>                                        expression statement */
>     bool            savedCallerFun:1; /* object 0 is caller function */
>     bool            hasSharps:1;      /* script uses sharp variables */
>+    bool            strictModeCode:1;   /* code is in strict mode */
> 
>     jsbytecode      *main;      /* main entry point, after predef'ing prolog */
>     JSAtomMap       atomMap;    /* maps immediate index to literal struct */

Comment 40

[Jim Blandy :jimb]

Hi, Igor.  There were no comments interspersed with that patch text.

You mentioned r=you in IRC, after we talked about how it recognizes use strict directives in function code, but the patch isn't marked.

Comment 41

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/ad37dab7ea17

Comment 42

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e49b43757784

Comment 43

[Jim Blandy :jimb]

Last patch reverted: http://hg.mozilla.org/tracemonkey/rev/d7f6d69f6d25
Fixed and re-applied: http://hg.mozilla.org/tracemonkey/rev/c6e5543695e0