[V8] Use implicit references instead of object groups to keep registered MutationObservers alive
https://bugs.webkit.org/show_bug.cgi?id=111382
Reviewed by Adam Barth.
.:
* ManualTests/mutation-observer-leaks-nodes.html: Added.
Source/WebCore:
Two-phase approach to implicit references: after grouping objects
together, add an implicit reference between each registered node's
group and the MutationObserver's group (which includes wrappers from
all worlds).
Also changed many uses of v8::Value to v8::Object where we know we're
dealing with Object and the V8 API expects them.
Test: ManualTests/mutation-observer-leaks-nodes.html
* bindings/v8/V8GCController.cpp:
(WebCore::ImplicitConnection::ImplicitConnection):
(WebCore::ImplicitConnection::wrapper):
(ImplicitConnection):
(WebCore::ImplicitReference::ImplicitReference): Wrapper class holding a parent who should have an implicit reference to a child.
(ImplicitReference):
(WebCore::operator<): Needed for std::sort() call to avoid the overhead of using a HashMap
(WebCore::WrapperGrouper::addObjectWrapperToGroup):
(WebCore::WrapperGrouper::addNodeWrapperToGroup):
(WebCore::WrapperGrouper::addImplicitReference):
(WrapperGrouper):
(WebCore::WrapperGrouper::apply):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@144994 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/ChangeLog b/ChangeLog
index 830a29d..0d6640b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2013-03-06 Adam Klein <adamk@chromium.org>
+
+ [V8] Use implicit references instead of object groups to keep registered MutationObservers alive
+ https://bugs.webkit.org/show_bug.cgi?id=111382
+
+ Reviewed by Adam Barth.
+
+ * ManualTests/mutation-observer-leaks-nodes.html: Added.
+
2013-03-06 Gustavo Noronha Silva <gns@gnome.org>
Build fix. Fixes problems building code that uses deprecated functions from GTK+ 2,
diff --git a/ManualTests/mutation-observer-leaks-nodes.html b/ManualTests/mutation-observer-leaks-nodes.html
new file mode 100644
index 0000000..0efc6e8
--- /dev/null
+++ b/ManualTests/mutation-observer-leaks-nodes.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<body>
+<script>
+testRunner.dumpAsText();
+var count = 100;
+var observer = new MutationObserver(function(){});
+for (var i = 0; i < count; i++) {
+ var span = document.createElement('span');
+ observer.observe(span, {attributes:true});
+};
+GCController.collect();
+</script>
+<p>Number of leaked nodes reported by DRT should be less than 100</p>
+</body>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index d152af2..045fccc 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,33 @@
+2013-03-06 Adam Klein <adamk@chromium.org>
+
+ [V8] Use implicit references instead of object groups to keep registered MutationObservers alive
+ https://bugs.webkit.org/show_bug.cgi?id=111382
+
+ Reviewed by Adam Barth.
+
+ Two-phase approach to implicit references: after grouping objects
+ together, add an implicit reference between each registered node's
+ group and the MutationObserver's group (which includes wrappers from
+ all worlds).
+
+ Also changed many uses of v8::Value to v8::Object where we know we're
+ dealing with Object and the V8 API expects them.
+
+ Test: ManualTests/mutation-observer-leaks-nodes.html
+
+ * bindings/v8/V8GCController.cpp:
+ (WebCore::ImplicitConnection::ImplicitConnection):
+ (WebCore::ImplicitConnection::wrapper):
+ (ImplicitConnection):
+ (WebCore::ImplicitReference::ImplicitReference): Wrapper class holding a parent who should have an implicit reference to a child.
+ (ImplicitReference):
+ (WebCore::operator<): Needed for std::sort() call to avoid the overhead of using a HashMap
+ (WebCore::WrapperGrouper::addObjectWrapperToGroup):
+ (WebCore::WrapperGrouper::addNodeWrapperToGroup):
+ (WebCore::WrapperGrouper::addImplicitReference):
+ (WrapperGrouper):
+ (WebCore::WrapperGrouper::apply):
+
2013-03-06 Ankur Taly <ataly@google.com>
Modify log method in V8DOMActivityLogger so that the apiName and
diff --git a/Source/WebCore/bindings/v8/V8GCController.cpp b/Source/WebCore/bindings/v8/V8GCController.cpp
index fd01a9e..f387752 100644
--- a/Source/WebCore/bindings/v8/V8GCController.cpp
+++ b/Source/WebCore/bindings/v8/V8GCController.cpp
@@ -49,13 +49,13 @@
class ImplicitConnection {
public:
- ImplicitConnection(void* root, v8::Persistent<v8::Value> wrapper)
+ ImplicitConnection(void* root, v8::Persistent<v8::Object> wrapper)
: m_root(root)
, m_wrapper(wrapper)
, m_rootNode(0)
{
}
- ImplicitConnection(Node* root, v8::Persistent<v8::Value> wrapper)
+ ImplicitConnection(Node* root, v8::Persistent<v8::Object> wrapper)
: m_root(root)
, m_wrapper(wrapper)
, m_rootNode(root)
@@ -63,7 +63,7 @@
}
void* root() const { return m_root; }
- v8::Persistent<v8::Value> wrapper() const { return m_wrapper; }
+ v8::Persistent<v8::Object> wrapper() const { return m_wrapper; }
PassOwnPtr<RetainedObjectInfo> retainedObjectInfo()
{
@@ -74,7 +74,7 @@
private:
void* m_root;
- v8::Persistent<v8::Value> m_wrapper;
+ v8::Persistent<v8::Object> m_wrapper;
Node* m_rootNode;
};
@@ -83,6 +83,22 @@
return left.root() < right.root();
}
+struct ImplicitReference {
+ ImplicitReference(void* parent, v8::Persistent<v8::Object> child)
+ : parent(parent)
+ , child(child)
+ {
+ }
+
+ void* parent;
+ v8::Persistent<v8::Object> child;
+};
+
+bool operator<(const ImplicitReference& left, const ImplicitReference& right)
+{
+ return left.parent < right.parent;
+}
+
class WrapperGrouper {
public:
WrapperGrouper()
@@ -90,16 +106,22 @@
m_liveObjects.append(V8PerIsolateData::current()->ensureLiveRoot());
}
- void addObjectWrapperToGroup(void* root, v8::Persistent<v8::Value> wrapper)
+ void addObjectWrapperToGroup(void* root, v8::Persistent<v8::Object> wrapper)
{
m_connections.append(ImplicitConnection(root, wrapper));
}
- void addNodeWrapperToGroup(Node* root, v8::Persistent<v8::Value> wrapper)
+ void addNodeWrapperToGroup(Node* root, v8::Persistent<v8::Object> wrapper)
{
m_connections.append(ImplicitConnection(root, wrapper));
}
+ void addImplicitReference(void* parent, v8::Persistent<v8::Object> child)
+ {
+ m_references.append(ImplicitReference(parent, child));
+ m_rootGroupMap.add(parent, v8::Persistent<v8::Object>());
+ }
+
void keepAlive(v8::Persistent<v8::Value> wrapper)
{
m_liveObjects.append(wrapper);
@@ -115,6 +137,7 @@
size_t i = 0;
while (i < m_connections.size()) {
void* root = m_connections[i].root();
+ v8::Persistent<v8::Object> groupRepresentativeWrapper = m_connections[i].wrapper();
OwnPtr<RetainedObjectInfo> retainedObjectInfo = m_connections[i].retainedObjectInfo();
do {
@@ -124,13 +147,37 @@
if (group.size() > 1)
v8::V8::AddObjectGroup(group.data(), group.size(), retainedObjectInfo.leakPtr());
+ HashMap<void*, v8::Persistent<v8::Object> >::iterator iter = m_rootGroupMap.find(root);
+ if (iter != m_rootGroupMap.end())
+ iter->value = groupRepresentativeWrapper;
+
group.shrink(0);
}
+
+ std::sort(m_references.begin(), m_references.end());
+ i = 0;
+ while (i < m_references.size()) {
+ void* parent = m_references[i].parent;
+ v8::Persistent<v8::Object> parentWrapper = m_rootGroupMap.get(parent);
+ if (parentWrapper.IsEmpty()) {
+ ++i;
+ continue;
+ }
+
+ Vector<v8::Persistent<v8::Value> > children;
+ do {
+ children.append(m_references[i++].child);
+ } while (i < m_references.size() && parent == m_references[i].parent);
+
+ v8::V8::AddImplicitReferences(parentWrapper, children.data(), children.size());
+ }
}
private:
Vector<v8::Persistent<v8::Value> > m_liveObjects;
Vector<ImplicitConnection> m_connections;
+ Vector<ImplicitReference> m_references;
+ HashMap<void*, v8::Persistent<v8::Object> > m_rootGroupMap;
};
// FIXME: This should use opaque GC roots.
@@ -316,7 +363,7 @@
MutationObserver* observer = static_cast<MutationObserver*>(object);
HashSet<Node*> observedNodes = observer->getObservedNodes();
for (HashSet<Node*>::iterator it = observedNodes.begin(); it != observedNodes.end(); ++it)
- m_grouper.addObjectWrapperToGroup(V8GCController::opaqueRootForGC(*it, m_isolate), wrapper);
+ m_grouper.addImplicitReference(V8GCController::opaqueRootForGC(*it, m_isolate), wrapper);
} else {
ActiveDOMObject* activeDOMObject = type->toActiveDOMObject(wrapper);
if (activeDOMObject && activeDOMObject->hasPendingActivity())
