Bug 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
Summary: our parsing for "use strict" is wrong when we first parse other directives th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-01 17:17 PDT by Saam Barati
Modified: 2016-07-05 13:05 PDT (History)
14 users (show)

See Also:


Attachments
patch (5.89 KB, patch)
2016-07-01 17:45 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (9.89 KB, patch)
2016-07-01 18:10 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details
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 Details
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 Details
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 Details
patch (5.38 KB, patch)
2016-07-02 17:03 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (5.36 KB, patch)
2016-07-02 17:05 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details
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 Details
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 Details
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 Details
patch (5.14 KB, patch)
2016-07-03 01:10 PDT, Saam Barati
benjamin: review+
Details | Formatted Diff | Diff
patch for landing (7.52 KB, patch)
2016-07-05 12:09 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-07-01 17:17:42 PDT
...
Comment 1 Saam Barati 2016-07-01 17:18:15 PDT
<rdar://problem/27108773>
Comment 2 Saam Barati 2016-07-01 17:45:03 PDT
Created attachment 282617 [details]
patch
Comment 3 Saam Barati 2016-07-01 18:10:15 PDT
Created attachment 282622 [details]
patch

updated wrong layout tests
Comment 4 Saam Barati 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.
Comment 5 Saam Barati 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";
}
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Build Bot 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
Comment 12 Build Bot 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
Comment 13 Build Bot 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
Comment 14 Oliver Hunt 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 )
Comment 15 Saam Barati 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.
Comment 16 Saam Barati 2016-07-02 17:03:14 PDT
Created attachment 282647 [details]
patch
Comment 17 Saam Barati 2016-07-02 17:05:14 PDT
Created attachment 282648 [details]
patch

changed the changelog a bit
Comment 18 Build Bot 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.
Comment 19 Build Bot 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
Comment 20 Build Bot 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.
Comment 21 Build Bot 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
Comment 22 Build Bot 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.
Comment 23 Build Bot 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
Comment 24 Build Bot 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.
Comment 25 Build Bot 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
Comment 26 Saam Barati 2016-07-03 01:10:09 PDT
Created attachment 282656 [details]
patch
Comment 27 Benjamin Poulain 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.
Comment 28 Saam Barati 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?
Comment 29 Saam Barati 2016-07-05 12:09:12 PDT
Created attachment 282803 [details]
patch for landing
Comment 30 WebKit Commit Bot 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>
Comment 31 WebKit Commit Bot 2016-07-05 13:05:42 PDT
All reviewed patches have been landed.  Closing bug.