736792 - Add JSOPTION_STRICT_MODE for embedders to force strict mode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Andrew Paprocki]

As an embedder, I need a way to force the engine, at runtime, to parse all code in strict mode, without requiring script to include the "use strict"; directive.

I'm attaching a patch which adds a JSOPTION_USE_STRICT runtime option which, when enabled, automatically sets the strict mode flags where appropriate without requiring the parser to find "use strict"; in the directive prologue.

Comment 1

[Andrew Paprocki]

Attachment: Add JSOPTION_USE_STRICT for embedders

Asking jimb for review, as I see his name associated with the existing strict mode lines in the frontend.

Comment 2

[David Mandelin [:dmandelin]]

Comment on attachment 606916 [details] [diff] [review]
Add JSOPTION_USE_STRICT for embedders

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

Flipping review to Nick--he's about to overhaul the flag bits.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 606916 [details] [diff] [review]
Add JSOPTION_USE_STRICT for embedders

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

This is on the right track, but I'm giving r- because I'd like you to be able to turn this on via Spidermonkey's options() function, probably with a "strict_mode" option.  See js_options[] in shell/js.cpp for how to do this.

And then I'd like a simple test to ensure that works.  See js1_8_5/regress/regress-559402-1.js for a test of the existing JSOPTION_STRICT.  You'll need something similar.  Note that the options() call only takes effect within the eval, which is why eval is used.

::: js/src/frontend/BytecodeEmitter.h
@@ +372,5 @@
> +             * If the embedding has set JSOPTION_USE_STRICT, always set
> +             * the tree context into strict mode regardless of whether
> +             * the directive prologue contains "use strict".
> +             */
> +            flags |= TCF_STRICT_MODE_CODE;

I don't think this comment is necessary.

::: js/src/frontend/Parser.cpp
@@ +153,5 @@
> +        /*
> +         * If the embedding has set JSOPTION_USE_STRICT, always set
> +         * the token stream into strict mode regardless of whether
> +         * the directive prologue contains "use strict".
> +         */

Nor this one.

::: js/src/jsapi.h
@@ +2585,5 @@
>                                                     don't tune at run-time. */
>  #define JSOPTION_PCCOUNT        JS_BIT(17)      /* Collect per-op execution counts */
>  
>  #define JSOPTION_TYPE_INFERENCE JS_BIT(18)      /* Perform type inference. */
> +#define JSOPTION_USE_STRICT     JS_BIT(19)      /* Force "use strict" mode. */

But the comment here could be longer -- e.g. 'this provides a way to force ES5 strict mode for all code without requiring "use strict" annotations'.

Also, we have JSOPTION_STRICT already, which is for SpiderMonkey's own "strict option".  So JSOPTION_STRICT_MODE would be a clearer name, I think;  we use "strict mode" to denote the ES5 strict mode.

Comment 4

[Andrew Paprocki]

Attachment: Add JSOPTION_STRICT_MODE for embedders

Incorporated review comments, added option to shell and regression test.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 621334 [details] [diff] [review]
Add JSOPTION_STRICT_MODE for embedders

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

Looks good.  Do you have commit access?  I can land this for you if you don't.

Comment 6

[Andrew Paprocki]

No, I'd appreciate it if you could. Thanks

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Depending on his timeframe, I may beat him to it ;)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef39e03c9d9

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c3ad1222873c - jsreftest says (e.g. in https://tbpl.mozilla.org/php/getParsedLog.php?id=11504435&tree=Mozilla-Inbound) "js1_8_5/regress/regress-736792.js | Unknown :0: uncaught exception: Unsupported JSContext option 'strict_mode' item 1"

Comment 10

[Andrew Paprocki]

Is there something I'm missing here? That test can only fail if the code in this patch was not built into the shell used to run the tests.

Comment 11

[Andrew Paprocki]

Hmm.. it seems there are other places new options need to go?

Taking the "atline" option as an example:

/js/src/tests/browser.js (View Hg log or Hg annotations)
line 223 -- atline: true,
/js/xpconnect/idl/xpccomponents.idl (View Hg log or Hg annotations)
line 349 -- attribute boolean atline;
/js/xpconnect/shell/xpcshell.cpp (View Hg log or Hg annotations)
line 715 -- {"atline", JSOPTION_ATLINE},

Are there any others?

Comment 12

[Andrew Paprocki]

Attachment: Add JSOPTION_STRICT_MODE for embedders

Same patch, added missing bits to xpconnect, tests/browser.js and tested that reftests pass in-browser.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/?tree=Try&rev=60012376e0b5

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Second try...
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf078ed96b9

Comment 15

[Nicholas Nethercote [inactive]]

Sorry I didn't know about the XPConnect stuff!  Thanks.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Original landing and backout:
https://hg.mozilla.org/mozilla-central/rev/4ef39e03c9d9
https://hg.mozilla.org/mozilla-central/rev/c3ad1222873c

Comment 17

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/eaf078ed96b9

Comment 18

[Jeff Walden [:Waldo]]

Note that this means the semantics of a JS file, or a string of JS code, or whatever, will noticeably vary (and not always in fail-fast ways) depending on an externally-provided variable.  I guess we have JSOPTION_* which does this already, but I'm not entirely comfortable with that, or with doubling down on it and continuing it further.  Which isn't to say I'm entirely uncomfortable with it -- just that it makes me a bit leery.  Meh.