Window and WorkerGlobalScope should inherit EventTarget

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Window and WorkerGlobalScope should inherit EventTarget instead of duplicating its API.

[Chris Dumez]

Created attachment 271245 [details]
WIP Patch

[Chris Dumez]

Created attachment 271246 [details]
WIP Patch

[Chris Dumez]

Created attachment 271251 [details]
Patch

[Chris Dumez]

Created attachment 271267 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/24642377>

[Darin Adler]

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");

[Chris Dumez]

Created attachment 271317 [details]
Patch

[Chris Dumez]

Comment on attachment 271317 [details]
Patch

Clearing flags on attachment: 271317

Committed r196563: <http://trac.webkit.org/changeset/196563>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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

[Carlos Alberto Lopez Perez]

(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