Skip to content
Permalink
Browse files
Add CEReactions=NotNeeded for reactions only needed for customized bu…
…iltins

https://bugs.webkit.org/show_bug.cgi?id=187851

Reviewed by Chris Dumez.

Source/WebCore:

Because WebKit doesn't and will not support customized builtin elements, there are many DOM APIs marked with
[CEReactions] which don't actually need CustomElementReactionStack.

To clarify and document this difference, this patch introduces WebKit extention: [CEReactions=NotNeeded].
When this IDL attribute is specified, we generate CustomElementReactionDisallowedScope in the bindings code
to assert that there are no custom elements reactions being enqueued within the DOM API.

We suppress this assertion in CustomElementReactionStack since a DOM API with [CEReactions=NotNeeded] can
synchronously fire an event and otherwise execute arbirary scripts, which in turn could invoke a DOM API
with [CEReactions].

This patch deployes this change to HTMLIFrameElement since "src" IDL attribute triggers this second scenario.

Test: fast/custom-elements/custom-element-reaction-within-disallowed-scope.html

* bindings/scripts/CodeGeneratorJS.pm:
(GeneratePut):
(GeneratePutByIndex):
(GenerateDefineOwnProperty):
(GenerateDeletePropertyCommon):
(GenerateAttributeSetterBodyDefinition):
(GenerateCustomElementReactionsStackIfNeeded): Added. Generate CustomElementReactionStack for [CEReactions]
and CustomElementReactionDisallowedScope for [CEReactions=NotNeeded].
* bindings/scripts/test/JS/JSTestCEReactions.cpp:
* bindings/scripts/test/TestCEReactions.idl: Added test cases for [CEReactions=NotNeeded].
* bindings/scripts/test/TestCEReactionsStringifier.idl: Ditto.
* dom/CustomElementReactionQueue.cpp:
(WebCore::CustomElementReactionQueue::enqueueElementUpgrade): Added an assertion to catch cases where
a DOM API with [CEReactions=NotNeeded] enqueues a custom element reaction; i.e. cases where [CEReactions]
should have been used.
(WebCore::CustomElementReactionQueue::enqueueElementUpgradeIfDefined): Ditto.
(WebCore::CustomElementReactionQueue::enqueueConnectedCallbackIfNeeded): Ditto.
(WebCore::CustomElementReactionQueue::enqueueDisconnectedCallbackIfNeeded): Ditto.
(WebCore::CustomElementReactionQueue::enqueueAdoptedCallbackIfNeeded): Ditto.
(WebCore::CustomElementReactionQueue::enqueueAttributeChangedCallbackIfNeeded): Ditto.
(WebCore::CustomElementReactionQueue::enqueuePostUpgradeReactions): Ditto.
* dom/CustomElementReactionQueue.h:
(WebCore::CustomElementReactionDisallowedScope): Added. Enables the assertion in enqueue* functions above.
(WebCore::CustomElementReactionDisallowedScope::CustomElementReactionDisallowedScope): Added.
(WebCore::CustomElementReactionDisallowedScope::~CustomElementReactionDisallowedScope): Added.
(WebCore::CustomElementReactionDisallowedScope::isReactionAllowed): Added.
(WebCore::CustomElementReactionDisallowedScope::AllowedScope): Added.
(WebCore::CustomElementReactionDisallowedScope::AllowedScope::AllowedScope): Added.
(WebCore::CustomElementReactionDisallowedScope::AllowedScope::~AllowedScope): Added.
(WebCore::CustomElementReactionStack): Suppress the assertion. See above for why this is needed.
* html/HTMLIFrameElement.idl:

LayoutTests:

Added a regression test for enqueuing a custom element reaction in a DOM API marked as [CEReaction]
inside another DOM API with [CEReaction=NotNeeded]. WebKit should not hit a debug assertion added
by this patch.

* fast/custom-elements/custom-element-reaction-within-disallowed-scope-expected.txt: Added.
* fast/custom-elements/custom-element-reaction-within-disallowed-scope.html: Added.


Canonical link: https://commits.webkit.org/203471@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@234636 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
rniwa committed Aug 7, 2018
1 parent bb4d5af commit a8349c95fd83cfcc888f185b1d29ebdeec9f779b
Showing 12 changed files with 404 additions and 40 deletions.
@@ -1,3 +1,17 @@
2018-08-04 Ryosuke Niwa <rniwa@webkit.org>

