Oilpan: move custom element objects to the heap.

Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp

Index: Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp
diff --git a/Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp b/Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp
index ccbbed39b8fba00d16423aee900933929ea26b33..ed4b003be74ab244e40292a7194f2dd44b173e57 100644
--- a/Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp
+++ b/Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp
@@ -35,11 +35,20 @@
 
 namespace WebCore {
 
+PassOwnPtrWillBeRawPtr<CustomElementUpgradeCandidateMap> CustomElementUpgradeCandidateMap::create()
+{
+    return adoptPtrWillBeNoop(new CustomElementUpgradeCandidateMap());
+}
+
 CustomElementUpgradeCandidateMap::~CustomElementUpgradeCandidateMap()
 {
+#if !ENABLE(OILPAN)
+    // With Oilpan enabled, the observer table keeps a weak reference to the

[Mads Ager (chromium), 2014/05/22 08:33:27]

The problem is if we have a strong pointer to the element in the upgrade
candidate map. Then the element will not die while we are alive and we will not
die because the element does not die and therefore there is a strong pointer to
us in a global table?

[sof, 2014/05/25 16:30:01]

The registration context (a member on the Document) keeps an upgrade candidate
map. Once the Document is GCed, the candidate map is presumably no longer
root-reachable and it will also be. This seems no different from non-Oilpan, I
think.

What is the semantics of weak hash tables as implemented here? If the key is
weak, does its value keep the key alive if it also references the key? The
validity of _only_ keeping an Element weak pointer on the ElementObserverMap
hinges on it not keeping it alive. If it does, then we ought to have a weak
reference on the UpgradeCandidateMap. If it doesn't, then possibly.

IOW, it'd be good to know the weak hash table semantics here before weak'ifying
Element references in CustomElementUpgradeCandidateMap.

[Mads Ager (chromium), 2014/05/26 10:08:26]

That is exactly the right question to ask. :-)

And yes, the semantics of weak maps is that we always mark from Members and if
we end up marking the WeakMember part of the map in that marking we will leave
the mapping in there.

The other style of weak map (where you only mark the value if the key is
reachable) require a fix point algorithm and we don't have them. We could add
them but would prefer not to.

[sof, 2014/05/26 14:21:25]

Thanks, that's really useful information. So all the Element references from
CustomElementUpgradeCandidateMap need to be weak to prevent Element being
retained when they shouldn't be.

+    // element; no need for explicit removal.
     UpgradeCandidateMap::const_iterator::Keys end = m_upgradeCandidates.end().keys();
     for (UpgradeCandidateMap::const_iterator::Keys it = m_upgradeCandidates.begin().keys(); it != end; ++it)
         unobserve(*it);
+#endif
 }
 
 void CustomElementUpgradeCandidateMap::add(const CustomElementDescriptor& descriptor, Element* element)
@@ -98,9 +107,9 @@ void CustomElementUpgradeCandidateMap::moveToEnd(Element* element)
     elements->value.appendOrMoveToLast(element);
 }
 
-ListHashSet<Element*> CustomElementUpgradeCandidateMap::takeUpgradeCandidatesFor(const CustomElementDescriptor& descriptor)
+CustomElementUpgradeCandidateMap::ElementSet CustomElementUpgradeCandidateMap::takeUpgradeCandidatesFor(const CustomElementDescriptor& descriptor)
 {
-    const ListHashSet<Element*>& candidates = m_unresolvedDefinitions.take(descriptor);
+    const ElementSet& candidates = m_unresolvedDefinitions.take(descriptor);
 
     for (ElementSet::const_iterator candidate = candidates.begin(); candidate != candidates.end(); ++candidate) {
         unobserve(*candidate);
@@ -110,4 +119,11 @@ ListHashSet<Element*> CustomElementUpgradeCandidateMap::takeUpgradeCandidatesFor
     return candidates;
 }
 
+void CustomElementUpgradeCandidateMap::trace(Visitor* visitor)
+{
+    visitor->trace(m_upgradeCandidates);
+    visitor->trace(m_unresolvedDefinitions);
+    CustomElementObserver::trace(visitor);
+}
+
 }