Attachment 346584 Details for Bug 187851 – Adds CEReactions=NotNeeded

[patch] Adds CEReactions=NotNeeded

bug-187851-20180804020754.patch (text/plain), 19.98 KB, created by Ryosuke Niwa on 2018-08-04 02:07:54 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-08-04 02:07:54 PDT

Size: 19.98 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 234577)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,54 @@
>+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 NOBODY (OOPS!).
>+
>+        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):
>+        (GenerateCustomElementReactionsStackIfNeeed): Added. Generate CustomElementReactionStack for [CEReactions]
>+        and CustomElementReactionDisallowedScope for [CEReactions=NotNeeded].
>+        * 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-03  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         innerHTML should not synchronously create a custom element
>Index: Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
>===================================================================
>--- Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	(revision 234576)
>+++ Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	(working copy)
>@@ -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) {
>+        GenerateCustomElementReactionsStackIfNeeed($outputArray, $namedSetterOperation, "*state");
>+    }
>+    if ($indexedSetterOperation) {
>+        GenerateCustomElementReactionsStackIfNeeed($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) {
>+        GenerateCustomElementReactionsStackIfNeeed($outputArray, $namedSetterOperation, "*state");
>+    }
>+    if ($indexedSetterOperation) {
>+        GenerateCustomElementReactionsStackIfNeeed($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) {
>+        GenerateCustomElementReactionsStackIfNeeed($outputArray, $namedSetterOperation, "*state");
>+    }
>+    if ($indexedSetterOperation) {
>+        GenerateCustomElementReactionsStackIfNeeed($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);
>-    }
>+    GenerateCustomElementReactionsStackIfNeeed($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 GenerateAttributeSetterBodyDefinitio
>     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);
>-    }
>-    
>+
>+    GenerateCustomElementReactionsStackIfNeeed($outputArray, $attribute, "state");
>+
>     if ($interface->extendedAttributes->{CheckSecurity} && !$attribute->extendedAttributes->{DoNotCheckSecurity} && !$attribute->extendedAttributes->{DoNotCheckSecurityOnSetter}) {
>         AddToImplIncludes("JSDOMBindingSecurity.h", $conditional);
>         if ($interface->type->name eq "DOMWindow") {
>@@ -7410,4 +7416,21 @@ sub GenerateCallTracer()
>     }
> }
> 
>+sub GenerateCustomElementReactionsStackIfNeeed
>+{
>+    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;
>Index: Source/WebCore/dom/CustomElementReactionQueue.cpp
>===================================================================
>--- Source/WebCore/dom/CustomElementReactionQueue.cpp	(revision 234577)
>+++ Source/WebCore/dom/CustomElementReactionQueue.cpp	(working copy)
>@@ -119,6 +119,7 @@ void CustomElementReactionQueue::clear()
> 
> void CustomElementReactionQueue::enqueueElementUpgrade(Element& element, bool alreadyScheduledToUpgrade)
> {
>+    ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     auto& queue = ensureCurrentQueue(element);
>     if (alreadyScheduledToUpgrade) {
>         ASSERT(queue.m_items.size() == 1);
>@@ -129,6 +130,7 @@ void CustomElementReactionQueue::enqueue
> 
> void CustomElementReactionQueue::enqueueElementUpgradeIfDefined(Element& element)
> {
>+    ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isCustomElementUpgradeCandidate());
>     auto* window = element.document().domWindow();
>     if (!window)
>@@ -147,6 +149,7 @@ void CustomElementReactionQueue::enqueue
> 
> void CustomElementReactionQueue::enqueueConnectedCallbackIfNeeded(Element& element)
> {
>+    ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isDefinedCustomElement());
>     ASSERT(element.document().refCount() > 0);
>     auto& queue = ensureCurrentQueue(element);
>@@ -156,6 +159,7 @@ void CustomElementReactionQueue::enqueue
> 
> void CustomElementReactionQueue::enqueueDisconnectedCallbackIfNeeded(Element& element)
> {
>+    ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isDefinedCustomElement());
>     if (element.document().refCount() <= 0)
>         return; // Don't enqueue disconnectedCallback if the entire document is getting destructed.
>@@ -166,6 +170,7 @@ void CustomElementReactionQueue::enqueue
> 
> void CustomElementReactionQueue::enqueueAdoptedCallbackIfNeeded(Element& element, Document& oldDocument, Document& newDocument)
> {
>+    ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isDefinedCustomElement());
>     ASSERT(element.document().refCount() > 0);
>     auto& queue = ensureCurrentQueue(element);
>@@ -175,6 +180,7 @@ void CustomElementReactionQueue::enqueue
> 
> void CustomElementReactionQueue::enqueueAttributeChangedCallbackIfNeeded(Element& element, const QualifiedName& attributeName, const AtomicString& oldValue, const AtomicString& newValue)
> {
>+    ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isDefinedCustomElement());
>     ASSERT(element.document().refCount() > 0);
>     auto& queue = ensureCurrentQueue(element);
>@@ -184,6 +190,7 @@ void CustomElementReactionQueue::enqueue
> 
> void CustomElementReactionQueue::enqueuePostUpgradeReactions(Element& element)
> {
>+    ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isCustomElementUpgradeCandidate());
>     if (!element.hasAttributes() && !element.isConnected())
>         return;
>@@ -280,6 +287,10 @@ CustomElementReactionQueue& CustomElemen
>     return *element.reactionQueue();
> }
> 
>+#if !ASSERT_DISABLED
>+unsigned CustomElementReactionDisallowedScope::s_customElementReactionDisallowedCount = 0;
>+#endif
>+
> CustomElementReactionStack* CustomElementReactionStack::s_currentProcessingStack = nullptr;
> 
> void CustomElementReactionStack::processQueue(JSC::ExecState* state)
>Index: Source/WebCore/dom/CustomElementReactionQueue.h
>===================================================================
>--- Source/WebCore/dom/CustomElementReactionQueue.h	(revision 234577)
>+++ Source/WebCore/dom/CustomElementReactionQueue.h	(working copy)
>@@ -84,7 +84,51 @@ private:
>     Vector<CustomElementReactionQueueItem> m_items;
> };
> 
>-class CustomElementReactionStack {
>+class CustomElementReactionDisallowedScope {
>+public:
>+#if !ASSERT_DISABLED
>+    CustomElementReactionDisallowedScope()
>+    {
>+        s_customElementReactionDisallowedCount++;
>+    }
>+
>+    ~CustomElementReactionDisallowedScope()
>+    {
>+        ASSERT(s_customElementReactionDisallowedCount);
>+        s_customElementReactionDisallowedCount--;
>+    }
>+
>+    static bool isReactionAllowed() { return !s_customElementReactionDisallowedCount; }
>+#endif
>+
>+    class AllowedScope {
>+#if !ASSERT_DISABLED
>+    public:
>+        AllowedScope()
>+            : m_originalCount(s_customElementReactionDisallowedCount)
>+        {
>+            s_customElementReactionDisallowedCount = 0;
>+        }
>+
>+        ~AllowedScope()
>+        {
>+            s_customElementReactionDisallowedCount = m_originalCount;
>+        }
>+
>+    private:
>+        unsigned m_originalCount;
>+#endif
>+    };
>+
>+private:
>+#if !ASSERT_DISABLED
>+    WEBCORE_EXPORT static unsigned s_customElementReactionDisallowedCount;
>+
>+    friend class AllowedScope;
>+#endif
>+};
>+
>+class CustomElementReactionStack : public CustomElementReactionDisallowedScope::AllowedScope {
> public:
>     ALWAYS_INLINE CustomElementReactionStack(JSC::ExecState* state)
>         : m_previousProcessingStack(s_currentProcessingStack)
>Index: Source/WebCore/html/HTMLIFrameElement.idl
>===================================================================
>--- Source/WebCore/html/HTMLIFrameElement.idl	(revision 234576)
>+++ Source/WebCore/html/HTMLIFrameElement.idl	(working copy)
>@@ -19,22 +19,22 @@
>  */
> 
> interface HTMLIFrameElement : HTMLElement {
>-    [Reflect] attribute DOMString align;
>-    [Reflect] attribute DOMString frameBorder;
>-    [Reflect] attribute DOMString height;
>-    [Reflect, URL] attribute USVString longDesc;
>-    [Reflect] attribute [TreatNullAs=EmptyString] DOMString marginHeight;
>-    [Reflect] attribute [TreatNullAs=EmptyString] DOMString marginWidth;
>-    [Reflect] attribute DOMString name;
>+    [Reflect, CEReactions=NotNeeded] attribute DOMString align;
>+    [Reflect, CEReactions=NotNeeded] attribute DOMString frameBorder;
>+    [Reflect, CEReactions=NotNeeded] attribute DOMString height;
>+    [Reflect, URL, CEReactions=NotNeeded] attribute USVString longDesc;
>+    [Reflect, CEReactions=NotNeeded] attribute [TreatNullAs=EmptyString] DOMString marginHeight;
>+    [Reflect, CEReactions=NotNeeded] attribute [TreatNullAs=EmptyString] DOMString marginWidth;
>+    [Reflect, CEReactions=NotNeeded] attribute DOMString name;
> 
>     [PutForwards=value] readonly attribute DOMTokenList sandbox;
>-    [Reflect] attribute boolean allowFullscreen;
>-    [CEReactions, Reflect] attribute DOMString allow;
>+    [Reflect, CEReactions=NotNeeded] attribute boolean allowFullscreen;
>+    [Reflect, CEReactions=NotNeeded] attribute DOMString allow;
> 
>-    [Reflect] attribute DOMString scrolling;
>-    [Reflect, URL] attribute USVString src;
>-    [Reflect] attribute DOMString srcdoc;
>-    [Reflect] attribute DOMString width;
>+    [Reflect, CEReactions=NotNeeded] attribute DOMString scrolling;
>+    [Reflect, URL, CEReactions=NotNeeded] attribute USVString src;
>+    [Reflect, CEReactions=NotNeeded] attribute DOMString srcdoc;
>+    [Reflect, CEReactions=NotNeeded] attribute DOMString width;
> 
>     [CheckSecurityForNode] readonly attribute Document contentDocument;
> 
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 234576)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        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-03  Justin Fan  <justin_fan@apple.com>
> 
>         WebGL 2 conformance: vertex_arrays/vertex_array_object.html
>Index: LayoutTests/fast/custom-elements/custom-element-reaction-within-disallowed-scope-expected.txt
>===================================================================
>--- LayoutTests/fast/custom-elements/custom-element-reaction-within-disallowed-scope-expected.txt	(nonexistent)
>+++ LayoutTests/fast/custom-elements/custom-element-reaction-within-disallowed-scope-expected.txt	(working copy)
>@@ -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
>+
>Index: LayoutTests/fast/custom-elements/custom-element-reaction-within-disallowed-scope.html
>===================================================================
>--- LayoutTests/fast/custom-elements/custom-element-reaction-within-disallowed-scope.html	(nonexistent)
>+++ LayoutTests/fast/custom-elements/custom-element-reaction-within-disallowed-scope.html	(working copy)
>@@ -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>

Actions: View | Formatted Diff | Diff

Attachments on bug 187851: 345514 | 345531 | 345538 | 345564 | 345656 | 345839 | 346584 | 346585 | 346652