MutationObserver wrapper should not be collected while still observing
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 Nov 2012 21:48:03 +0000 (21:48 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 Nov 2012 21:48:03 +0000 (21:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=102328

Patch by Elliott Sprehn <esprehn@chromium.org> on 2012-11-15
Reviewed by Adam Barth.

Source/WebCore:

Make MutationObserver an ActiveDOMObject so that the wrapper is not
collected while it is still observing the DOM. This is needed because
the wrapper is passed into the callback and expandos on the wrapper
should be preserved.

Test: fast/mutation/observer-wrapper-dropoff.html

* bindings/js/JSMutationObserverCustom.cpp:
(WebCore::JSMutationObserverConstructor::constructJSMutationObserver):
* bindings/v8/custom/V8MutationObserverCustom.cpp:
(WebCore::V8MutationObserver::constructorCallback):
* dom/MutationObserver.cpp:
(WebCore::MutationObserver::create):
(WebCore::MutationObserver::MutationObserver):
(WebCore::MutationObserver::observationStarted):
(WebCore::MutationObserver::observationEnded):
* dom/MutationObserver.h:
(WebCore):
* dom/MutationObserver.idl:

LayoutTests:

Test that a MutationObserver wrapper is not collected while the observer
is still observing since the wrapper must be passed into the callback
later.

* fast/mutation/observer-wrapper-dropoff-expected.txt: Added.
* fast/mutation/observer-wrapper-dropoff.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/mutation/observer-wrapper-dropoff-expected.txt [new file with mode: 0644]
LayoutTests/fast/mutation/observer-wrapper-dropoff.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSMutationObserverCustom.cpp
Source/WebCore/bindings/v8/custom/V8MutationObserverCustom.cpp
Source/WebCore/dom/MutationObserver.cpp
Source/WebCore/dom/MutationObserver.h
Source/WebCore/dom/MutationObserver.idl

index 348657f..4bd80c2 100644 (file)
@@ -1,3 +1,17 @@
+2012-11-15  Elliott Sprehn  <esprehn@chromium.org>
+
+        MutationObserver wrapper should not be collected while still observing
+        https://bugs.webkit.org/show_bug.cgi?id=102328
+
+        Reviewed by Adam Barth.
+
+        Test that a MutationObserver wrapper is not collected while the observer
+        is still observing since the wrapper must be passed into the callback
+        later.
+
+        * fast/mutation/observer-wrapper-dropoff-expected.txt: Added.
+        * fast/mutation/observer-wrapper-dropoff.html: Added.
+
 2012-11-15  David Grogan  <dgrogan@chromium.org>
 
         IndexedDB: setVersion test conversion batch 6
diff --git a/LayoutTests/fast/mutation/observer-wrapper-dropoff-expected.txt b/LayoutTests/fast/mutation/observer-wrapper-dropoff-expected.txt
new file mode 100644 (file)
index 0000000..c8c0f2e
--- /dev/null
@@ -0,0 +1,10 @@
+MutationObserver wrappers should survive GC for passing into the callback even if JS has lost references.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS observer.testProperty is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/mutation/observer-wrapper-dropoff.html b/LayoutTests/fast/mutation/observer-wrapper-dropoff.html
new file mode 100644 (file)
index 0000000..c5dbcbb
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+
+<script src="../js/resources/js-test-pre.js"></script>
+
+<script>
+description('MutationObserver wrappers should survive GC for passing into the callback even if JS has lost references.');
+
+jsTestIsAsync = true;
+
+function addObserver(node, fn) {
+    var observer = new WebKitMutationObserver(fn);
+    observer.testProperty = true;
+    observer.observe(node, {attributes:true});
+}
+
+onload = function() {
+    addObserver(document.body, function(records, observer) {
+        window.observer = observer;
+        shouldBe('observer.testProperty', 'true');
+        finishJSTest();
+    });
+
+    gc();
+
+    document.body.setAttribute('touch', 'the node');
+};
+</script>
+
+<script src="../js/resources/js-test-post.js"></script>
index 2cdcadb..b81e810 100644 (file)
@@ -1,3 +1,30 @@
+2012-11-15  Elliott Sprehn  <esprehn@chromium.org>
+
+        MutationObserver wrapper should not be collected while still observing
+        https://bugs.webkit.org/show_bug.cgi?id=102328
+
+        Reviewed by Adam Barth.
+
+        Make MutationObserver an ActiveDOMObject so that the wrapper is not
+        collected while it is still observing the DOM. This is needed because
+        the wrapper is passed into the callback and expandos on the wrapper
+        should be preserved.
+
+        Test: fast/mutation/observer-wrapper-dropoff.html
+
+        * bindings/js/JSMutationObserverCustom.cpp:
+        (WebCore::JSMutationObserverConstructor::constructJSMutationObserver):
+        * bindings/v8/custom/V8MutationObserverCustom.cpp:
+        (WebCore::V8MutationObserver::constructorCallback):
+        * dom/MutationObserver.cpp:
+        (WebCore::MutationObserver::create):
+        (WebCore::MutationObserver::MutationObserver):
+        (WebCore::MutationObserver::observationStarted):
+        (WebCore::MutationObserver::observationEnded):
+        * dom/MutationObserver.h:
+        (WebCore):
+        * dom/MutationObserver.idl:
+
 2012-11-15  Tony Chang  <tony@chromium.org>
 
         Generate Settings from a .in file
index ab5560d..5f7fe59 100644 (file)
@@ -54,8 +54,11 @@ EncodedJSValue JSC_HOST_CALL JSMutationObserverConstructor::constructJSMutationO
     }
 
     JSMutationObserverConstructor* jsConstructor = jsCast<JSMutationObserverConstructor*>(exec->callee());
-    RefPtr<MutationCallback> callback = JSMutationCallback::create(object, jsConstructor->globalObject());
-    return JSValue::encode(asObject(toJS(exec, jsConstructor->globalObject(), MutationObserver::create(callback.release()))));
+    RefPtr<JSMutationCallback> callback = JSMutationCallback::create(object, jsConstructor->globalObject());
+    RefPtr<MutationObserver> observer = MutationObserver::create(jsConstructor->globalObject()->scriptExecutionContext(), callback);
+    JSObject* wrapper = asObject(toJS(exec, jsConstructor->globalObject(), observer.release()));
+
+    return JSValue::encode(wrapper);
 }
 
 } // namespace WebCore
