Title: [236825] trunk/Source/WebCore
Revision
236825
Author
rn...@webkit.org
Date
2018-10-03 20:55:12 -0700 (Wed, 03 Oct 2018)

Log Message

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:

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (236824 => 236825)


--- trunk/Source/WebCore/ChangeLog	2018-10-04 03:29:57 UTC (rev 236824)
+++ trunk/Source/WebCore/ChangeLog	2018-10-04 03:55:12 UTC (rev 236825)
@@ -1,3 +1,35 @@
+2018-10-03  Ryosuke Niwa  <rn...@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  <you...@apple.com>
 
         Add VP8 support to WebRTC

Modified: trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm (236824 => 236825)


--- trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	2018-10-04 03:29:57 UTC (rev 236824)
+++ trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	2018-10-04 03:55:12 UTC (rev 236825)
@@ -5151,7 +5151,14 @@
 
         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");

Modified: trunk/Source/WebCore/bindings/scripts/IDLAttributes.json (236824 => 236825)


--- trunk/Source/WebCore/bindings/scripts/IDLAttributes.json	2018-10-04 03:29:57 UTC (rev 236824)
+++ trunk/Source/WebCore/bindings/scripts/IDLAttributes.json	2018-10-04 03:55:12 UTC (rev 236825)
@@ -195,6 +195,9 @@
                 "url": "https://heycam.github.io/webidl/#EnforceRange"
             }
         },
+        "ResultField": {
+            "contextsAllowed": ["operation"]
+        },
         "ExportMacro": {
             "contextsAllowed": ["interface", "dictionary", "enum", "callback-function"],
             "values": ["WEBCORE_EXPORT", "WEBCORE_TESTSUPPORT_EXPORT"]

Modified: trunk/Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp (236824 => 236825)


--- trunk/Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp	2018-10-04 03:29:57 UTC (rev 236824)
+++ trunk/Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp	2018-10-04 03:55:12 UTC (rev 236825)
@@ -56,7 +56,12 @@
 #include "JSTestObj.h"
 #endif
 
+#if ENABLE(Condition22) || ENABLE(Condition23)
+#include "JSDOMConvertSequences.h"
+#include <_javascript_Core/JSArray.h>
+#endif
 
+
 namespace WebCore {
 using namespace JSC;
 
@@ -74,6 +79,9 @@
 #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 @@
 #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 @@
 
 #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)
 {

Modified: trunk/Source/WebCore/bindings/scripts/test/TestImplements.idl (236824 => 236825)


--- trunk/Source/WebCore/bindings/scripts/test/TestImplements.idl	2018-10-04 03:29:57 UTC (rev 236824)
+++ trunk/Source/WebCore/bindings/scripts/test/TestImplements.idl	2018-10-04 03:55:12 UTC (rev 236825)
@@ -47,5 +47,7 @@
 
     const unsigned short IMPLEMENTSCONSTANT1 = 1;
     [Reflect=CONST_IMPL] const unsigned short IMPLEMENTSCONSTANT2 = 2;
+
+    [ResultField=nodes] sequence<Node> takeNodes();
 };
 

Modified: trunk/Source/WebCore/dom/MutationObserver.cpp (236824 => 236825)


--- trunk/Source/WebCore/dom/MutationObserver.cpp	2018-10-04 03:29:57 UTC (rev 236824)
+++ trunk/Source/WebCore/dom/MutationObserver.cpp	2018-10-04 03:55:12 UTC (rev 236825)
@@ -110,13 +110,9 @@
     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 @@
     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);

Modified: trunk/Source/WebCore/dom/MutationObserver.h (236824 => 236825)


--- trunk/Source/WebCore/dom/MutationObserver.h	2018-10-04 03:29:57 UTC (rev 236824)
+++ trunk/Source/WebCore/dom/MutationObserver.h	2018-10-04 03:55:12 UTC (rev 236825)
@@ -86,7 +86,12 @@
     };
 
     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&);

Modified: trunk/Source/WebCore/dom/MutationObserver.idl (236824 => 236825)


--- trunk/Source/WebCore/dom/MutationObserver.idl	2018-10-04 03:29:57 UTC (rev 236824)
+++ trunk/Source/WebCore/dom/MutationObserver.idl	2018-10-04 03:55:12 UTC (rev 236825)
@@ -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 {
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to