Bug 130554 - toThis() on a JSWorkerGlobalScope should return a JSProxy and not undefined
Summary: toThis() on a JSWorkerGlobalScope should return a JSProxy and not undefined
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL: https://developer.mozilla.org/en-US/d...
Keywords: InRadar
Depends on: 130553
Blocks:
  Show dependency treegraph
 
Reported: 2014-03-20 18:40 PDT by Michael Saboff
Modified: 2014-03-28 11:14 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.32 KB, patch)
2014-03-20 21:32 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff
Patch for Landing (12.17 KB, patch)
2014-03-21 15:21 PDT, Michael Saboff
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (548.21 KB, application/zip)
2014-03-21 16:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (512.83 KB, application/zip)
2014-03-21 17:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (626.21 KB, application/zip)
2014-03-21 17:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (621.45 KB, application/zip)
2014-03-21 18:06 PDT, Build Bot
no flags Details
Updated patch to fix test failures (13.63 KB, patch)
2014-03-22 13:48 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2014-03-20 18:40:49 PDT
This is the root cause for a radar defect <rdar://problem/15940627> where we can't render the website https://developer.mozilla.org/en-US/demos/detail/zen-photon-garden/launch.  The website uses workers to do rendering.  The script file is basically of the format of:

"use strict";

(function () {
    var locals.....

    this.myFunc1 = function () { ... }
    this.myFunc2 = function () { ... }
}).call(this);

Due to the string mode, when we process the to_this byte code, we return undefined per strict mode.  We then try to set "myFunc1" on undefined and throw an exception.  Instead, the worker's WorkerGlobalScope should have a proxy that we can set attributes on.

We'll do this based on the work for https://bugs.webkit.org/show_bug.cgi?id=130553
Comment 1 Michael Saboff 2014-03-20 21:32:02 PDT
Created attachment 227382 [details]
Patch

This patch depends on <https://bugs.webkit.org/show_bug.cgi?id=130553> - "Change CodeGeneratorJS.pm special cases for "DOMWindow" to be general purpose" and won't build without it.  Therefore the EWS bots will fail trying to build.  I can resubmit this patch for testing after 130553 lands.
Comment 2 Geoffrey Garen 2014-03-20 23:22:48 PDT
Comment on attachment 227382 [details]
Patch

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

r=me with a bug fix below, and please fix the build before landing.

> Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:134
> +    if (classInfo == JSProxy::info())
> +        return static_cast<JSDedicatedWorkerGlobalScope*>(jsCast<JSProxy*>(asObject(value))->target());

This needs to be jsDynamicCast<JSDedicatedWorkerGlobalScope*> because it's not guaranteed that all proxies proxy to JSDedicatedWorkerGlobalScope.
Comment 3 Michael Saboff 2014-03-21 13:33:56 PDT
This patch needs some more work as some jsregress tests fail because of it.
Comment 4 Michael Saboff 2014-03-21 15:21:14 PDT
Created attachment 227498 [details]
Patch for Landing

Already reviewed by Geoff.

Made two small fixes to reviewed patch:
 1. Eliminated ASSERT in testapi.c that the object used to create the GlobalObject is the same as the result from JSContextGetGlobalObject(context).  It is now a proxy to the original object.
 2. Move the call to setGlobalThis() in both JSGlobalObject::finishCreation() to after init has been called so our prototype can be created before we create the JSProxy that needs the prototype.

These changes rubber stamped by Geoff.
Comment 5 Build Bot 2014-03-21 16:36:18 PDT
Comment on attachment 227498 [details]
Patch for Landing

Attachment 227498 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5082087598063616

New failing tests:
media/adopt-node-crash.html
js/dom/global-constructors-attributes-shared-worker.html
js/dom/global-constructors-attributes-dedicated-worker.html
Comment 6 Build Bot 2014-03-21 16:36:22 PDT
Created attachment 227510 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 7 Build Bot 2014-03-21 17:15:22 PDT
Comment on attachment 227498 [details]
Patch for Landing

Attachment 227498 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4974679055925248

New failing tests:
js/dom/global-constructors-attributes-shared-worker.html
js/dom/global-constructors-attributes-dedicated-worker.html
Comment 8 Build Bot 2014-03-21 17:15:27 PDT
Created attachment 227513 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 9 Build Bot 2014-03-21 17:25:01 PDT
Comment on attachment 227498 [details]
Patch for Landing

Attachment 227498 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6342983427293184

New failing tests:
js/dom/global-constructors-attributes-shared-worker.html
js/dom/global-constructors-attributes-dedicated-worker.html
Comment 10 Build Bot 2014-03-21 17:25:05 PDT
Created attachment 227514 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 11 Build Bot 2014-03-21 18:06:41 PDT
Comment on attachment 227498 [details]
Patch for Landing

Attachment 227498 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4779732637843456

New failing tests:
js/dom/global-constructors-attributes-shared-worker.html
js/dom/global-constructors-attributes-dedicated-worker.html
Comment 12 Build Bot 2014-03-21 18:06:47 PDT
Created attachment 227519 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 13 Michael Saboff 2014-03-22 13:48:30 PDT
Created attachment 227568 [details]
Updated patch to fix test failures

Expect this to fix js/dom/global-constructors-attributes-dedicated-worker.html and js/dom/global-constructors-attributes-shared-worker.html.
Comment 14 Michael Saboff 2014-03-22 16:07:22 PDT
Committed r166126: <http://trac.webkit.org/changeset/166126>
Comment 15 Michael Saboff 2014-03-25 13:28:22 PDT
<rdar://problem/15940627>
Comment 16 Michael Saboff 2014-03-26 10:29:28 PDT
Change rolled out in <http://trac.webkit.org/changeset/166248> due to page load time performance regression.
Comment 17 Michael Saboff 2014-03-28 11:14:41 PDT
Relanded in change set r166415 - <http://trac.webkit.org/changeset/166415>