Release assert when throwing exceptions in custom element reactions
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Aug 2018 08:16:48 +0000 (08:16 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Aug 2018 08:16:48 +0000 (08:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187805
<rdar://problem/42432714>

Reviewed by Saam Barati.

LayoutTests/imported/w3c:

Generated the expected result.

* web-platform-tests/custom-elements/reactions/with-exceptions-expected.txt: Added.

Source/WebCore:

The release assertion was hit because we were not catching & re-throwing the exception thrown by DOM API
before trying to execute custom elements reactions in ~CustomElementReactionStack as specified here:
https://html.spec.whatwg.org/multipage/custom-elements.html#cereactions
Fixed the bug by capturing the exception and re-throwing the exception as specified.

Tests: imported/w3c/web-platform-tests/custom-elements/reactions/with-exceptions.html

* bindings/js/JSMainThreadExecState.h:
(WebCore::JSMainThreadNullState::JSMainThreadNullState): Use the previous JS state.
* bindings/scripts/CodeGeneratorJS.pm:
(GeneratePut): Pass in the exec state to CustomElementReactionStack.
(GeneratePutByIndex): Ditto.
(GenerateDefineOwnProperty): Ditto.
(GenerateDeletePropertyCommon): Ditto.
(GenerateAttributeSetterBodyDefinition): Ditto.
(GenerateOperationBodyDefinition): Ditto.
* bindings/scripts/test/JS/JSTestCEReactions.cpp:
(WebCore::setJSTestCEReactionsAttributeWithCEReactionsSetter):
(WebCore::setJSTestCEReactionsReflectAttributeWithCEReactionsSetter):
(WebCore::jsTestCEReactionsPrototypeFunctionMethodWithCEReactionsBody):
* bindings/scripts/test/JS/JSTestCEReactionsStringifier.cpp:
(WebCore::setJSTestCEReactionsStringifierValueSetter):
* dom/CustomElementReactionQueue.cpp:
(WebCore::CustomElementReactionQueue::ElementQueue::processQueue): Added. If there is a script running
in the stack (i.e. ExecState is not null), catch any exception before executing custom element reactions,
then re-throw the exception afterwards. ExecState is null when DOM API is invoked via Objective-C bindings
or when custom element reactions are executed in the backup queue (e.g. for editing operations).
(WebCore::CustomElementReactionStack::processQueue):
(WebCore::CustomElementReactionQueue::processBackupQueue):
* dom/CustomElementReactionQueue.h:
(WebCore::CustomElementReactionStack::CustomElementReactionStack):
(WebCore::CustomElementReactionStack::~CustomElementReactionStack):

LayoutTests:

Unskipped the previously crashing test.

* TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@234539 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/custom-elements/reactions/with-exceptions-expected.txt [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSMainThreadExecState.h
Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
Source/WebCore/bindings/scripts/test/JS/JSTestCEReactions.cpp
Source/WebCore/bindings/scripts/test/JS/JSTestCEReactionsStringifier.cpp
Source/WebCore/dom/CustomElementReactionQueue.cpp
Source/WebCore/dom/CustomElementReactionQueue.h

index 5ef8510..fea830e 100644 (file)
@@ -1,3 +1,15 @@
+2018-08-02  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Release assert when throwing exceptions in custom element reactions
+        https://bugs.webkit.org/show_bug.cgi?id=187805
+        <rdar://problem/42432714>
+
+        Reviewed by Saam Barati.
+
+        Unskipped the previously crashing test.
+
+        * TestExpectations:
+
 2018-08-02  Basuke Suzuki  <Basuke.Suzuki@sony.com>
 
         [Curl] Test gardening
index 2c7f6dc..dc885e8 100644 (file)
@@ -571,7 +571,6 @@ imported/w3c/web-platform-tests/html/semantics/document-metadata/the-meta-elemen
 # WPT tests for custom elements
 webkit.org/b/187800 imported/w3c/web-platform-tests/custom-elements/Document-createElement-svg.svg [ Skip ]
 webkit.org/b/187802 imported/w3c/web-platform-tests/custom-elements/parser/parser-uses-create-an-element-for-a-token-svg.svg [ Skip ]
-webkit.org/b/187805 imported/w3c/web-platform-tests/custom-elements/reactions/with-exceptions.html [ Skip ]
 
 # selectors
 webkit.org/b/64861 imported/w3c/web-platform-tests/css/selectors/selectors-dir-selector-ltr-001.html [ ImageOnlyFailure ]
index 0d8527f..2409531 100644 (file)
@@ -1,3 +1,15 @@
+2018-08-02  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Release assert when throwing exceptions in custom element reactions
+        https://bugs.webkit.org/show_bug.cgi?id=187805
+        <rdar://problem/42432714>
+
+        Reviewed by Saam Barati.
+
+        Generated the expected result.
+
+        * web-platform-tests/custom-elements/reactions/with-exceptions-expected.txt: Added.
+
 2018-08-01  Ryosuke Niwa  <rniwa@webkit.org>
 
         Implement customElements.upgrade()
diff --git a/LayoutTests/imported/w3c/web-platform-tests/custom-elements/reactions/with-exceptions-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/custom-elements/reactions/with-exceptions-expected.txt
new file mode 100644 (file)
index 0000000..3dea5a2
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS Reaction must run even after the exception is thrown 
+
index aa50efa..11982cf 100644 (file)
@@ -1,3 +1,44 @@
+2018-08-02  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Release assert when throwing exceptions in custom element reactions
+        https://bugs.webkit.org/show_bug.cgi?id=187805
+        <rdar://problem/42432714>
+
+        Reviewed by Saam Barati.
+
+        The release assertion was hit because we were not catching & re-throwing the exception thrown by DOM API
+        before trying to execute custom elements reactions in ~CustomElementReactionStack as specified here:
+        https://html.spec.whatwg.org/multipage/custom-elements.html#cereactions
+        Fixed the bug by capturing the exception and re-throwing the exception as specified.
+
+        Tests: imported/w3c/web-platform-tests/custom-elements/reactions/with-exceptions.html
+
+        * bindings/js/JSMainThreadExecState.h:
+        (WebCore::JSMainThreadNullState::JSMainThreadNullState): Use the previous JS state.
+        * bindings/scripts/CodeGeneratorJS.pm:
+        (GeneratePut): Pass in the exec state to CustomElementReactionStack.
+        (GeneratePutByIndex): Ditto.
+        (GenerateDefineOwnProperty): Ditto.
+        (GenerateDeletePropertyCommon): Ditto.
+        (GenerateAttributeSetterBodyDefinition): Ditto.
+        (GenerateOperationBodyDefinition): Ditto.
+        * bindings/scripts/test/JS/JSTestCEReactions.cpp:
+        (WebCore::setJSTestCEReactionsAttributeWithCEReactionsSetter):
+        (WebCore::setJSTestCEReactionsReflectAttributeWithCEReactionsSetter):
+        (WebCore::jsTestCEReactionsPrototypeFunctionMethodWithCEReactionsBody):
+        * bindings/scripts/test/JS/JSTestCEReactionsStringifier.cpp:
+        (WebCore::setJSTestCEReactionsStringifierValueSetter):
+        * dom/CustomElementReactionQueue.cpp:
+        (WebCore::CustomElementReactionQueue::ElementQueue::processQueue): Added. If there is a script running
+        in the stack (i.e. ExecState is not null), catch any exception before executing custom element reactions,
+        then re-throw the exception afterwards. ExecState is null when DOM API is invoked via Objective-C bindings
+        or when custom element reactions are executed in the backup queue (e.g. for editing operations).
+        (WebCore::CustomElementReactionStack::processQueue):
+        (WebCore::CustomElementReactionQueue::processBackupQueue):
+        * dom/CustomElementReactionQueue.h:
+        (WebCore::CustomElementReactionStack::CustomElementReactionStack):
+        (WebCore::CustomElementReactionStack::~CustomElementReactionStack):
+
 2018-08-02  Zalan Bujtas  <zalan@apple.com>
 
         [LFC][BFC] Apply the "10.6.6 Complicated cases" when computing height and margin for the document renderer
index 5a51925..5ba8eaa 100644 (file)
@@ -161,6 +161,7 @@ class JSMainThreadNullState {
 public:
     explicit JSMainThreadNullState()
         : m_previousState(JSMainThreadExecState::s_mainThreadState)
+        , m_customElementReactionStack(m_previousState)
     {
         ASSERT(isMainThread());
         JSMainThreadExecState::s_mainThreadState = nullptr;
index d9d4583..a188368 100644 (file)
@@ -932,7 +932,7 @@ sub GeneratePut
     push(@$outputArray, "    ASSERT_GC_OBJECT_INHERITS(thisObject, info());\n\n");
     
     if (($namedSetterOperation && $namedSetterOperation->extendedAttributes->{CEReactions}) || ($indexedSetterOperation && $indexedSetterOperation->extendedAttributes->{CEReactions})) {
-        push(@$outputArray, "    CustomElementReactionStack customElementReactionStack;\n\n");
+        push(@$outputArray, "    CustomElementReactionStack customElementReactionStack(*state);\n\n");
         AddToImplIncludes("CustomElementReactionQueue.h");
     }
     
@@ -1004,7 +1004,7 @@ sub GeneratePutByIndex
     push(@$outputArray, "    ASSERT_GC_OBJECT_INHERITS(thisObject, info());\n\n");
     
     if (($namedSetterOperation && $namedSetterOperation->extendedAttributes->{CEReactions}) || ($indexedSetterOperation && $indexedSetterOperation->extendedAttributes->{CEReactions})) {
-        push(@$outputArray, "    CustomElementReactionStack customElementReactionStack;\n\n");
+        push(@$outputArray, "    CustomElementReactionStack customElementReactionStack(*state);\n\n");
         AddToImplIncludes("CustomElementReactionQueue.h");
     }
     
@@ -1100,7 +1100,7 @@ sub GenerateDefineOwnProperty
     push(@$outputArray, "    ASSERT_GC_OBJECT_INHERITS(thisObject, info());\n\n");
     
     if (($namedSetterOperation && $namedSetterOperation->extendedAttributes->{CEReactions}) || ($indexedSetterOperation && $indexedSetterOperation->extendedAttributes->{CEReactions})) {
-        push(@$outputArray, "    CustomElementReactionStack customElementReactionStack;\n\n");
+        push(@$outputArray, "    CustomElementReactionStack customElementReactionStack(*state);\n\n");
         AddToImplIncludes("CustomElementReactionQueue.h");
     }
     
@@ -1224,7 +1224,7 @@ sub GenerateDeletePropertyCommon
     push(@$outputArray, "    if (isVisibleNamedProperty<${overrideBuiltin}>(*state, thisObject, propertyName)) {\n");
 
     if ($operation->extendedAttributes->{CEReactions}) {
-        push(@$outputArray, "        CustomElementReactionStack customElementReactionStack;\n");
+        push(@$outputArray, "        CustomElementReactionStack customElementReactionStack(*state);\n");
         AddToImplIncludes("CustomElementReactionQueue.h", $conditional);
     }
 
@@ -4844,7 +4844,7 @@ sub GenerateAttributeSetterBodyDefinition
     push(@$outputArray, "    UNUSED_PARAM(throwScope);\n");
     
     if ($attribute->extendedAttributes->{CEReactions}) {
-        push(@$outputArray, "    CustomElementReactionStack customElementReactionStack;\n");
+        push(@$outputArray, "    CustomElementReactionStack customElementReactionStack(state);\n");
         AddToImplIncludes("CustomElementReactionQueue.h", $conditional);
     }
     
@@ -5070,7 +5070,7 @@ sub GenerateOperationBodyDefinition
     if (!$generatingOverloadDispatcher) {
         if ($operation->extendedAttributes->{CEReactions}) {
             AddToImplIncludes("CustomElementReactionQueue.h", $conditional);
-            push(@$outputArray, "    CustomElementReactionStack customElementReactionStack;\n");
+            push(@$outputArray, "    CustomElementReactionStack customElementReactionStack(*state);\n");
         }
 
         if ($interface->extendedAttributes->{CheckSecurity} and !$operation->extendedAttributes->{DoNotCheckSecurity}) {
index 39281f1..0e0bef0 100644 (file)
@@ -203,7 +203,7 @@ EncodedJSValue jsTestCEReactionsAttributeWithCEReactions(ExecState* state, Encod
 static inline bool setJSTestCEReactionsAttributeWithCEReactionsSetter(ExecState& state, JSTestCEReactions& thisObject, JSValue value, ThrowScope& throwScope)
 {
     UNUSED_PARAM(throwScope);
-    CustomElementReactionStack customElementReactionStack;
+    CustomElementReactionStack customElementReactionStack(state);
     auto& impl = thisObject.wrapped();
     auto nativeValue = convert<IDLDOMString>(state, value);
     RETURN_IF_EXCEPTION(throwScope, false);
@@ -235,7 +235,7 @@ EncodedJSValue jsTestCEReactionsReflectAttributeWithCEReactions(ExecState* state
 static inline bool setJSTestCEReactionsReflectAttributeWithCEReactionsSetter(ExecState& state, JSTestCEReactions& thisObject, JSValue value, ThrowScope& throwScope)
 {
     UNUSED_PARAM(throwScope);
-    CustomElementReactionStack customElementReactionStack;
+    CustomElementReactionStack customElementReactionStack(state);
     auto& impl = thisObject.wrapped();
     auto nativeValue = convert<IDLDOMString>(state, value);
     RETURN_IF_EXCEPTION(throwScope, false);
@@ -290,7 +290,7 @@ static inline JSC::EncodedJSValue jsTestCEReactionsPrototypeFunctionMethodWithCE
 {
     UNUSED_PARAM(state);
     UNUSED_PARAM(throwScope);
-    CustomElementReactionStack customElementReactionStack;
+    CustomElementReactionStack customElementReactionStack(*state);
     auto& impl = castedThis->wrapped();
     impl.methodWithCEReactions();
     return JSValue::encode(jsUndefined());
index d7f1679..67188b1 100644 (file)
@@ -193,7 +193,7 @@ EncodedJSValue jsTestCEReactionsStringifierValue(ExecState* state, EncodedJSValu
 static inline bool setJSTestCEReactionsStringifierValueSetter(ExecState& state, JSTestCEReactionsStringifier& thisObject, JSValue value, ThrowScope& throwScope)
 {
     UNUSED_PARAM(throwScope);
-    CustomElementReactionStack customElementReactionStack;
+    CustomElementReactionStack customElementReactionStack(state);
     auto& impl = thisObject.wrapped();
     auto nativeValue = convert<IDLDOMString>(state, value);
     RETURN_IF_EXCEPTION(throwScope, false);
index c3f0989..1c75f3f 100644 (file)
@@ -34,6 +34,7 @@
 #include "JSCustomElementInterface.h"
 #include "JSDOMBinding.h"
 #include "Microtasks.h"
+#include <JavaScriptCore/CatchScope.h>
 #include <JavaScriptCore/Heap.h>
 #include <wtf/NeverDestroyed.h>
 #include <wtf/Optional.h>
@@ -231,6 +232,32 @@ inline void CustomElementReactionQueue::ElementQueue::invokeAll()
     RELEASE_ASSERT(m_elements.isEmpty());
 }
 
+inline void CustomElementReactionQueue::ElementQueue::processQueue(JSC::ExecState* state)
+{
+    if (!state) {
+        invokeAll();
+        return;
+    }
+
+    auto& vm = state->vm();
+    JSC::JSLockHolder lock(vm);
+
+    JSC::Exception* previousException = nullptr;
+    {
+        auto catchScope = DECLARE_CATCH_SCOPE(vm);
+        previousException = catchScope.exception();
+        if (previousException)
+            catchScope.clearException();
+    }
+
+    invokeAll();
+
+    if (previousException) {
+        auto throwScope = DECLARE_THROW_SCOPE(vm);
+        throwException(state, throwScope, previousException);
+    }
+}
+
 CustomElementReactionQueue& CustomElementReactionQueue::ensureCurrentQueue(Element& element)
 {
     ASSERT(element.reactionQueue());
@@ -249,10 +276,10 @@ CustomElementReactionQueue& CustomElementReactionQueue::ensureCurrentQueue(Eleme
 
 CustomElementReactionStack* CustomElementReactionStack::s_currentProcessingStack = nullptr;
 
-void CustomElementReactionStack::processQueue()
+void CustomElementReactionStack::processQueue(JSC::ExecState* state)
 {
     ASSERT(m_queue);
-    m_queue->invokeAll();
+    m_queue->processQueue(state);
     delete m_queue;
     m_queue = nullptr;
 }
@@ -280,7 +307,7 @@ CustomElementReactionQueue::ElementQueue& CustomElementReactionQueue::ensureBack
 
 void CustomElementReactionQueue::processBackupQueue()
 {
-    backupElementQueue().invokeAll();
+    backupElementQueue().processQueue(nullptr);
     s_processingBackupElementQueue = false;
 }
 
index f1ddbdf..c6768e3 100644 (file)
 #include <wtf/Noncopyable.h>
 #include <wtf/Vector.h>
 
+namespace JSC {
+
+class ExecState;
+
+}
+
 namespace WebCore {
 
 class CustomElementReactionQueueItem;
@@ -60,9 +66,11 @@ public:
     class ElementQueue {
     public:
         void add(Element&);
-        void invokeAll();
-        
+        void processQueue(JSC::ExecState*);
+
     private:
+        void invokeAll();
+
         Vector<Ref<Element>> m_elements;
         bool m_invoking { false };
     };
@@ -78,24 +86,30 @@ private:
 
 class CustomElementReactionStack {
 public:
-    CustomElementReactionStack()
+    ALWAYS_INLINE CustomElementReactionStack(JSC::ExecState* state)
         : m_previousProcessingStack(s_currentProcessingStack)
+        , m_state(state)
     {
         s_currentProcessingStack = this;
     }
 
-    ~CustomElementReactionStack()
+    ALWAYS_INLINE CustomElementReactionStack(JSC::ExecState& state)
+        : CustomElementReactionStack(&state)
+    { }
+
+    ALWAYS_INLINE ~CustomElementReactionStack()
     {
         if (UNLIKELY(m_queue))
-            processQueue();
+            processQueue(m_state);
         s_currentProcessingStack = m_previousProcessingStack;
     }
 
 private:
-    WEBCORE_EXPORT void processQueue();
+    WEBCORE_EXPORT void processQueue(JSC::ExecState*);
 
     CustomElementReactionQueue::ElementQueue* m_queue { nullptr }; // Use raw pointer to avoid generating delete in the destructor.
     CustomElementReactionStack* m_previousProcessingStack;
+    JSC::ExecState* m_state;
 
     WEBCORE_EXPORT static CustomElementReactionStack* s_currentProcessingStack;