Clear m_pendingTargets in MutationObserver::takeRecords
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Oct 2018 03:55:12 +0000 (03:55 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Oct 2018 03:55:12 +0000 (03:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=190240

Reviewed by Geoffrey Garen.

In r236781, we delayed the clearing of m_pendingTargets until the end of microtask to avoid a race between
mutation record's JS wrappers getting created and GC marking JS wrappers of elements in mutation records.

This patch shortens this delay to until mutation record's JS wrappers are created. Specifically, we make
MutationObserver::takeRecords() return a struct which has both pending targets hash set and the vector of
mutation records so that the hash set survives through the creation of JS wrappers for mutation records.

To do this, a new IDL extended attribute "ResultField" is introduced to specify the member variable in
which the result is stored.

No new tests. Unfortunately, this race condition appears to be impossible to capture in a regression test.

* bindings/scripts/CodeGeneratorJS.pm:
(GenerateOperationBodyDefinition):
* bindings/scripts/IDLAttributes.json:
* bindings/scripts/test/JS/JSTestInterface.cpp:
(WebCore::jsTestInterfacePrototypeFunctionTakeNodesBody):
(WebCore::jsTestInterfacePrototypeFunctionTakeNodes):
* bindings/scripts/test/TestImplements.idl: Added a test case.
* dom/MutationObserver.cpp:
(WebCore::MutationObserver::takeRecords):
(WebCore::MutationObserver::deliver):
* dom/MutationObserver.h:
* dom/MutationObserver.idl:

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

Source/WebCore/ChangeLog
Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
Source/WebCore/bindings/scripts/IDLAttributes.json
Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp
Source/WebCore/bindings/scripts/test/TestImplements.idl
Source/WebCore/dom/MutationObserver.cpp
Source/WebCore/dom/MutationObserver.h
Source/WebCore/dom/MutationObserver.idl

index 80cc128..8f841d6 100644 (file)
@@ -1,3 +1,35 @@
+2018-10-03  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Clear m_pendingTargets in MutationObserver::takeRecords
+        https://bugs.webkit.org/show_bug.cgi?id=190240
+
+        Reviewed by Geoffrey Garen.
+
+        In r236781, we delayed the clearing of m_pendingTargets until the end of microtask to avoid a race between
+        mutation record's JS wrappers getting created and GC marking JS wrappers of elements in mutation records.
+
+        This patch shortens this delay to until mutation record's JS wrappers are created. Specifically, we make
+        MutationObserver::takeRecords() return a struct which has both pending targets hash set and the vector of
+        mutation records so that the hash set survives through the creation of JS wrappers for mutation records.
+
+        To do this, a new IDL extended attribute "ResultField" is introduced to specify the member variable in
+        which the result is stored.
+
+        No new tests. Unfortunately, this race condition appears to be impossible to capture in a regression test.
+
+        * bindings/scripts/CodeGeneratorJS.pm:
+        (GenerateOperationBodyDefinition):
+        * bindings/scripts/IDLAttributes.json:
+        * bindings/scripts/test/JS/JSTestInterface.cpp:
+        (WebCore::jsTestInterfacePrototypeFunctionTakeNodesBody):
+        (WebCore::jsTestInterfacePrototypeFunctionTakeNodes):
+        * bindings/scripts/test/TestImplements.idl: Added a test case.
+        * dom/MutationObserver.cpp:
+        (WebCore::MutationObserver::takeRecords):
+        (WebCore::MutationObserver::deliver):
+        * dom/MutationObserver.h:
+        * dom/MutationObserver.idl:
+
 2018-10-03  Youenn Fablet  <youenn@apple.com>
 
         Add VP8 support to WebRTC
index e4a1e62..b29b887 100644 (file)
@@ -5151,7 +5151,14 @@ sub GenerateOperationBodyDefinition
 
         GenerateArgumentsCountCheck($outputArray, $operation, $interface, $indent);
         my $functionString = GenerateParametersCheck($outputArray, $operation, $interface, $functionImplementationName, $indent);
-        GenerateImplementationFunctionCall($outputArray, $operation, $interface, $functionString, $indent);
+
+        if ($operation->extendedAttributes->{ResultField}) {
+            my $resultName = $operation->extendedAttributes->{ResultField};
+            push(@$outputArray, "    auto implResult = $functionString;\n");
+            GenerateImplementationFunctionCall($outputArray, $operation, $interface, "WTFMove(implResult.$resultName)", $indent);
+        } else {
+            GenerateImplementationFunctionCall($outputArray, $operation, $interface, $functionString, $indent);
+        }
     }
 
     push(@$outputArray, "}\n\n");
index 3e14df8..2154619 100644 (file)
                 "url": "https://heycam.github.io/webidl/#EnforceRange"
             }
         },
