Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add JSOPTION_STRICT_MODE for embedders to force strict mode

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Andrew Paprocki, Assigned: Andrew Paprocki)

Tracking

Trunk
mozilla15
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Attachment #606916 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #606916 - Flags: review? → review?(jimb)
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.
Attachment #606916 - Flags: review?(jimb) → review?(n.nethercote)
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.
Attachment #606916 - Flags: review?(n.nethercote) → review-
(Assignee)

Comment 4

5 years ago
Created attachment 621334 [details] [diff] [review]
Add JSOPTION_STRICT_MODE for embedders

Incorporated review comments, added option to shell and regression test.
Attachment #606916 - Attachment is obsolete: true
Attachment #621334 - Flags: review?(n.nethercote)
(Assignee)

Updated

5 years ago
Summary: Add JSOPTION_USE_STRICT for embedders to force strict mode → Add JSOPTION_STRICT_MODE for embedders to force strict mode
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.
Attachment #621334 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 6

5 years ago
No, I'd appreciate it if you could. Thanks
Depending on his timeframe, I may beat him to it ;)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef39e03c9d9
Assignee: general → andrew
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla15
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"
Target Milestone: mozilla15 → ---
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
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?
(Assignee)

Comment 12

5 years ago
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.
Attachment #621334 - Attachment is obsolete: true
Attachment #621443 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=60012376e0b5
Second try...
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf078ed96b9
Keywords: checkin-needed
Target Milestone: --- → mozilla15
Sorry I didn't know about the XPConnect stuff!  Thanks.
Original landing and backout:
https://hg.mozilla.org/mozilla-central/rev/4ef39e03c9d9
https://hg.mozilla.org/mozilla-central/rev/c3ad1222873c
https://hg.mozilla.org/mozilla-central/rev/eaf078ed96b9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.