Add CEReactions=NotNeeded for reactions only needed for customized builtins
https://bugs.webkit.org/show_bug.cgi?id=187851

Reviewed by Chris Dumez.

Added a regression test for enqueuing a custom element reaction in a DOM API marked as [CEReaction]
inside another DOM API with [CEReaction=NotNeeded]. WebKit should not hit a debug assertion added
by this patch.

* fast/custom-elements/custom-element-reaction-within-disallowed-scope-expected.txt: Added.
* fast/custom-elements/custom-element-reaction-within-disallowed-scope.html: Added.

2018-08-06 Matt Baker <mattbaker@apple.com>

Web Inspector: split-up async stack trace test suite to improve clarity and maintainability
@@ -0,0 +1,4 @@
This tests enqueuing a custom element reaction inside an API with CEReaction=NotNeeded. WebKit should not hit a debug assertion.

PASS - WebKit did not hit an assertion

@@ -0,0 +1,45 @@
<!DOCTYPE html>
<html>
<body>
<script>

if (window.testRunner) {
testRunner.waitUntilDone();
testRunner.dumpAsText();
}

function startTest() {
iframe = document.createElement('iframe');
document.body.appendChild(iframe);
iframe.onload = continueTest;
iframe.src = 'custom-element-reaction-within-disallowed-scope.html';
}

function continueTest() {
iframe.src = 'custom-element-reaction-within-disallowed-scope.html#target';
document.getElementById('result').textContent = didChange
? 'PASS - WebKit did not hit an assertion'
: 'FAIL - The code to trigger a custom element reaction never ran';
if (window.testRunner)
testRunner.notifyDone();
}

if (top == self) {
document.write('<p>This tests enqueuing a custom element reaction inside an API with CEReaction=NotNeeded. WebKit should not hit a debug assertion.</p>');
window.onload = startTest;
} else {
document.write('<p><a id="target" href="#">child target</a></p>');

class SomeElement extends HTMLElement { };
customElements.define('some-element', SomeElement);

document.getElementById('target').addEventListener('focus', () => {
document.body.appendChild(new SomeElement);
top.didChange = true;
});
}

</script>
<pre id="result"></pre>
</body>
</html>
@@ -1,3 +1,57 @@
2018-08-04 Ryosuke Niwa <rniwa@webkit.org>

Add CEReactions=NotNeeded for reactions only needed for customized builtins
https://bugs.webkit.org/show_bug.cgi?id=187851

Reviewed by Chris Dumez.

Because WebKit doesn't and will not support customized builtin elements, there are many DOM APIs marked with
[CEReactions] which don't actually need CustomElementReactionStack.

To clarify and document this difference, this patch introduces WebKit extention: [CEReactions=NotNeeded].
When this IDL attribute is specified, we generate CustomElementReactionDisallowedScope in the bindings code
to assert that there are no custom elements reactions being enqueued within the DOM API.

We suppress this assertion in CustomElementReactionStack since a DOM API with [CEReactions=NotNeeded] can
synchronously fire an event and otherwise execute arbirary scripts, which in turn could invoke a DOM API
with [CEReactions].

This patch deployes this change to HTMLIFrameElement since "src" IDL attribute triggers this second scenario.

Test: fast/custom-elements/custom-element-reaction-within-disallowed-scope.html

