Title: [226101] releases/WebKitGTK/webkit-2.18
Revision
226101
Author
[email protected]
Date
2017-12-18 22:46:21 -0800 (Mon, 18 Dec 2017)

Log Message

Merge r224405 - Crash inside ChildListMutationAccumulator::enqueueMutationRecord()
https://bugs.webkit.org/show_bug.cgi?id=179234
<rdar://problem/35287748>

Reviewed by Darin Adler.

Source/WebCore:

Fixed the crash by keeping MutationObserver referenced by MutationObserverInterestGroup alive.

Also added hasCallback() virtual function on MutationObserver to check whether the callback is alive
to work around the bug that JS function referenced by MutationObserver isn't kept alive.
We'll address this bug separately in https://webkit.org/b/179224.

Test: fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash.html

* bindings/scripts/CodeGeneratorJS.pm:
(GenerateCallbackHeaderContent): Added an override for the newly added virtual hasCallback().
* dom/MutationCallback.h:
* dom/MutationObserver.cpp:
(WebCore::MutationObserver::deliver): Added the aforementioned workaround.
* dom/MutationObserverInterestGroup.cpp:
(WebCore::MutationObserverInterestGroup::MutationObserverInterestGroup): Fixed the crash by using Ref.
(WebCore::MutationObserverInterestGroup::enqueueMutationRecord): Ditto.
* dom/MutationObserverInterestGroup.h:
* dom/NativeNodeFilter.cpp:
(WebCore::NativeNodeFilter::hasCallback const): Always return true here. This function is never called
but we still need to implement it since NodeFilter has a pure virtual hasCallback() now.
* dom/NativeNodeFilter.h:
* dom/Node.cpp:
(WebCore::collectMatchingObserversForMutation): Use Ref to fix the crash.
(WebCore::Node::registeredMutationObservers): Ditto.
* dom/Node.h:
* dom/NodeFilter.h:

LayoutTests:

Added a regression test.

* fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash-expected.txt: Added.
* fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash.html: Added.

Modified Paths

Added Paths

Diff

