Last Comment Bug 736792 - Add JSOPTION_STRICT_MODE for embedders to force strict mode
: Add JSOPTION_STRICT_MODE for embedders to force strict mode
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla15
Assigned To: Andrew Paprocki
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-17 17:26 PDT by Andrew Paprocki
Modified: 2012-05-07 16:40 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add JSOPTION_USE_STRICT for embedders (3.37 KB, patch)
2012-03-17 17:28 PDT, Andrew Paprocki
n.nethercote: review-
Details | Diff | Splinter Review
Add JSOPTION_STRICT_MODE for embedders (4.59 KB, patch)
2012-05-05 12:30 PDT, Andrew Paprocki
n.nethercote: review+
Details | Diff | Splinter Review
Add JSOPTION_STRICT_MODE for embedders (7.54 KB, patch)
2012-05-06 11:46 PDT, Andrew Paprocki
andrew: review+
Details | Diff | Splinter Review

Description Andrew Paprocki 2012-03-17 17:26:52 PDT
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 2012-03-17 17:28:26 PDT
Created attachment 606916 [details] [diff] [review]
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] 2012-05-04 15:45:40 PDT
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 [:njn] 2012-05-04 16:24:22 PDT
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 2012-05-05 12:30:09 PDT
Created attachment 621334 [details] [diff] [review]
Add JSOPTION_STRICT_MODE for embedders

Incorporated review comments, added option to shell and regression test.
Comment 5 Nicholas Nethercote [:njn] 2012-05-05 14:34:20 PDT
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 2012-05-05 15:44:27 PDT
No, I'd appreciate it if you could. Thanks
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-05-05 16:11:29 PDT
Depending on his timeframe, I may beat him to it ;)
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-05-05 20:46:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef39e03c9d9
Comment 9 Phil Ringnalda (:philor) 2012-05-05 21:45:49 PDT
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 2012-05-05 23:20:04 PDT
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 2012-05-05 23:30:21 PDT
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 2012-05-06 11:46:01 PDT
Created attachment 621443 [details] [diff] [review]
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 (⌚ UTC+1/+2) 2012-05-06 13:05:20 PDT
https://tbpl.mozilla.org/?tree=Try&rev=60012376e0b5
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-05-06 13:11:33 PDT
Second try...
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf078ed96b9
Comment 15 Nicholas Nethercote [:njn] 2012-05-06 17:01:18 PDT
Sorry I didn't know about the XPConnect stuff!  Thanks.
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-05-06 17:34:37 PDT
Original landing and backout:
https://hg.mozilla.org/mozilla-central/rev/4ef39e03c9d9
https://hg.mozilla.org/mozilla-central/rev/c3ad1222873c
Comment 18 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-07 16:40:21 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.