754181 - Don't store the strict mode code flag twice

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

Currently we store a strict mode flag in two places:  TokenStream, and
SharedContext.  (Bug 752758 introduced SharedContext;  it just holds some
stuff that used to be in TreeContext.)

This is annoying and error-prone.  In my mind the SharedContext is the right
place for the flag, since it stores info specific to each function and
global code.

So this patch removes the strict mode flag from TokenStream.  The flag is
needed during scanning, so I had to replace it with something.  That
something is StrictModeGetter, which is a back-channel to SharedContext that
is used just for getting the strict mode flag.  It doesn't expose anything
additional (e.g. SharedContext or Parser) to TokenStream.

It's a hack, I admit, but I like it better than storing and maintaining two
copies of the flag.  It also allows ReportStrictModeError to be greatly
simplified.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 623044 [details] [diff] [review]
patch

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

Ugh, this is a mess, and not one I think is much better than before, if any.  I don't much have better ideas, tho.  Punting to someone else to act as a touchstone on the idea here, because it smells all ugly but I can't quite muster up any passion to approve or disapprove of it.  :-\

::: js/src/frontend/Parser.cpp
@@ +108,5 @@
>          }                                                                                   \
>      JS_END_MACRO
>  #define MUST_MATCH_TOKEN(tt, errno) MUST_MATCH_TOKEN_WITH_FLAGS(tt, errno, 0)
>  
> +bool StrictModeGetter::get()

bool
StrictModeGetter::get()

@@ +2831,2 @@
>              return NULL;
>          }

No braces here.

