Bug 10701 - [ES5] Implement strict mode
: [ES5] Implement strict mode
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: All All
: P3 Normal
Assigned To:
: http://ejohn.org/blog/ecmascript-5-st...
: ES5
: 34019
:
  Show dependency treegraph
 
Reported: 2006-09-03 00:25 PST by
Modified: 2010-10-11 14:29 PST (History)


Attachments
Patch (189.73 KB, patch)
2010-10-01 16:52 PST, Oliver Hunt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (191.67 KB, patch)
2010-10-01 18:32 PST, Oliver Hunt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (195.52 KB, patch)
2010-10-02 12:31 PST, Oliver Hunt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (195.99 KB, patch)
2010-10-09 17:10 PST, Oliver Hunt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (199.20 KB, patch)
2010-10-10 18:16 PST, Oliver Hunt
barraclough: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2006-09-03 00:25:52 PST
JavaScript "strict-mode" options for easier debugging

A strict-mode could include such features as:
1. require 'var' for local variables (implicit globals cause warnings)
2. getting/setting custom properties on built-in types (catches things like for (var x = 0; x < myarray.size; x++) -- here Array.size does not exist, yet x < undefined is always false and silently fails)
I imagine others might have additional suggestions.

IMO these type of features could really set our javascript debugger apart from the pack.
------- Comment #1 From 2006-09-03 01:03:07 PST -------
It's not clear to me how this relates to Drosera.  It seems more like something that hooks into JSCore.  Drosera could be one possible UI for this, but the core of the work would likely happen at lower levels.
------- Comment #2 From 2009-12-07 13:07:15 PST -------
JavaScriptCore should implement the ES5 "use strict" behaviour.  Suggest we rename this bug reflecting that.  Or create a new one, and close this one as WONTFIX.
------- Comment #3 From 2010-05-26 13:04:43 PST -------
Renaming per suggestion from Patrick Mueller, to focus on ECMA 5 script mode:

http://ejohn.org/blog/ecmascript-5-strict-mode-json-and-more/

Bumped up to P3, since this is now part of the standard.
------- Comment #4 From 2010-05-26 13:25:55 PST -------
Section 4.2.2 in ECMA 262 Edition 5
------- Comment #5 From 2010-10-01 16:52:38 PST -------
Created an attachment (id=69540) [details]
Patch
------- Comment #6 From 2010-10-01 17:42:47 PST -------
Attachment 69540 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4208060
------- Comment #7 From 2010-10-01 17:50:58 PST -------
Attachment 69540 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4196048
------- Comment #8 From 2010-10-01 18:32:51 PST -------
Created an attachment (id=69558) [details]
Patch
------- Comment #9 From 2010-10-01 18:33:26 PST -------
Now it should actually build on 32bit (oops)
------- Comment #10 From 2010-10-01 19:58:27 PST -------
Attachment 69540 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4219054
------- Comment #11 From 2010-10-02 12:31:47 PST -------
Created an attachment (id=69578) [details]
Patch

Updated test to cover a few more cases, and added missing branches in the interpreter impl.
------- Comment #12 From 2010-10-02 12:36:02 PST -------
Attachment 69578 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/interpreter/Interpreter.cpp:3086:  vm_throw is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/interpreter/Interpreter.cpp:3278:  vm_throw is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 65 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #13 From 2010-10-02 13:45:03 PST -------
Attachment 69578 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4204047
------- Comment #14 From 2010-10-02 14:42:48 PST -------
I've removed the Platform.h change locally :D
------- Comment #15 From 2010-10-03 12:49:12 PST -------
(From update of attachment 69578 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=69578&action=review

> JavaScriptCore/bytecode/CodeBlock.h:557
> +        bool m_isStrictMode;

I think this would read better as an enum. That way we are not passing around random bools.

> JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1553
> +    if (m_codeBlock->isStrictMode())
> +        return;

Since this is a little subtle, a comment indicating why you are returning early here might be nice.

> JavaScriptCore/parser/ASTBuilder.h:583
> +    bool isResolve(ExpressionNode* expr) { return expr->isResolveNode(); }
>  private:

It is nice to have new line before the access control modifier.

> JavaScriptCore/runtime/StrictEvalActivation.h:42
> +}
> +

We like to put // namespace JSC at the end.


