Bug 182074 - JSC incorrectly interpreting script, sets Global Property instead of Global Lexical variable (LiteralParser / JSONP path)
Summary: JSC incorrectly interpreting script, sets Global Property instead of Global L...
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: 2018-01-24 16:40 PST by Chris Dumez
Modified: 2018-01-31 02:18 PST (History)
18 users (show)

See Also:


Attachments
patch (11.90 KB, patch)
2018-01-30 11:17 PST, Saam Barati
mark.lam: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.84 MB, application/zip)
2018-01-30 12:13 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews101 for mac-sierra (2.25 MB, application/zip)
2018-01-30 12:18 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (2.94 MB, application/zip)
2018-01-30 12:45 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.19 MB, application/zip)
2018-01-30 12:59 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews200 for win-future (11.65 MB, application/zip)
2018-01-30 13:35 PST, EWS Watchlist
no flags Details
patch for landing (14.68 KB, patch)
2018-01-30 14:13 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (14.82 KB, patch)
2018-01-30 14:15 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (61.26 KB, patch)
2018-01-30 14:20 PST, 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 Chris Dumez 2018-01-24 16:40:59 PST
I have a test.js file containing:
foo = "root"; bar=5

Then if I launch jsc interpreter:
>>> let foo = null
>>> load("test.js")
5
>>> foo
null // should be "root", not null.

The bug does not reproduce if you don't do "let foo = null" before loading the file. It also does not reproduce if you do "var foo = null" instead.

The bug does not reproduce on Firefox and Chrome.
Comment 1 Chris Dumez 2018-01-24 16:41:27 PST
This is causing imported/w3c/web-platform-tests/service-workers/service-worker/import-scripts-updated-flag.https.html layout test to fail. Mark helped me reduce the issue.
Comment 2 Mark Lam 2018-01-24 16:44:10 PST
I debugged this with Chris.  Interpreter::executeProgram() is interpreting the incoming source as JSON, and hence not executing it.

