RESOLVED FIXED 159376
our parsing for "use strict" is wrong when we first parse other directives that are not "use strict" but are located in a place where "use strict" would be valid
https://bugs.webkit.org/show_bug.cgi?id=159376
Summary our parsing for "use strict" is wrong when we first parse other directives th...
Saam Barati
Reported 2016-07-01 17:17:42 PDT
...
Attachments
patch (5.89 KB, patch)
2016-07-01 17:45 PDT, Saam Barati
no flags
patch (9.89 KB, patch)
2016-07-01 18:10 PDT, Saam Barati
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-yosemite (877.46 KB, application/zip)
2016-07-01 18:39 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.06 MB, application/zip)
2016-07-01 18:43 PDT, Build Bot
no flags
Archive of layout-test-results from ews125 for ios-simulator-wk2 (648.25 KB, application/zip)
2016-07-01 18:53 PDT, Build Bot
no flags
Archive of layout-test-results from ews113 for mac-yosemite (1.48 MB, application/zip)
2016-07-01 19:09 PDT, Build Bot
no flags
patch (5.38 KB, patch)
2016-07-02 17:03 PDT, Saam Barati
no flags
patch (5.36 KB, patch)
2016-07-02 17:05 PDT, Saam Barati
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-yosemite (306.94 KB, application/zip)
2016-07-02 17:39 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (346.16 KB, application/zip)
2016-07-02 17:41 PDT, Build Bot
no flags
Archive of layout-test-results from ews113 for mac-yosemite (323.34 KB, application/zip)
2016-07-02 17:42 PDT, Build Bot
no flags
Archive of layout-test-results from ews125 for ios-simulator-wk2 (304.89 KB, application/zip)
2016-07-02 17:47 PDT, Build Bot
no flags
patch (5.14 KB, patch)
2016-07-03 01:10 PDT, Saam Barati
benjamin: review+
patch for landing (7.52 KB, patch)
2016-07-05 12:09 PDT, Saam Barati
no flags
Saam Barati
Comment 1 2016-07-01 17:18:15 PDT
Saam Barati
Comment 2 2016-07-01 17:45:03 PDT
Saam Barati
Comment 3 2016-07-01 18:10:15 PDT
Created attachment 282622 [details] patch updated wrong layout tests
Saam Barati
Comment 4 2016-07-01 18:17:40 PDT
Comment on attachment 282622 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=282622&action=review > LayoutTests/js/script-tests/basic-strict-mode.js:111 > +shouldBe("'\\007'; 'use strict';", "'use strict'"); This change might be subtly wrong. I need to verify.
Saam Barati
Comment 5 2016-07-01 18:20:37 PDT
Comment on attachment 282622 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=282622&action=review >> LayoutTests/js/script-tests/basic-strict-mode.js:111 >> +shouldBe("'\\007'; 'use strict';", "'use strict'"); > > This change might be subtly wrong. I need to verify. So the behavior v8 and spider monkey implement are that they allow many successive directives to be declared before the use strict directive. This will make the code strict still. However, if there is an interleaving non-directive, they are not strict. I'll implement the same behavior: i.e, this foo is strict here: function foo() { "hello world"; "foo bar bas"; "use strict"; } and foo is not strict here: function foo() { "hello world"; function bar() { } "use strict"; }
Build Bot
Comment 6 2016-07-01 18:39:03 PDT
Comment on attachment 282622 [details] patch Attachment 282622 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1610225 New failing tests: js/mozilla/strict/directive-prologue-01.html
Build Bot
Comment 7 2016-07-01 18:39:07 PDT
Created attachment 282625 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 8 2016-07-01 18:43:21 PDT
Comment on attachment 282622 [details] patch Attachment 282622 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1610230 New failing tests: js/mozilla/strict/directive-prologue-01.html
Build Bot
Comment 9 2016-07-01 18:43:24 PDT
Created attachment 282626 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 10 2016-07-01 18:53:11 PDT
Comment on attachment 282622 [details] patch Attachment 282622 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1610243 New failing tests: js/mozilla/strict/directive-prologue-01.html
Build Bot
Comment 11 2016-07-01 18:53:15 PDT
Created attachment 282630 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Build Bot
Comment 12 2016-07-01 19:09:43 PDT
Comment on attachment 282622 [details] patch Attachment 282622 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1610319 New failing tests: js/mozilla/strict/directive-prologue-01.html
Build Bot
Comment 13 2016-07-01 19:09:47 PDT
Created attachment 282631 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Oliver Hunt
Comment 14 2016-07-01 20:12:18 PDT
The correct (spec) behaviour is to allow an arbitrarily long list of string literals before the "use strict" declaration. What's the bug here? (You need to include the actual bug in the report :D )
Saam Barati
Comment 15 2016-07-01 21:59:28 PDT
(In reply to comment #14) > The correct (spec) behaviour is to allow an arbitrarily long list of string > literals before the "use strict" declaration. Correct. Yeah, I realized this after I posted my last patch. I'll upload a correct patch soon. > > What's the bug here? (You need to include the actual bug in the report :D ) The test case is the easiest way to see the bug, but I'll give a better description: If a function starts with a non "use strict" string literal, we will allow "use strict" to be in any arbitrary statement inside the top level block in the function body. For example, this means that you can have a list of string literal, followed by arbitrary statements, followed by "use strict", and we will parse the function as if it's in strict mode. This is obviously the wrong behavior. This has consequences in other ways that break invariants of the language. For example, we currently allow functions that are lexically nested inside a strict function to be non-strict. This will fire an assertion if we ever skip over that function using the source provider cache, but it will work just fine in release builds.
Saam Barati
Comment 16 2016-07-02 17:03:14 PDT
Saam Barati
Comment 17 2016-07-02 17:05:14 PDT
Created attachment 282648 [details] patch changed the changelog a bit
Build Bot
Comment 18 2016-07-02 17:39:50 PDT
Comment on attachment 282648 [details] patch Attachment 282648 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1615783 Number of test failures exceeded the failure limit.
Build Bot
Comment 19 2016-07-02 17:39:54 PDT
Created attachment 282649 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 20 2016-07-02 17:41:01 PDT
Comment on attachment 282648 [details] patch Attachment 282648 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1615781 Number of test failures exceeded the failure limit.
Build Bot
Comment 21 2016-07-02 17:41:05 PDT
Created attachment 282650 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 22 2016-07-02 17:42:10 PDT
Comment on attachment 282648 [details] patch Attachment 282648 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1615772 Number of test failures exceeded the failure limit.
Build Bot
Comment 23 2016-07-02 17:42:14 PDT
Created attachment 282651 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 24 2016-07-02 17:47:08 PDT
Comment on attachment 282648 [details] patch Attachment 282648 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1615787 Number of test failures exceeded the failure limit.
Build Bot
Comment 25 2016-07-02 17:47:11 PDT
Created attachment 282652 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Saam Barati
Comment 26 2016-07-03 01:10:09 PDT
Benjamin Poulain
Comment 27 2016-07-03 19:42:42 PDT
Comment on attachment 282656 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=282656&action=review What a silly bug :) Good catch! > Source/JavaScriptCore/ChangeLog:15 > + the function body. For example, this meant that if had a sequence of string "if had" > Source/JavaScriptCore/ChangeLog:21 > + For example, we used to allow functions that are lexically nested inside what we deemed > + a strict function to be non-strict. This used to fire an assertion if we ever skipped over > + that function using the source provider cache, but it worked just fine in release builds. No test coverage for this? > Source/JavaScriptCore/parser/Parser.cpp:407 > if (directive) { I assume a "directive" in this context is any statement that is a constant string literal? Do we have test coverage with several directives in front of "use strict" to make sure this case still works? > Source/JavaScriptCore/parser/Parser.cpp:409 > + if (directiveLiteralLength == lengthOfUseStrictLiteral && m_vm->propertyNames->useStrictIdentifier == *directive) { I wonder why directiveLiteralLength is important since Identifiers are interned.
Saam Barati
Comment 28 2016-07-05 11:43:10 PDT
Comment on attachment 282656 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=282656&action=review >> Source/JavaScriptCore/ChangeLog:21 >> + that function using the source provider cache, but it worked just fine in release builds. > > No test coverage for this? The test I am landing covers this. We crash in that test in ToT debug JSC because the "bar" function is non-strict but "foo" is strict. I'll add one more test that makes this more explicit by actually calling the inner function. >> Source/JavaScriptCore/parser/Parser.cpp:407 >> if (directive) { > > I assume a "directive" in this context is any statement that is a constant string literal? > > Do we have test coverage with several directives in front of "use strict" to make sure this case still works? Yeah, we do. I know because I broke those tests before realizing that this is the specced behavior. >> Source/JavaScriptCore/parser/Parser.cpp:409 >> + if (directiveLiteralLength == lengthOfUseStrictLiteral && m_vm->propertyNames->useStrictIdentifier == *directive) { > > I wonder why directiveLiteralLength is important since Identifiers are interned. Good point. I wont change this now, but I suspect this code might have been here before we had UniquedStringImpl? Not quite sure. Oliver, do you know?
Saam Barati
Comment 29 2016-07-05 12:09:12 PDT
Created attachment 282803 [details] patch for landing
WebKit Commit Bot
Comment 30 2016-07-05 13:05:36 PDT
Comment on attachment 282803 [details] patch for landing Clearing flags on attachment: 282803 Committed r202828: <http://trac.webkit.org/changeset/202828>
WebKit Commit Bot
Comment 31 2016-07-05 13:05:42 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.