* bindings/scripts/CodeGeneratorJS.pm:
(GeneratePut):
(GeneratePutByIndex):
(GenerateDefineOwnProperty):
(GenerateDeletePropertyCommon):
(GenerateAttributeSetterBodyDefinition):
(GenerateCustomElementReactionsStackIfNeeded): Added. Generate CustomElementReactionStack for [CEReactions]
and CustomElementReactionDisallowedScope for [CEReactions=NotNeeded].
* bindings/scripts/test/JS/JSTestCEReactions.cpp:
* bindings/scripts/test/TestCEReactions.idl: Added test cases for [CEReactions=NotNeeded].
* bindings/scripts/test/TestCEReactionsStringifier.idl: Ditto.
* dom/CustomElementReactionQueue.cpp:
(WebCore::CustomElementReactionQueue::enqueueElementUpgrade): Added an assertion to catch cases where
a DOM API with [CEReactions=NotNeeded] enqueues a custom element reaction; i.e. cases where [CEReactions]
should have been used.
(WebCore::CustomElementReactionQueue::enqueueElementUpgradeIfDefined): Ditto.
(WebCore::CustomElementReactionQueue::enqueueConnectedCallbackIfNeeded): Ditto.
(WebCore::CustomElementReactionQueue::enqueueDisconnectedCallbackIfNeeded): Ditto.
(WebCore::CustomElementReactionQueue::enqueueAdoptedCallbackIfNeeded): Ditto.
(WebCore::CustomElementReactionQueue::enqueueAttributeChangedCallbackIfNeeded): Ditto.
(WebCore::CustomElementReactionQueue::enqueuePostUpgradeReactions): Ditto.
* dom/CustomElementReactionQueue.h:
(WebCore::CustomElementReactionDisallowedScope): Added. Enables the assertion in enqueue* functions above.
(WebCore::CustomElementReactionDisallowedScope::CustomElementReactionDisallowedScope): Added.
(WebCore::CustomElementReactionDisallowedScope::~CustomElementReactionDisallowedScope): Added.
(WebCore::CustomElementReactionDisallowedScope::isReactionAllowed): Added.
(WebCore::CustomElementReactionDisallowedScope::AllowedScope): Added.
(WebCore::CustomElementReactionDisallowedScope::AllowedScope::AllowedScope): Added.
(WebCore::CustomElementReactionDisallowedScope::AllowedScope::~AllowedScope): Added.
(WebCore::CustomElementReactionStack): Suppress the assertion. See above for why this is needed.
* html/HTMLIFrameElement.idl:

2018-08-06 Simon Fraser <simon.fraser@apple.com>

Clean up initialization of some RenderLayer members
@@ -930,10 +930,14 @@ sub GeneratePut
push(@$outputArray, "{\n");
push(@$outputArray, " auto* thisObject = jsCast<${className}*>(cell);\n");
push(@$outputArray, " ASSERT_GC_OBJECT_INHERITS(thisObject, info());\n\n");