+        "ResultField": {
+            "contextsAllowed": ["operation"]
+        },
         "ExportMacro": {
             "contextsAllowed": ["interface", "dictionary", "enum", "callback-function"],
             "values": ["WEBCORE_EXPORT", "WEBCORE_TESTSUPPORT_EXPORT"]
index 4c64c19..40cf7c9 100644 (file)
 #include "JSTestObj.h"
 #endif
 
+#if ENABLE(Condition22) || ENABLE(Condition23)
+#include "JSDOMConvertSequences.h"
+#include <JavaScriptCore/JSArray.h>
+#endif
+
 
 namespace WebCore {
 using namespace JSC;
@@ -74,6 +79,9 @@ JSC::EncodedJSValue JSC_HOST_CALL jsTestInterfacePrototypeFunctionImplementsMeth
 #if ENABLE(Condition22) || ENABLE(Condition23)
 JSC::EncodedJSValue JSC_HOST_CALL jsTestInterfaceConstructorFunctionImplementsMethod4(JSC::ExecState*);
 #endif
+#if ENABLE(Condition22) || ENABLE(Condition23)
+JSC::EncodedJSValue JSC_HOST_CALL jsTestInterfacePrototypeFunctionTakeNodes(JSC::ExecState*);
+#endif
 #if ENABLE(Condition11) || ENABLE(Condition12)
 JSC::EncodedJSValue JSC_HOST_CALL jsTestInterfacePrototypeFunctionSupplementalMethod1(JSC::ExecState*);
 #endif
@@ -342,6 +350,11 @@ static const HashTableValue JSTestInterfacePrototypeTableValues[] =
 #else
     { 0, 0, NoIntrinsic, { 0, 0 } },
 #endif
+#if ENABLE(Condition22) || ENABLE(Condition23)
+    { "takeNodes", static_cast<unsigned>(JSC::PropertyAttribute::Function), NoIntrinsic, { (intptr_t)static_cast<RawNativeFunction>(jsTestInterfacePrototypeFunctionTakeNodes), (intptr_t) (0) } },
+#else
+    { 0, 0, NoIntrinsic, { 0, 0 } },
+#endif
 #if ENABLE(Condition11) || ENABLE(Condition12)
     { "supplementalMethod1", static_cast<unsigned>(JSC::PropertyAttribute::Function), NoIntrinsic, { (intptr_t)static_cast<RawNativeFunction>(jsTestInterfacePrototypeFunctionSupplementalMethod1), (intptr_t) (0) } },
 #else
@@ -914,6 +927,23 @@ EncodedJSValue JSC_HOST_CALL jsTestInterfaceConstructorFunctionImplementsMethod4
 
 #endif
 
+#if ENABLE(Condition22) || ENABLE(Condition23)
+static inline JSC::EncodedJSValue jsTestInterfacePrototypeFunctionTakeNodesBody(JSC::ExecState* state, typename IDLOperation<JSTestInterface>::ClassParameter castedThis, JSC::ThrowScope& throwScope)
+{
+    UNUSED_PARAM(state);
+    UNUSED_PARAM(throwScope);
+    auto& impl = castedThis->wrapped();
+    auto implResult = impl.takeNodes();
+    return JSValue::encode(toJS<IDLSequence<IDLInterface<Node>>>(*state, *castedThis->globalObject(), WTFMove(implResult.nodes)));
+}
+
+EncodedJSValue JSC_HOST_CALL jsTestInterfacePrototypeFunctionTakeNodes(ExecState* state)
+{
+    return IDLOperation<JSTestInterface>::call<jsTestInterfacePrototypeFunctionTakeNodesBody>(*state, "takeNodes");
+}
+
+#endif
+
 #if ENABLE(Condition11) || ENABLE(Condition12)
 static inline JSC::EncodedJSValue jsTestInterfacePrototypeFunctionSupplementalMethod1Body(JSC::ExecState* state, typename IDLOperation<JSTestInterface>::ClassParameter castedThis, JSC::ThrowScope& throwScope)
 {
index b2a0abb..76f9f21 100644 (file)
@@ -47,5 +47,7 @@
 
     const unsigned short IMPLEMENTSCONSTANT1 = 1;
     [Reflect=CONST_IMPL] const unsigned short IMPLEMENTSCONSTANT2 = 2;
+
+    [ResultField=nodes] sequence<Node> takeNodes();
 };
 
index e33aad3..10486ae 100644 (file)
@@ -110,13 +110,9 @@ ExceptionOr<void> MutationObserver::observe(Node& node, const Init& init)
     return { };
 }
 
-Vector<Ref<MutationRecord>> MutationObserver::takeRecords()
+auto MutationObserver::takeRecords() -> TakenRecords
 {
-    Vector<Ref<MutationRecord>> records;
-    records.swap(m_records);
-    // Don't clear m_pendingTargets here because we can collect JS wrappers
-    // between the time takeRecords is called and nodes in records are accesssed.
-    return records;
+    return { WTFMove(m_records), WTFMove(m_pendingTargets) };
 }
 
 void MutationObserver::disconnect()
@@ -239,8 +235,10 @@ void MutationObserver::deliver()
     for (auto& registration : transientRegistrations)
         nodesToKeepAlive.append(registration->takeTransientRegistrations());
 
-    if (m_records.isEmpty())
+    if (m_records.isEmpty()) {
+        ASSERT(m_pendingTargets.isEmpty());
         return;
+    }
 
     Vector<Ref<MutationRecord>> records;
     records.swap(m_records);
index 2798d58..b0817f0 100644 (file)
@@ -86,7 +86,12 @@ public:
     };
 
     ExceptionOr<void> observe(Node&, const Init&);
-    Vector<Ref<MutationRecord>> takeRecords();
+    
+    struct TakenRecords {
+        Vector<Ref<MutationRecord>> records;
+        HashSet<GCReachableRef<Node>> pendingTargets;
+    };
+    TakenRecords takeRecords();
     void disconnect();
 
     void observationStarted(MutationObserverRegistration&);
index 1b52a70..e9994fc 100644 (file)
@@ -37,7 +37,7 @@
 ] interface MutationObserver {
     [MayThrowException] void observe(Node target, optional MutationObserverInit options);
     void disconnect();
-    sequence<MutationRecord> takeRecords();
+    [ResultField=records] sequence<MutationRecord> takeRecords();
 };
 
 dictionary MutationObserverInit {