Bug 182074

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.

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

<rdar://problem/36846261>

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

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

patch forthcoming

Created attachment 332671 [details] patch

Comment on attachment 332671 [details] patch r=me

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

I Would expect a service worker test to need rebaselining.

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

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

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

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

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

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

Created attachment 332702 [details] patch for landing

Created attachment 332704 [details] patch for landing

Created attachment 332706 [details] patch for landing

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

(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 on attachment 332706 [details] patch for landing Clearing flags on attachment: 332706 Committed r227898: <https://trac.webkit.org/changeset/227898>

All reviewed patches have been landed. Closing bug.