Bug 182074 – JSC incorrectly interpreting script, sets Global Property instead of Global Lexical variable (LiteralParser / JSONP path)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

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.

[Mark Lam]

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).

[Radar WebKit Bug Importer]

<rdar://problem/36846261>

[Saam Barati]

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

[Joseph Pecoraro]

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`.

[Saam Barati]

patch forthcoming

[Saam Barati]

Created attachment 332671 [details]
patch

[Mark Lam]

Comment on attachment 332671 [details]
patch

r=me

[Joseph Pecoraro]

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.

[Saam Barati]

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

[Chris Dumez]

I Would expect a service worker test to need rebaselining.

[Chris Dumez]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

Created attachment 332702 [details]
patch for landing

[Saam Barati]

Created attachment 332704 [details]
patch for landing

[Saam Barati]

Created attachment 332706 [details]
patch for landing

[EWS Watchlist]

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

[Saam Barati]

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

[WebKit Commit Bot]

Comment on attachment 332706 [details]
patch for landing

Clearing flags on attachment: 332706

Committed r227898: <https://trac.webkit.org/changeset/227898>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.