WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(13)
View All
Add attachment
proposed patch, testcase, etc.
Saam Barati
Comment 1
2016-07-01 17:18:15 PDT
<
rdar://problem/27108773
>
Saam Barati
Comment 2
2016-07-01 17:45:03 PDT
Created
attachment 282617
[details]
patch
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
Created
attachment 282647
[details]
patch
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
Created
attachment 282656
[details]
patch
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.
Top of Page
Format For Printing
XML
Clone This Bug