Modified: releases/WebKitGTK/webkit-2.18/LayoutTests/ChangeLog (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/LayoutTests/ChangeLog	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/LayoutTests/ChangeLog	2017-12-19 06:46:21 UTC (rev 226101)
@@ -1,3 +1,16 @@
+2017-11-03  Ryosuke Niwa  <[email protected]>
+
+        Crash inside ChildListMutationAccumulator::enqueueMutationRecord()
+        https://bugs.webkit.org/show_bug.cgi?id=179234
+        <rdar://problem/35287748>
+
+        Reviewed by Darin Adler.
+
+        Added a regression test.
+
+        * fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash-expected.txt: Added.
+        * fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash.html: Added.
+
 2017-11-10  Carlos Garcia Campos  <[email protected]>
 
         Unreviewed GTK+ gardening. Rebaseline tests after r224572 and r224573. Part 3.

Added: releases/WebKitGTK/webkit-2.18/LayoutTests/fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash-expected.txt (0 => 226101)


--- releases/WebKitGTK/webkit-2.18/LayoutTests/fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash-expected.txt	                        (rev 0)
+++ releases/WebKitGTK/webkit-2.18/LayoutTests/fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash-expected.txt	2017-12-19 06:46:21 UTC (rev 226101)
@@ -0,0 +1,10 @@
+This tests disconnecting a MutationObserver while a mutation record is enqueued. WebKit should not crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS WebKit did not crash
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: releases/WebKitGTK/webkit-2.18/LayoutTests/fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash.html (0 => 226101)


--- releases/WebKitGTK/webkit-2.18/LayoutTests/fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash.html	                        (rev 0)
+++ releases/WebKitGTK/webkit-2.18/LayoutTests/fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash.html	2017-12-19 06:46:21 UTC (rev 226101)
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<script>
+
+description('This tests disconnecting a MutationObserver while a mutation record is enqueued. WebKit should not crash.');
+
+if (!window.GCController)
+    testFailed('This test requires GCController');
+else {
+    jsTestIsAsync = true;
+
+    var observer = new MutationObserver(function() {});
+
+    var div = document.createElement('div');
+    document.body.appendChild(div);
+
+    observer.observe(div, {childList: true});
+
+    var script = document.createElement('script');
+    script.textContent = 'disconnectObserver()';
+
+    var child = document.createElement('div');
+    child.appendChild(script);
+    div.appendChild(child);
+
+    function disconnectObserver() {
+        observer.disconnect();
+        observer = null;
+        GCController.collect();
+        observer = new MutationObserver(function() {});
+    }
+
+    window._onload_ = () => {
+        testPassed('WebKit did not crash');
+        finishJSTest();
+    }
+}
+
+var successfullyParsed = true;
+
+</script>
+</body>
+</html>

Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/ChangeLog (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/Source/WebCore/ChangeLog	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/ChangeLog	2017-12-19 06:46:21 UTC (rev 226101)
@@ -1,3 +1,38 @@
+2017-11-03  Ryosuke Niwa  <[email protected]>
+
+        Crash inside ChildListMutationAccumulator::enqueueMutationRecord()
+        https://bugs.webkit.org/show_bug.cgi?id=179234
+        <rdar://problem/35287748>
+
+        Reviewed by Darin Adler.
+
+        Fixed the crash by keeping MutationObserver referenced by MutationObserverInterestGroup alive.
+
+        Also added hasCallback() virtual function on MutationObserver to check whether the callback is alive
+        to work around the bug that JS function referenced by MutationObserver isn't kept alive.
+        We'll address this bug separately in https://webkit.org/b/179224.
+
+        Test: fast/dom/MutationObserver/disconnect-observer-while-mutation-records-are-enqueued-crash.html
+
+        * bindings/scripts/CodeGeneratorJS.pm:
+        (GenerateCallbackHeaderContent): Added an override for the newly added virtual hasCallback().
+        * dom/MutationCallback.h:
+        * dom/MutationObserver.cpp:
+        (WebCore::MutationObserver::deliver): Added the aforementioned workaround.
+        * dom/MutationObserverInterestGroup.cpp:
+        (WebCore::MutationObserverInterestGroup::MutationObserverInterestGroup): Fixed the crash by using Ref.
+        (WebCore::MutationObserverInterestGroup::enqueueMutationRecord): Ditto.
+        * dom/MutationObserverInterestGroup.h:
+        * dom/NativeNodeFilter.cpp:
+        (WebCore::NativeNodeFilter::hasCallback const): Always return true here. This function is never called
+        but we still need to implement it since NodeFilter has a pure virtual hasCallback() now.
+        * dom/NativeNodeFilter.h:
+        * dom/Node.cpp:
+        (WebCore::collectMatchingObserversForMutation): Use Ref to fix the crash.
+        (WebCore::Node::registeredMutationObservers): Ditto.
+        * dom/Node.h:
+        * dom/NodeFilter.h:
+
 2017-11-14  Carlos Garcia Campos  <[email protected]>
 
         Move JSONValues to WTF and convert uses of InspectorValues.h to JSONValues.h

Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	2017-12-19 06:46:21 UTC (rev 226101)
@@ -5831,6 +5831,10 @@
 
     push(@$contentRef, "    ${className}(JSC::JSObject*, JSDOMGlobalObject*);\n\n");
 
+    if ($interfaceOrCallback->extendedAttributes->{IsWeakCallback}) {
+        push(@$contentRef, "    bool hasCallback() const final { return m_data && m_data->callback(); }\n\n");
+    }
+
     push(@$contentRef, "    virtual void visitJSFunction(JSC::SlotVisitor&) override;\n\n") if $interfaceOrCallback->extendedAttributes->{IsWeakCallback};
 
     push(@$contentRef, "    ${callbackDataType}* m_data;\n");

Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationCallback.h (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationCallback.h	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationCallback.h	2017-12-19 06:46:21 UTC (rev 226101)
@@ -45,6 +45,8 @@
 public:
     using ActiveDOMCallback::ActiveDOMCallback;
 
+    virtual bool hasCallback() const = 0;
+
     virtual CallbackResult<void> handleEvent(MutationObserver&, const Vector<Ref<MutationRecord>>&, MutationObserver&) = 0;
 };
 

Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationObserver.cpp (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationObserver.cpp	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationObserver.cpp	2017-12-19 06:46:21 UTC (rev 226101)
@@ -232,7 +232,9 @@
     Vector<Ref<MutationRecord>> records;
     records.swap(m_records);
 
-    m_callback->handleEvent(*this, records, *this);
+    // FIXME: Keep mutation observer callback as long as its observed nodes are alive. See https://webkit.org/b/179224.
+    if (m_callback->hasCallback())
+        m_callback->handleEvent(*this, records, *this);
 }
 
 void MutationObserver::notifyMutationObservers()

Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationObserverInterestGroup.cpp (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationObserverInterestGroup.cpp	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationObserverInterestGroup.cpp	2017-12-19 06:46:21 UTC (rev 226101)
@@ -37,7 +37,7 @@
 
 namespace WebCore {
 
-inline MutationObserverInterestGroup::MutationObserverInterestGroup(HashMap<MutationObserver*, MutationRecordDeliveryOptions>&& observers, MutationRecordDeliveryOptions oldValueFlag)
+inline MutationObserverInterestGroup::MutationObserverInterestGroup(HashMap<Ref<MutationObserver>, MutationRecordDeliveryOptions>&& observers, MutationRecordDeliveryOptions oldValueFlag)
     : m_observers(WTFMove(observers))
     , m_oldValueFlag(oldValueFlag)
 {
@@ -67,9 +67,9 @@
 {
     RefPtr<MutationRecord> mutationWithNullOldValue;
     for (auto& observerOptionsPair : m_observers) {
-        MutationObserver* observer = observerOptionsPair.key;
+        auto& observer = observerOptionsPair.key.get();
         if (hasOldValue(observerOptionsPair.value)) {
-            observer->enqueueMutationRecord(mutation.copyRef());
+            observer.enqueueMutationRecord(mutation.copyRef());
             continue;
         }
         if (!mutationWithNullOldValue) {
@@ -78,7 +78,7 @@
             else
                 mutationWithNullOldValue = MutationRecord::createWithNullOldValue(mutation).ptr();
         }
-        observer->enqueueMutationRecord(*mutationWithNullOldValue);
+        observer.enqueueMutationRecord(*mutationWithNullOldValue);
     }
 }
 

Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationObserverInterestGroup.h (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationObserverInterestGroup.h	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/MutationObserverInterestGroup.h	2017-12-19 06:46:21 UTC (rev 226101)
@@ -42,7 +42,7 @@
 class MutationObserverInterestGroup {
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    MutationObserverInterestGroup(HashMap<MutationObserver*, MutationRecordDeliveryOptions>&&, MutationRecordDeliveryOptions);
+    MutationObserverInterestGroup(HashMap<Ref<MutationObserver>, MutationRecordDeliveryOptions>&&, MutationRecordDeliveryOptions);
 
     static std::unique_ptr<MutationObserverInterestGroup> createForChildListMutation(Node& target)
     {
@@ -77,7 +77,7 @@
 
     bool hasOldValue(MutationRecordDeliveryOptions options) const { return options & m_oldValueFlag; }
 
-    HashMap<MutationObserver*, MutationRecordDeliveryOptions> m_observers;
+    HashMap<Ref<MutationObserver>, MutationRecordDeliveryOptions> m_observers;
     MutationRecordDeliveryOptions m_oldValueFlag;
 };
 

Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/NativeNodeFilter.cpp (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/NativeNodeFilter.cpp	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/NativeNodeFilter.cpp	2017-12-19 06:46:21 UTC (rev 226101)
@@ -34,6 +34,11 @@
 {
 }
 
+bool NativeNodeFilter::hasCallback() const
+{
+    return true;
+}
+
 CallbackResult<unsigned short> NativeNodeFilter::acceptNode(Node& node)
 {
     return m_condition->acceptNode(node);

Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/NativeNodeFilter.h (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/NativeNodeFilter.h	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/NativeNodeFilter.h	2017-12-19 06:46:21 UTC (rev 226101)
@@ -43,6 +43,8 @@
 private:
     WEBCORE_EXPORT explicit NativeNodeFilter(ScriptExecutionContext*, Ref<NodeFilterCondition>&&);
 
+    bool hasCallback() const final;
+
     Ref<NodeFilterCondition> m_condition;
 };
 

Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/Node.cpp (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/Node.cpp	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/Node.cpp	2017-12-19 06:46:21 UTC (rev 226101)
@@ -2262,7 +2262,7 @@
     return &data->transientRegistry;
 }
 
-template<typename Registry> static inline void collectMatchingObserversForMutation(HashMap<MutationObserver*, MutationRecordDeliveryOptions>& observers, Registry* registry, Node& target, MutationObserver::MutationType type, const QualifiedName* attributeName)
+template<typename Registry> static inline void collectMatchingObserversForMutation(HashMap<Ref<MutationObserver>, MutationRecordDeliveryOptions>& observers, Registry* registry, Node& target, MutationObserver::MutationType type, const QualifiedName* attributeName)
 {
     if (!registry)
         return;
@@ -2270,7 +2270,7 @@
     for (auto& registration : *registry) {
         if (registration->shouldReceiveMutationFrom(target, type, attributeName)) {
             auto deliveryOptions = registration->deliveryOptions();
-            auto result = observers.add(&registration->observer(), deliveryOptions);
+            auto result = observers.add(registration->observer(), deliveryOptions);
             if (!result.isNewEntry)
                 result.iterator->value |= deliveryOptions;
         }
@@ -2277,9 +2277,9 @@
     }
 }
 
-HashMap<MutationObserver*, MutationRecordDeliveryOptions> Node::registeredMutationObservers(MutationObserver::MutationType type, const QualifiedName* attributeName)
+HashMap<Ref<MutationObserver>, MutationRecordDeliveryOptions> Node::registeredMutationObservers(MutationObserver::MutationType type, const QualifiedName* attributeName)
 {
-    HashMap<MutationObserver*, MutationRecordDeliveryOptions> result;
+    HashMap<Ref<MutationObserver>, MutationRecordDeliveryOptions> result;
     ASSERT((type == MutationObserver::Attributes && attributeName) || !attributeName);
     collectMatchingObserversForMutation(result, mutationObserverRegistry(), *this, type, attributeName);
     collectMatchingObserversForMutation(result, transientMutationObserverRegistry(), *this, type, attributeName);

Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/Node.h (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/Node.h	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/Node.h	2017-12-19 06:46:21 UTC (rev 226101)
@@ -534,7 +534,7 @@
     EventTargetData* eventTargetDataConcurrently() final;
     EventTargetData& ensureEventTargetData() final;
 
-    HashMap<MutationObserver*, MutationRecordDeliveryOptions> registeredMutationObservers(MutationObserver::MutationType, const QualifiedName* attributeName);
+    HashMap<Ref<MutationObserver>, MutationRecordDeliveryOptions> registeredMutationObservers(MutationObserver::MutationType, const QualifiedName* attributeName);
     void registerMutationObserver(MutationObserver&, MutationObserverOptions, const HashSet<AtomicString>& attributeFilter);
     void unregisterMutationObserver(MutationObserverRegistration&);
     void registerTransientMutationObserver(MutationObserverRegistration&);

Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/NodeFilter.h (226100 => 226101)


--- releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/NodeFilter.h	2017-12-19 06:21:20 UTC (rev 226100)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/dom/NodeFilter.h	2017-12-19 06:46:21 UTC (rev 226101)
@@ -38,6 +38,8 @@
 
     virtual CallbackResult<unsigned short> acceptNode(Node&) = 0;
 
+    virtual bool hasCallback() const = 0;
+
     /*
      * The following constants are returned by the acceptNode()
      * method:
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to