The bug also does not manifest if test.js contains '; foo = "root"; bar=5' (i.e. starts with a semicolon).
Comment 3 Radar WebKit Bug Importer 2018-01-24 16:46:16 PST
<rdar://problem/36846261>
Comment 4 Saam Barati 2018-01-24 17:00:46 PST
(In reply to Mark Lam from comment #2)
> I debugged this with Chris.  Interpreter::executeProgram() is interpreting
> the incoming source as JSON, and hence not executing it.
> 
> The bug also does not manifest if test.js contains '; foo = "root"; bar=5'
> (i.e. starts with a semicolon).

It seems weird that we think this is valid JSON. Any idea why?
Comment 5 Joseph Pecoraro 2018-01-24 17:18:52 PST
The test file can be reduced to just:

    foo = "test";

And in JSC:

    jsc> let foo = null;
    jsc> load("test.js")
    "root"

    jsc> foo // should be "root"
    null

    jsc> this.foo
    "root"

It looks like LiteralParser gathers a set of JSONP operations. For example:

    foo = "test"; bar=5

Would be something like:

    {
       type: JSONPPathEntryTypeDot,
       name: "foo",
       value: JSValue("test")
    }
    {
       type: JSONPPathEntryTypeDot,
       name: "bar",
       value: JSValue(5)
    }

And Interpreter::executeProgram attempts to apply the JSONPData operations. In this case treating the Dot like `global.foo = "test"` and `global.bar = 5`.

Though it uses the `globalObject` as the baseObject for assignments.

>     JSValue baseObject(globalObject);
>     for (unsigned i = 0; i < JSONPPath.size() - 1; i++) {
>         ASSERT(JSONPPath[i].m_type != JSONPPathEntryTypeDeclare);
>         switch (JSONPPath[i].m_type) {
>         case JSONPPathEntryTypeDot: {
>             if (i == 0) {
>                 PropertySlot slot(globalObject, PropertySlot::InternalMethodType::Get);
>                 if (!globalObject->getPropertySlot(callFrame, JSONPPath[i].m_pathEntryName, slot)) {
>                     RETURN_IF_EXCEPTION(throwScope, JSValue());
>                     if (entry)
>                         return throwException(callFrame, throwScope, createUndefinedVariableError(callFrame, JSONPPath[i].m_pathEntryName));
>                     goto failedJSONP;
>                 }
>                 baseObject = slot.getValue(callFrame, JSONPPath[i].m_pathEntryName);
>             } else
>                 baseObject = baseObject.get(callFrame, JSONPPath[i].m_pathEntryName);
>             RETURN_IF_EXCEPTION(throwScope, JSValue());
>             continue;
>         }

It seems like using the GlobalObject misses the GlobalLexicalEnvironment containing the `let` variables.

And indeed that is confirmed by `global.foo` being "root", but just `foo` being `null`.
Comment 6 Saam Barati 2018-01-30 10:43:25 PST
patch forthcoming
Comment 7 Saam Barati 2018-01-30 11:17:14 PST
Created attachment 332671 [details]
patch
Comment 8 Mark Lam 2018-01-30 11:34:22 PST
Comment on attachment 332671 [details]
patch

r=me
Comment 9 Joseph Pecoraro 2018-01-30 11:39:16 PST
Comment on attachment 332671 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332671&action=review

> JSTests/stress/jsonp-program-evaluate-path-must-consider-global-lexical-environment.js:10
> +let a = 22;
> +loadString("a = 42");
> +assert(a === 42);

Add a test with var? To ensure the global variable gets updated and no lexical variable creeps in? Something like:

    var c = 10;
    loadString("c = 20");
    assert(c === 20);
    assert(this.c === 20); // `this` being the global object

> Source/JavaScriptCore/interpreter/Interpreter.cpp:863
> +                        if (!result) {

I think this part would read easier re-using the pattern used above, to continue on a successful get:

    if (result) {
        baseObject = result;
        continue;
    }

    // The error case.
Comment 10 Saam Barati 2018-01-30 11:45:13 PST
Comment on attachment 332671 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332671&action=review

>> JSTests/stress/jsonp-program-evaluate-path-must-consider-global-lexical-environment.js:10
>> +assert(a === 42);
> 
> Add a test with var? To ensure the global variable gets updated and no lexical variable creeps in? Something like:
> 
>     var c = 10;
>     loadString("c = 20");
>     assert(c === 20);
>     assert(this.c === 20); // `this` being the global object

Makes sense to have a test for this too. Will add

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:863
>> +                        if (!result) {
> 
> I think this part would read easier re-using the pattern used above, to continue on a successful get:
> 
>     if (result) {
>         baseObject = result;
>         continue;
>     }
> 
>     // The error case.

sounds good
Comment 11 Chris Dumez 2018-01-30 11:55:01 PST
I Would expect a service worker test to need rebaselining.
Comment 12 Chris Dumez 2018-01-30 11:56:23 PST
(In reply to Chris Dumez from comment #11)
> I Would expect a service worker test to need rebaselining.

Yep, imported/w3c/web-platform-tests/service-workers/service-worker/import-scripts-updated-flag.https.html seems to be failing on EWS.
Comment 13 EWS Watchlist 2018-01-30 12:13:29 PST
Comment on attachment 332671 [details]
patch

Attachment 332671 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6263667

New failing tests:
http/tests/security/regress-52192.html
imported/w3c/web-platform-tests/service-workers/service-worker/import-scripts-updated-flag.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/navigation-redirect.https.html
Comment 14 EWS Watchlist 2018-01-30 12:13:30 PST
Created attachment 332679 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 15 EWS Watchlist 2018-01-30 12:18:48 PST
Comment on attachment 332671 [details]
patch

Attachment 332671 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6263783

New failing tests:
http/tests/security/regress-52192.html
Comment 16 EWS Watchlist 2018-01-30 12:18:50 PST
Created attachment 332681 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 17 EWS Watchlist 2018-01-30 12:45:05 PST
Comment on attachment 332671 [details]
patch

Attachment 332671 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6263808

New failing tests:
http/tests/security/regress-52192.html
Comment 18 EWS Watchlist 2018-01-30 12:45:07 PST
Created attachment 332688 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 19 EWS Watchlist 2018-01-30 12:59:05 PST
Comment on attachment 332671 [details]
patch

Attachment 332671 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6263990

New failing tests:
http/tests/security/regress-52192.html
imported/w3c/web-platform-tests/service-workers/service-worker/import-scripts-updated-flag.https.html
Comment 20 EWS Watchlist 2018-01-30 12:59:07 PST
Created attachment 332690 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 21 EWS Watchlist 2018-01-30 13:35:21 PST
Comment on attachment 332671 [details]
patch

Attachment 332671 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6264532

New failing tests:
http/tests/security/regress-52192.html
Comment 22 EWS Watchlist 2018-01-30 13:35:32 PST
Created attachment 332696 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 23 Saam Barati 2018-01-30 14:13:25 PST
Created attachment 332702 [details]
patch for landing
Comment 24 Saam Barati 2018-01-30 14:15:48 PST
Created attachment 332704 [details]
patch for landing
Comment 25 Saam Barati 2018-01-30 14:20:14 PST
Created attachment 332706 [details]
patch for landing
Comment 26 EWS Watchlist 2018-01-30 17:06:54 PST
Comment on attachment 332706 [details]
patch for landing

Attachment 332706 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6269238

New failing tests:
stress/ftl-get-by-id-getter-exception-interesting-live-state.js.ftl-eager
Comment 27 Saam Barati 2018-01-30 20:13:27 PST
(In reply to Build Bot from comment #26)
> Comment on attachment 332706 [details]
> patch for landing
> 
> Attachment 332706 [details] did not pass jsc-ews (mac):
> Output: http://webkit-queues.webkit.org/results/6269238
> 
> New failing tests:
> stress/ftl-get-by-id-getter-exception-interesting-live-state.js.ftl-eager

I can't reproduce this. It's likely a flaky test not related to this change since that test has nothing to do with JSONP at all.
Comment 28 WebKit Commit Bot 2018-01-31 02:18:33 PST
Comment on attachment 332706 [details]
patch for landing

Clearing flags on attachment: 332706

Committed r227898: <https://trac.webkit.org/changeset/227898>
Comment 29 WebKit Commit Bot 2018-01-31 02:18:35 PST
All reviewed patches have been landed.  Closing bug.