index f80ce59..0db65b1 100644 (file)
@@ -63,7 +63,7 @@ v8::Handle<v8::Value> V8MutationObserver::constructorCallback(const v8::Argument
     ScriptExecutionContext* context = getScriptExecutionContext();
 
     RefPtr<MutationCallback> callback = V8MutationCallback::create(arg, context);
-    RefPtr<MutationObserver> observer = MutationObserver::create(callback.release());
+    RefPtr<MutationObserver> observer = MutationObserver::create(context, callback.release());
 
     v8::Handle<v8::Object> wrapper = args.Holder();
     V8DOMWrapper::createDOMWrapper(observer.release(), &info, wrapper);
index 0c210fa..8032c21 100644 (file)
@@ -57,14 +57,17 @@ struct MutationObserver::ObserverLessThan {
     }
 };
 
-PassRefPtr<MutationObserver> MutationObserver::create(PassRefPtr<MutationCallback> callback)
+PassRefPtr<MutationObserver> MutationObserver::create(ScriptExecutionContext* context, PassRefPtr<MutationCallback> callback)
 {
     ASSERT(isMainThread());
-    return adoptRef(new MutationObserver(callback));
+    RefPtr<MutationObserver> observer = adoptRef(new MutationObserver(context, callback));
+    observer->suspendIfNeeded();
+    return observer.release();
 }
 
-MutationObserver::MutationObserver(PassRefPtr<MutationCallback> callback)
-    : m_callback(callback)
+MutationObserver::MutationObserver(ScriptExecutionContext* context, PassRefPtr<MutationCallback> callback)
+    : ActiveDOMObject(context, this)
+    , m_callback(callback)
     , m_priority(s_observerPriority++)
 {
 }
@@ -138,12 +141,14 @@ void MutationObserver::observationStarted(MutationObserverRegistration* registra
 {
     ASSERT(!m_registrations.contains(registration));
     m_registrations.add(registration);
+    setPendingActivity(this);
 }
 
 void MutationObserver::observationEnded(MutationObserverRegistration* registration)
 {
     ASSERT(m_registrations.contains(registration));
     m_registrations.remove(registration);
+    unsetPendingActivity(this);
 }
 
 typedef HashSet<RefPtr<MutationObserver> > MutationObserverSet;
index fadb378..d10b5ee 100644 (file)
@@ -33,6 +33,7 @@
 
 #if ENABLE(MUTATION_OBSERVERS)
 
+#include "ActiveDOMObject.h"
 #include <wtf/HashMap.h>
 #include <wtf/HashSet.h>
 #include <wtf/PassRefPtr.h>
@@ -48,13 +49,14 @@ class MutationCallback;
 class MutationObserverRegistration;
 class MutationRecord;
 class Node;
+class ScriptExecutionContext;
 
 typedef int ExceptionCode;
 
 typedef unsigned char MutationObserverOptions;
 typedef unsigned char MutationRecordDeliveryOptions;
 
-class MutationObserver : public RefCounted<MutationObserver> {
+class MutationObserver : public RefCounted<MutationObserver>, public ActiveDOMObject {
 public:
     enum MutationType {
         ChildList = 1 << 0,
@@ -74,10 +76,10 @@ public:
         CharacterDataOldValue = 1 << 6,
     };
 
-    static PassRefPtr<MutationObserver> create(PassRefPtr<MutationCallback>);
+    static PassRefPtr<MutationObserver> create(ScriptExecutionContext*, PassRefPtr<MutationCallback>);
     static void deliverAllMutations();
 
-    ~MutationObserver();
+    virtual ~MutationObserver();
 
     void observe(Node*, const Dictionary&, ExceptionCode&);
     Vector<RefPtr<MutationRecord> > takeRecords();
@@ -90,7 +92,7 @@ public:
 private:
     struct ObserverLessThan;
 
-    explicit MutationObserver(PassRefPtr<MutationCallback>);
+    MutationObserver(ScriptExecutionContext*, PassRefPtr<MutationCallback>);
     void deliver();
 
     static bool validateOptions(MutationObserverOptions);
index 1174f29..286e063 100644 (file)
@@ -29,6 +29,7 @@
  */
 
 [
+    ActiveDOMObject,
     Conditional=MUTATION_OBSERVERS,
     CustomConstructor,
     ConstructorParameters=1