::: js/src/frontend/TokenStream.h
@@ +488,5 @@
>       * This class uses JSContext.tempLifoAlloc to allocate internal buffers. The
>       * caller should JS_ARENA_MARK before calling |init| and JS_ARENA_RELEASE
>       * after calling |close|.
>       */
> +    TokenStream(JSContext *, JSPrincipals *principals, JSPrincipals *originPrincipals,

Add a "cx" while you're changing this, we don't omit parameter names.

@@ +894,5 @@
>  
>  /*
>   * Report a condition that should elicit a warning with JSOPTION_STRICT,
> + * or an error if the current context is handling strict mode code.  This
> + * function defers to ReportCompileErrorNumber to do the real work.

It doesn't seem necessary to explain how it defers -- that's an implementation detail.

::: js/src/frontend/TreeContext.h
@@ +70,1 @@
>      // JSREPORT_STRICT_ERROR.

Might as well also mention JSOPTION_STRICT_MODE here, too.

::: js/src/jsfun.cpp
@@ +1113,5 @@
>  
> +        /*
> +         * Initialize a tokenstream that reads from the given string.
> +         * No strictModeGetter is needed because this TokenStream won't report
> +         * any strict mode errors.

Add "Any strict mode errors which might be reported here (duplicate argument names, etc.) will be detected when we compile the function body (iff it is strict mode code, which we can discover only after parsing it." or something to that effect.  I'm thinking of cases like this:

js> Function("a, a", "'use strict'; return 2")
typein:1: SyntaxError: duplicate formal argument a:
typein:1: 'use strict'; return 2
typein:1: ^

Comment 2

[Nicholas Nethercote [inactive]]

An argument in favour of my change:  I'm about to land bug 757690, and I had to resolve some merge conflicts.  When JSOPTION_STRICT_MODE is present we have to set the strict mode flag in both places -- the SharedContext and the TokenStream.  While resolving the conflicts I very nearly omitted the setting of TokenStream's flag in that case.  And sure enough, the test added in bug 736792 only tested the setting of the SharedContext's strict mode flag, not TokenStream's flag!

TL;DR: storing the flag twice is error-prone.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 623044 [details] [diff] [review]
patch

The basic idea of StrictModeGetter makes sense: just give the TokenStream a pointer to the parser state, but give it a limited interface so that we make it is clear what subset of the Parser's state is observed by the TokenStream (which is unfortunately not the empty set).

Are there any other bits that could be unified in the same manner as strict mode (not in this bug, but possibly in the future)?  Because if so, it would seem aesthetically less hackish to slightly generalize what you have into:

class TokenizingContext {
    Parser *parser;
  public:
    bool inStrictMode() const;
    // future shared bits
};

Even if you don't foresee any future flags to unify, I'd much prefer this color of bikeshed.

Comment 4

[Nicholas Nethercote [inactive]]

Pushed with all nits picked:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ee09416b1d7

Comment 5

[Ryan VanderMeulen [:RyanVM]]

http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert

Backed out due to jsreftest crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ba9d5cbf23

https://tbpl.mozilla.org/php/getParsedLog.php?id=12044826&tree=Mozilla-Inbound

REFTEST TEST-START | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma_5/misc/future-reserved-words.js | 1571 / 3409 (46%)
++DOMWINDOW == 84 (0x4731a30) [serial = 2998] [outer = 0x2f20210]
497869: Implement FutureReservedWords per-spec
TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma_5/misc/future-reserved-words.js | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:05:40.164405
INFO | automation.py | Reading PID log: /tmp/tmp-TNDi3pidlog
Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-debug/1337902975/firefox-15.0a1.en-US.linux-x86_64.crashreporter-symbols.zip
PROCESS-CRASH | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma_5/misc/future-reserved-words.js | application crashed (minidump found)
Crash dump filename: /tmp/tmpoMGPxJ/minidumps/00c8c7aa-98ab-4cab-099e4ae6-5ff4632e.dmp
Operating system: Linux
                  0.0.0 Linux 2.6.31.5-127.fc12.x86_64 #1 SMP Sat Nov 7 21:11:14 EST 2009 x86_64
CPU: amd64
     family 6 model 23 stepping 10
     2 CPUs

Crash reason:  SIGSEGV
Crash address: 0x0

Thread 0 (crashed)
 0  libxul.so!js::PartialTokenizingContext::inStrictMode [Parser.cpp:2ee09416b1d7 : 82 + 0x0]
    rbx = 0xdc73dc00   r12 = 0xddd637f0   r13 = 0xdc73db30   r14 = 0xdc73dc50
    r15 = 0x0000000a   rip = 0xdcc49940   rsp = 0xdc73da68   rbp = 0xdc73dab0
    Found by: given as instruction pointer in context
 1  libxul.so!js::TokenStream::checkForKeyword [TokenStream.h:2ee09416b1d7 : 498 + 0xb]
    rbx = 0xdc73dc00   r12 = 0xddd637f0   r13 = 0xdc73db30   r14 = 0xdc73dc50
    r15 = 0x0000000a   rip = 0xdcc651e5   rsp = 0xdc73da70   rbp = 0xdc73dab0
    Found by: call frame info
 2  libxul.so!js::TokenStream::getTokenInternal [TokenStream.cpp:2ee09416b1d7 : 1493 + 0x8]
    rbx = 0xdc73dc00   r12 = 0x032db414   r13 = 0x00000000   r14 = 0x032db400
    r15 = 0xdd127040   rip = 0xdcc69fe5   rsp = 0xdc73dac0   rbp = 0xdc73db70
    Found by: call frame info
 3  libxul.so!js::Function [jsfun.cpp:2ee09416b1d7 : 1101 + 0x7]
    rbx = 0xc01f05a4   r12 = 0xdc73dc00   r13 = 0xc01f05a4   r14 = 0xc01f0590
    r15 = 0x032db414   rip = 0xdca7f665   rsp = 0xdc73db80   rbp = 0xdc73e130
    Found by: call frame info
 4  libxul.so!js::CallJSNative [jscntxtinlines.h:2ee09416b1d7 : 395 + 0x8]
    rbx = 0xd0454220   r12 = 0x02f20630   r13 = 0xdc73e2c0   r14 = 0xdca7f3b0
    r15 = 0xffffffff   rip = 0xdcac505f   rsp = 0xdc73e140   rbp = 0xdc73e1a0
    Found by: call frame info
 5  libxul.so!js::InvokeKernel [jsinterp.cpp:2ee09416b1d7 : 310 + 0xf]
    rbx = 0x00000000   r12 = 0x02f20630   r13 = 0x99cd30c0   r14 = 0x00000000
    r15 = 0xdddbc180   rip = 0xdcadec80   rsp = 0xdc73e1b0   rbp = 0xdc73e2b0
    Found by: call frame info
 6  libxul.so!js::Interpret [jsinterp.cpp:2ee09416b1d7 : 2512 + 0x2c]
    rbx = 0x00000000   r12 = 0xd0454198   r13 = 0x02f20630   r14 = 0x99b9ab68
    r15 = 0xdc73e3f0   rip = 0xdcad82d8   rsp = 0xdc73e2c0   rbp = 0xdc73ea90
    Found by: call frame info
 7  libxul.so!js::RunScript [jsinterp.cpp:2ee09416b1d7 : 266 + 0xc]
    rbx = 0x02f20630   r12 = 0x04711fc8   r13 = 0x99b9acb8   r14 = 0xd04540f8
    r15 = 0xdc73eb60   rip = 0xdcade575   rsp = 0xdc73eaa0   rbp = 0xdc73eae0
    Found by: call frame info
 8  libxul.so!js::InvokeKernel [jsinterp.cpp:2ee09416b1d7 : 326 + 0x12]
    rbx = 0x00000000   r12 = 0x02f20630   r13 = 0x99b0a700   r14 = 0x00000000
    r15 = 0xdc73eb60   rip = 0xdcaded56   rsp = 0xdc73eaf0   rbp = 0xdc73ebf0
    Found by: call frame info
 9  libxul.so!array_forEach [jsinterp.h:2ee09416b1d7 : 125 + 0x27]
    rbx = 0x00000000   r12 = 0x02f20630   r13 = 0xdc73ec70   r14 = 0xd04540a0
    r15 = 0x99b59080   rip = 0xdca2e170   rsp = 0xdc73ec00   rbp = 0xdc73ed50

Comment 6

[Nicholas Nethercote [inactive]]

It turns out that "we don't the back-channel when parsing function arguments" isn't true.  What is strange is that I was getting seg faults when I ran this test locally but jstests.py was eating them and saying the test passed.  Not confidence-inspiring!

Comment 7

[Nicholas Nethercote [inactive]]

> Are there any other bits that could be unified in the same manner as strict
> mode (not in this bug, but possibly in the future)?  Because if so, it would
> seem aesthetically less hackish to slightly generalize what you have into:
> 
> class TokenizingContext {
>     Parser *parser;
>   public:
>     bool inStrictMode() const;
>     // future shared bits
> };
> 
> Even if you don't foresee any future flags to unify, I'd much prefer this
> color of bikeshed.

I made this change and it was in the patch that was backed out.  But after thinking some more I'm not happy with it and would prefer the original StrictModeGetter version.  The reason is that I have a rule of thumb:  "don't generalize something until you have at least two instances of it".  (It's just a special case of YAGNI, really.)  And I don't foresee any future flags that will need this treatment.

Furthermore, there's this special NULL case in jsfun.cpp which is specific to strict mode and wouldn't generalize.

With that in mind, Luke, are you happy if I go back to StrictModeGetter?

Comment 8

[Luke Wagner [:luke]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> StrictModeGetter version.  The reason is that I have a rule of thumb: 
> "don't generalize something until you have at least two instances of it". 

I have the same rule but this isn't exactly "generalizing"; it's painting the bikeshed a less peculiar color.  Anyhow, I don't care that much, your patch, so you pick the color.

Comment 9

[Nicholas Nethercote [inactive]]

Second attempt:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cca745f94bf

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9cca745f94bf