I also, think this patch could use a more detailed ChangeLog, explaining most of the changes.
------- Comment #16 From 2010-10-05 12:20:13 PST -------
(From update of attachment 69578 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=69578&action=review

> JavaScriptCore/interpreter/Interpreter.cpp:378
> +        // FIXME: We can use the preparser in strict mode, we just need additional logic

Would be really great to file a bug for this, and reference the bug number in the comment!

> JavaScriptCore/parser/JSParser.cpp:259
> +        void pushLabel(const Identifier* label)

probably should have a newline before function definition.

> JavaScriptCore/parser/JSParser.cpp:265
> +        void popLabel()

probably should have a newline before function definition.

> JavaScriptCore/parser/JSParser.cpp:271
> +        bool hasLabel(const Identifier* label)

probably should have a newline before function definition.

> JavaScriptCore/runtime/Arguments.cpp:203
> +        createStrictModeCalleeIfNecessary(exec);

You seem to be checking d->overrodeCallee twice?
Since we only createStrictModeCalleeIfNecessary this if !d->overrodeCallee, we should only need an ASSERT within the function?

Agreed to all Sam's comments.
Three more issues:

(1) failIfStrictTrue/failIfStrictFalse.

I find these names a little confusing.  Strict is usually a modifier to the thing it precedes, e.g. "strict equal".  I think something like "strictModeFailIfFalse" or "failIfFalseIfStrict" would parse in a more understandably fashion for me.

(2) Performance.

Given the size of this change and the additional parameterization in all the 'put' methods I think this bug really needs before and after SunSpidey & v8 numbers.  (We should also probably also have numbers for the interpreter - maybe just for SunSpider - to at least be aware in advance of any impact there).

(3) Passing exec through reparseExceptionInfo/parse/jsParse/parseProgram.

We really shouldn't be pushing a pointer into the JS Stack this deep into the parser – and we really shouldn't need to.  We want to be moving in the other direction – paring back our use of ExecState, to places where we may actually trigger new execution.  It looks like you've passed the exec state to parseProgram because it needs to check for the presence of certain properties on the LGO? If so, we should have an appropriate hasProperty method that does not require an exec state (and if we don't have one, I'd think you should be able to add one that just wraps the getPropertySlot that you're calling, passing the globalExec from the LGO).  Did I miss a use of ExecState that really requires a JS stack? – if not, I think we need to revert this.

r- for the JSGlobalData* -> ExecState* change, that makes me too sad. :'-( :-P

All looks great otherwise!
G.
------- Comment #17 From 2010-10-09 17:10:11 PST -------
Created an attachment (id=70380) [details]
Patch
------- Comment #18 From 2010-10-09 17:14:35 PST -------
Attachment 70380 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/interpreter/Interpreter.cpp:3128:  vm_throw is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/interpreter/Interpreter.cpp:3320:  vm_throw is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #19 From 2010-10-09 18:05:57 PST -------
Fixed all the issues you and sam pointed out.

> (1) failIfStrictTrue/failIfStrictFalse.
> 
> I find these names a little confusing.  Strict is usually a modifier to the thing it precedes, e.g. "strict equal".  I think something like "strictModeFailIfFalse" or "failIfFalseIfStrict" would parse in a more understandably fashion for me.

renamed to the IfStrict suffix variant you suggested

> 
> (2) Performance.
> 
> Given the size of this change and the additional parameterization in all the 'put' methods I think this bug really needs before and after SunSpidey & v8 numbers.  (We should also probably also have numbers for the interpreter - maybe just for SunSpider - to at least be aware in advance of any impact there).

** TOTAL **:           ??                327.0ms +/- 0.3%   327.7ms +/- 0.2% 

> 
> (3) Passing exec through reparseExceptionInfo/parse/jsParse/parseProgram.
> 
> We really shouldn't be pushing a pointer into the JS Stack this deep into the parser – and we really shouldn't need to.  We want to be moving in the other direction – paring back our use of ExecState, to places where we may actually trigger new execution.  It looks like you've passed the exec state to parseProgram because it needs to check for the presence of certain properties on the LGO? If so, we should have an appropriate hasProperty method that does not require an exec state (and if we don't have one, I'd think you should be able to add one that just wraps the getPropertySlot that you're calling, passing the globalExec from the LGO).  Did I miss a use of ExecState that really requires a JS stack? – if not, I think we need to revert this.

Avoid passing an execstate to getOwnPropertySlot would grossly inflate the size of this patch as every class that overrides getOwnPropertySlot would need to have an implementation of the non-execstate taking hasOwnProperty (or whatever).  While I agree that in an ideal world we wouldn't have this execstate, i can't see much of an alternative at this time.
------- Comment #20 From 2010-10-10 18:16:48 PST -------
Created an attachment (id=70418) [details]
Patch
------- Comment #21 From 2010-10-10 18:19:49 PST -------
Attachment 70418 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/interpreter/Interpreter.cpp:3128:  vm_throw is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/interpreter/Interpreter.cpp:3320:  vm_throw is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #22 From 2010-10-11 12:12:48 PST -------
Committed r69516: <http://trac.webkit.org/changeset/69516>
------- Comment #23 From 2010-10-11 14:29:38 PST -------
http://trac.webkit.org/changeset/69516 might have broken GTK Linux 32-bit Release
The following tests are not passing:
fast/js/basic-strict-mode.html