Attachment 346454 Details for Bug 187805 – Fixes the bug

[patch] Fixes the bug

bug-187805-20180802220249.patch (text/plain), 13.29 KB, created by Ryosuke Niwa on 2018-08-02 22:02:50 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-08-02 22:02:50 PDT

Size: 13.29 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 234520)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,38 @@
>+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 NOBODY (OOPS!).
>+
>+        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.
>+        * 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  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         Move queue processing logic from CustomElementReactionStack to CustomElementReactionQueue
>Index: Source/WebCore/bindings/js/JSMainThreadExecState.h
>===================================================================
>--- Source/WebCore/bindings/js/JSMainThreadExecState.h	(revision 234520)
>+++ Source/WebCore/bindings/js/JSMainThreadExecState.h	(working copy)
>@@ -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: Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
>===================================================================
>--- Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	(revision 234520)
>+++ Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	(working copy)
>@@ -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 GenerateAttributeSetterBodyDefinitio
>     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: Source/WebCore/dom/CustomElementReactionQueue.cpp
>===================================================================
>--- Source/WebCore/dom/CustomElementReactionQueue.cpp	(revision 234520)
>+++ Source/WebCore/dom/CustomElementReactionQueue.cpp	(working copy)
>@@ -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,30 @@ inline void CustomElementReactionQueue::
>     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 +274,10 @@ CustomElementReactionQueue& CustomElemen
> 
> 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 +305,7 @@ CustomElementReactionQueue::ElementQueue
> 
> void CustomElementReactionQueue::processBackupQueue()
> {
>-    backupElementQueue().invokeAll();
>+    backupElementQueue().processQueue(nullptr);
>     s_processingBackupElementQueue = false;
> }
> 
>Index: Source/WebCore/dom/CustomElementReactionQueue.h
>===================================================================
>--- Source/WebCore/dom/CustomElementReactionQueue.h	(revision 234520)
>+++ Source/WebCore/dom/CustomElementReactionQueue.h	(working copy)
>@@ -29,6 +29,12 @@
> #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;
> 
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 234520)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2018-08-02  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Crash when throwing exceptions in custom element reactions
>+        https://bugs.webkit.org/show_bug.cgi?id=187805
>+        <rdar://problem/42432714>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Unskipped the previously crashing test.
>+
>+        * TestExpectations:
>+
> 2018-08-02  Megan Gardner  <megan_gardner@apple.com>
> 
>         Reformat editable selection tests to remove potential flakeyness due to autoscroll
>Index: LayoutTests/TestExpectations
>===================================================================
>--- LayoutTests/TestExpectations	(revision 234520)
>+++ LayoutTests/TestExpectations	(working copy)
>@@ -571,7 +571,6 @@ imported/w3c/web-platform-tests/html/sem
> # 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: LayoutTests/imported/w3c/ChangeLog
>===================================================================
>--- LayoutTests/imported/w3c/ChangeLog	(revision 234537)
>+++ LayoutTests/imported/w3c/ChangeLog	(working copy)
>@@ -1,3 +1,25 @@
>+2018-08-02  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Crash when throwing exceptions in custom element reactions
>+        https://bugs.webkit.org/show_bug.cgi?id=187805
>+        <rdar://problem/42432714>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Generated the expected result.
>+
>+        * web-platform-tests/custom-elements/reactions/with-exceptions-expected.txt: Added.
>+
>+2018-08-02  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Crash when throwing exceptions in custom element reactions
>+        https://bugs.webkit.org/show_bug.cgi?id=187805
>+        <rdar://problem/42432714>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * web-platform-tests/custom-elements/reactions/with-exceptions.html:
>+
> 2018-08-01  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         Implement customElements.upgrade()
>Index: LayoutTests/imported/w3c/web-platform-tests/custom-elements/reactions/with-exceptions-expected.txt
>===================================================================
>--- LayoutTests/imported/w3c/web-platform-tests/custom-elements/reactions/with-exceptions-expected.txt	(nonexistent)
>+++ LayoutTests/imported/w3c/web-platform-tests/custom-elements/reactions/with-exceptions-expected.txt	(working copy)
>@@ -0,0 +1,3 @@
>+
>+PASS Reaction must run even after the exception is thrown 
>+

Actions: View | Formatted Diff | Diff

Attachments on bug 187805: 346454 | 346455 | 346460 | 346461