Bug 154170 - Window and WorkerGlobalScope should inherit EventTarget
Summary: Window and WorkerGlobalScope should inherit EventTarget
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar, WebExposed
Depends on: 154121 154245 154547
Blocks: 154171
  Show dependency treegraph
 
Reported: 2016-02-12 08:59 PST by Chris Dumez
Modified: 2016-02-22 12:23 PST (History)
13 users (show)

See Also:


Attachments
WIP Patch (21.40 KB, patch)
2016-02-12 16:38 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (21.48 KB, patch)
2016-02-12 16:40 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (97.33 KB, patch)
2016-02-12 17:16 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (99.15 KB, patch)
2016-02-12 20:45 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (101.27 KB, patch)
2016-02-14 18:14 PST, Chris Dumez
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 2016-02-12 08:59:41 PST
Window and WorkerGlobalScope should inherit EventTarget instead of duplicating its API.
Comment 1 Chris Dumez 2016-02-12 16:38:09 PST
Created attachment 271245 [details]
WIP Patch
Comment 2 Chris Dumez 2016-02-12 16:40:21 PST
Created attachment 271246 [details]
WIP Patch
Comment 3 Chris Dumez 2016-02-12 17:16:15 PST
Created attachment 271251 [details]
Patch
Comment 4 Chris Dumez 2016-02-12 20:45:18 PST
Created attachment 271267 [details]
Patch
Comment 5 Radar WebKit Bug Importer 2016-02-12 20:47:44 PST
<rdar://problem/24642377>
Comment 6 Darin Adler 2016-02-14 14:36:05 PST
Comment on attachment 271267 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +        * bindings/js/JSDOMWindowCustom.cpp:
> +        Drop custom bindings for Window's addEventListener() and
> +        removeEventListener(). The only reason these needed custom
> +        code was to add a check for frameless windows. The frameless
> +        Window checks was moved to the respective methods in the
> +        JSEventTarget generated bindings.

Which test covers the frameless window check?

> Source/WebCore/bindings/js/JSEventTargetCustom.cpp:84
> +    if (auto* jsEventTarget = JSC::jsDynamicCast<JSEventTarget*>(thisValue))
> +        return std::make_unique<JSEventTargetOrGlobalScope>(jsEventTarget->wrapped(), *jsEventTarget);
> +    if (auto* jsDOMWindow = toJSDOMWindow(thisValue))
> +        return std::make_unique<JSEventTargetOrGlobalScope>(jsDOMWindow->wrapped(), *jsDOMWindow);
> +    if (auto* jsWorkerGlobalScope = toJSWorkerGlobalScope(thisValue))
> +        return std::make_unique<JSEventTargetOrGlobalScope>(jsWorkerGlobalScope->wrapped(), *jsWorkerGlobalScope);

I don’t think these "js" prefixes are needed for the local variable names. I suggest the names "target", "window", and "scope".

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1739
> +    return "JSEventTargetOrGlobalScope::create" if $interfaceName eq "EventTarget";

Can we make this a global level function, too, like the others? It could even be named jsEventTargetCast. Does it really need to be a static member function? I understand that its return type will need to be a JSEventTargetOrGlobalScope (but we might be able to come up with a better name for that class). Maybe JSEventTargetWrapper is a suitable name. Comments in the header can explain that JSEventTargetWrapper makes it so we can wrap both literal EventTarget objects and also understand the wrappers for global scopes, which are equivalent to EventTarget even though they are not actually derived from EventTarget.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1740
> +    return "jsDynamicCast<JS" . $interfaceName . "*>";

It was the "->" that meant we had to use explicit concatenation. We can not just write this more simply:

    return "jsDynamicCast<JS$interfaceName*>";

