Bug 154170

Created attachment 271245 [details] WIP Patch

Created attachment 271246 [details] WIP Patch

Created attachment 271251 [details] Patch

Created attachment 271267 [details] Patch

<rdar://problem/24642377>

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

Created attachment 271317 [details] Patch

Comment on attachment 271317 [details] Patch Clearing flags on attachment: 271317 Committed r196563: <http://trac.webkit.org/changeset/196563>

All reviewed patches have been landed. Closing bug.

(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

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

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

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

(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