...
<rdar://problem/27108773>
Created attachment 282617 [details] patch
Created attachment 282622 [details] patch updated wrong layout tests
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.
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"; }
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
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
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
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
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
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
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
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
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 )
(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.
Created attachment 282647 [details] patch
Created attachment 282648 [details] patch changed the changelog a bit
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.
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
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.
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
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.
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
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.
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
Created attachment 282656 [details] patch
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.
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?
Created attachment 282803 [details] patch for landing
Comment on attachment 282803 [details] patch for landing Clearing flags on attachment: 282803 Committed r202828: <http://trac.webkit.org/changeset/202828>
All reviewed patches have been landed. Closing bug.