> Source/WebCore/dom/ScriptExecutionContext.cpp:394
> +    auto targetIsWindow = !!target->toDOMWindow();
> +    if (targetIsWindow && is<Document>(*this)) {

I think bool is clearer than auto here. I also think we’d be OK without the local variable.

> LayoutTests/ChangeLog:15
> +        * http/tests/security/cross-frame-access-call-expected.txt:
> +        * http/tests/security/cross-frame-access-call.html:
> +        Add test coverage for cross-origin access to window.postMessage() which
> +        should not be allowed, in addition to window.addEventListener() and
> +        window.removeEventListener().

The code change adds coverage for dispatchEvent; but this comment instead mentions postMessage, addEventListener, and removeEventListener. What’s going on?

> LayoutTests/http/tests/security/cross-frame-access-call.html:46
> +    clickEvent = new Event("click");
> +    shouldBe("window.dispatchEvent.call(targetWindow, clickEvent);", "undefined");

Would read better in a single line:

    shouldBe("window.dispatchEvent.call(targetWindow, new Event('click'));", "undefined");
Comment 7 Chris Dumez 2016-02-14 18:14:49 PST
Created attachment 271317 [details]
Patch
Comment 8 Chris Dumez 2016-02-14 18:17:32 PST
Comment on attachment 271317 [details]
Patch

Clearing flags on attachment: 271317

Committed r196563: <http://trac.webkit.org/changeset/196563>
Comment 9 Chris Dumez 2016-02-14 18:17:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Csaba Osztrogonác 2016-02-15 01:48:58 PST
(In reply to comment #8)
> Comment on attachment 271317 [details]
> Patch
> 
> Clearing flags on attachment: 271317
> 
> Committed r196563: <http://trac.webkit.org/changeset/196563>

It made Dromaeo/cssquery-dojo.html performance test stuck in an 
infinite loop, see performance bots on build.webkit.org for details:
- https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Perf%29/builds/4188
- https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Perf%29/builds/1166
- https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Perf%29/builds/5275
Comment 11 Chris Dumez 2016-02-15 07:27:39 PST
(In reply to comment #10)
> (In reply to comment #8)
> > Comment on attachment 271317 [details]
> > Patch
> > 
> > Clearing flags on attachment: 271317
> > 
> > Committed r196563: <http://trac.webkit.org/changeset/196563>
> 
> It made Dromaeo/cssquery-dojo.html performance test stuck in an 
> infinite loop, see performance bots on build.webkit.org for details:
> -
> https://build.webkit.org/builders/
> Apple%20Yosemite%20Release%20WK2%20%28Perf%29/builds/4188
> -
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20WK2%20%28Perf%29/builds/1166
> -
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Perf%29/builds/5275

Ok, I Will fix it asap, thanks.
Comment 12 Chris Dumez 2016-02-15 08:40:14 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > Comment on attachment 271317 [details]
> > > Patch
> > > 
> > > Clearing flags on attachment: 271317
> > > 
> > > Committed r196563: <http://trac.webkit.org/changeset/196563>
> > 
> > It made Dromaeo/cssquery-dojo.html performance test stuck in an 
> > infinite loop, see performance bots on build.webkit.org for details:
> > -
> > https://build.webkit.org/builders/
> > Apple%20Yosemite%20Release%20WK2%20%28Perf%29/builds/4188
> > -
> > https://build.webkit.org/builders/
> > Apple%20El%20Capitan%20Release%20WK2%20%28Perf%29/builds/1166
> > -
> > https://build.webkit.org/builders/GTK%20Linux%2064-
> > bit%20Release%20%28Perf%29/builds/5275
> 
> Ok, I Will fix it asap, thanks.

The test seems to run fine in the browser, I'll try with run-perf-tests.
Comment 13 Chris Dumez 2016-02-15 08:58:13 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #8)
> > > > Comment on attachment 271317 [details]
> > > > Patch
> > > > 
> > > > Clearing flags on attachment: 271317
> > > > 
> > > > Committed r196563: <http://trac.webkit.org/changeset/196563>
> > > 
> > > It made Dromaeo/cssquery-dojo.html performance test stuck in an 
> > > infinite loop, see performance bots on build.webkit.org for details:
> > > -
> > > https://build.webkit.org/builders/
> > > Apple%20Yosemite%20Release%20WK2%20%28Perf%29/builds/4188
> > > -
> > > https://build.webkit.org/builders/
> > > Apple%20El%20Capitan%20Release%20WK2%20%28Perf%29/builds/1166
> > > -
> > > https://build.webkit.org/builders/GTK%20Linux%2064-
> > > bit%20Release%20%28Perf%29/builds/5275
> > 
> > Ok, I Will fix it asap, thanks.
> 
> The test seems to run fine in the browser, I'll try with run-perf-tests.

Ok, I can reproduce, it looks like it is hitting a JS error:
addEventListener — dojo-1.6.1.js:1628 TypeError: Can only call EventTarget.addEventListener on instances of EventTarget

I am investigating.
Comment 14 Carlos Alberto Lopez Perez 2016-02-22 12:22:09 PST
(In reply to comment #10)
> (In reply to comment #8)
> > Comment on attachment 271317 [details]
> > Patch
> > 
> > Clearing flags on attachment: 271317
> > 
> > Committed r196563: <http://trac.webkit.org/changeset/196563>
> 
> It made Dromaeo/cssquery-dojo.html performance test stuck in an 
> infinite loop, see performance bots on build.webkit.org for details:
> -
> https://build.webkit.org/builders/
> Apple%20Yosemite%20Release%20WK2%20%28Perf%29/builds/4188
> -
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20WK2%20%28Perf%29/builds/1166
> -
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Perf%29/builds/5275

Seems this was fixed in https://trac.webkit.org/changeset/196588