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

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

<rdar://problem/27108773>

[Saam Barati]

Created attachment 282617 [details]
patch

[Saam Barati]

Created attachment 282622 [details]
patch

updated wrong layout tests

[Saam Barati]

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 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 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]

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 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]

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 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]

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 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]

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]

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]

(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]

Created attachment 282647 [details]
patch

[Saam Barati]

Created attachment 282648 [details]
patch

changed the changelog a bit

[Build Bot]

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]

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 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]

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 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]

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 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]

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]

Created attachment 282656 [details]
patch

[Benjamin Poulain]

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 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]

Created attachment 282803 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 282803 [details]
patch for landing

Clearing flags on attachment: 282803

Committed r202828: <http://trac.webkit.org/changeset/202828>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.