if (($namedSetterOperation && $namedSetterOperation->extendedAttributes->{CEReactions}) || ($indexedSetterOperation && $indexedSetterOperation->extendedAttributes->{CEReactions})) {
push(@$outputArray, " CustomElementReactionStack customElementReactionStack(*state);\n\n");
AddToImplIncludes("CustomElementReactionQueue.h");

assert("CEReactions is not supported on having both named setters and indexed setters") if $namedSetterOperation && $namedSetterOperation->extendedAttributes->{CEReactions}
&& $indexedSetterOperation && $indexedSetterOperation->extendedAttributes->{CEReactions};
if ($namedSetterOperation) {
GenerateCustomElementReactionsStackIfNeeded($outputArray, $namedSetterOperation, "*state");
}
if ($indexedSetterOperation) {
GenerateCustomElementReactionsStackIfNeeded($outputArray, $indexedSetterOperation, "*state");
}

if ($indexedSetterOperation) {
@@ -1002,10 +1006,14 @@ sub GeneratePutByIndex
push(@$outputArray, "{\n");
push(@$outputArray, " auto* thisObject = jsCast<${className}*>(cell);\n");
push(@$outputArray, " ASSERT_GC_OBJECT_INHERITS(thisObject, info());\n\n");

if (($namedSetterOperation && $namedSetterOperation->extendedAttributes->{CEReactions}) || ($indexedSetterOperation && $indexedSetterOperation->extendedAttributes->{CEReactions})) {
push(@$outputArray, " CustomElementReactionStack customElementReactionStack(*state);\n\n");
AddToImplIncludes("CustomElementReactionQueue.h");

assert("CEReactions is not supported on having both named setters and indexed setters") if $namedSetterOperation && $namedSetterOperation->extendedAttributes->{CEReactions}
&& $indexedSetterOperation && $indexedSetterOperation->extendedAttributes->{CEReactions};
if ($namedSetterOperation) {
GenerateCustomElementReactionsStackIfNeeded($outputArray, $namedSetterOperation, "*state");
}
if ($indexedSetterOperation) {
GenerateCustomElementReactionsStackIfNeeded($outputArray, $indexedSetterOperation, "*state");
}

if ($indexedSetterOperation ) {
@@ -1098,10 +1106,14 @@ sub GenerateDefineOwnProperty
push(@$outputArray, "{\n");
push(@$outputArray, " auto* thisObject = jsCast<${className}*>(object);\n");
push(@$outputArray, " ASSERT_GC_OBJECT_INHERITS(thisObject, info());\n\n");

if (($namedSetterOperation && $namedSetterOperation->extendedAttributes->{CEReactions}) || ($indexedSetterOperation && $indexedSetterOperation->extendedAttributes->{CEReactions})) {
push(@$outputArray, " CustomElementReactionStack customElementReactionStack(*state);\n\n");
AddToImplIncludes("CustomElementReactionQueue.h");

assert("CEReactions is not supported on having both named setters and indexed setters") if $namedSetterOperation && $namedSetterOperation->extendedAttributes->{CEReactions}
&& $indexedSetterOperation && $indexedSetterOperation->extendedAttributes->{CEReactions};
if ($namedSetterOperation) {
GenerateCustomElementReactionsStackIfNeeded($outputArray, $namedSetterOperation, "*state");
}
if ($indexedSetterOperation) {
GenerateCustomElementReactionsStackIfNeeded($outputArray, $indexedSetterOperation, "*state");
}

# 1. If O supports indexed properties and P is an array index property name, then:
@@ -1223,10 +1235,7 @@ sub GenerateDeletePropertyCommon
my $overrideBuiltin = $codeGenerator->InheritsExtendedAttribute($interface, "OverrideBuiltins") ? "OverrideBuiltins::Yes" : "OverrideBuiltins::No";
push(@$outputArray, " if (isVisibleNamedProperty<${overrideBuiltin}>(*state, thisObject, propertyName)) {\n");

if ($operation->extendedAttributes->{CEReactions}) {
push(@$outputArray, " CustomElementReactionStack customElementReactionStack(*state);\n");
AddToImplIncludes("CustomElementReactionQueue.h", $conditional);
}
GenerateCustomElementReactionsStackIfNeeded($outputArray, $operation, "*state");

# 2.1. If O does not implement an interface with a named property deleter, then return false.
# 2.2. Let operation be the operation used to declare the named property deleter.
@@ -4842,12 +4851,9 @@ sub GenerateAttributeSetterBodyDefinition
push(@$outputArray, "static inline bool ${attributeSetterBodyName}(" . join(", ", @signatureArguments) . ")\n");
push(@$outputArray, "{\n");
push(@$outputArray, " UNUSED_PARAM(throwScope);\n");

if ($attribute->extendedAttributes->{CEReactions}) {
push(@$outputArray, " CustomElementReactionStack customElementReactionStack(state);\n");
AddToImplIncludes("CustomElementReactionQueue.h", $conditional);
}


GenerateCustomElementReactionsStackIfNeeded($outputArray, $attribute, "state");

if ($interface->extendedAttributes->{CheckSecurity} && !$attribute->extendedAttributes->{DoNotCheckSecurity} && !$attribute->extendedAttributes->{DoNotCheckSecurityOnSetter}) {
AddToImplIncludes("JSDOMBindingSecurity.h", $conditional);
if ($interface->type->name eq "DOMWindow") {
@@ -5068,10 +5074,7 @@ sub GenerateOperationBodyDefinition
push(@$outputArray, " UNUSED_PARAM(throwScope);\n");

if (!$generatingOverloadDispatcher) {
if ($operation->extendedAttributes->{CEReactions}) {
AddToImplIncludes("CustomElementReactionQueue.h", $conditional);
push(@$outputArray, " CustomElementReactionStack customElementReactionStack(*state);\n");
}
GenerateCustomElementReactionsStackIfNeeded($outputArray, $operation, "*state");

if ($interface->extendedAttributes->{CheckSecurity} and !$operation->extendedAttributes->{DoNotCheckSecurity}) {
assert("Security checks are not supported for static operations.") if $operation->isStatic;
@@ -7410,4 +7413,21 @@ sub GenerateCallTracer()
}
}

sub GenerateCustomElementReactionsStackIfNeeded
{
my ($outputArray, $context, $stateVariable) = @_;

my $CEReactions = $context->extendedAttributes->{CEReactions};

return if !$CEReactions;

AddToImplIncludes("CustomElementReactionQueue.h");

if ($CEReactions eq "NotNeeded") {
push(@$outputArray, " CustomElementReactionDisallowedScope customElementReactionDisallowedScope;\n");
} else {
push(@$outputArray, " CustomElementReactionStack customElementReactionStack($stateVariable);\n");
}
}

1;

0 comments on commit a8349c9

Please sign in to comment.