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..262fd682a2a1dcd18e60da936c4534fe228e12b2 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
+    // 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);

[haraken, 2014/05/26 15:40:23]

Just help me understand: As far as I see the code, it looks like we want to
remove CustomElementObserver from ElementObserverMap (in
CustomElementObserver.cpp) when the CustomElementObserver dies. Then, should the
ElementObserverMap be a map from a WeakMember<Element> to a
WeakMember<CustomElementObserver> ? Currently it's a map from a
WeakMember<Element> to a Member<CustomElementObserver>.

[sof, 2014/05/26 16:41:08]

The weakly held Elements keep the CustomElementObserver objects alive. In
non-Oilpan builds, this is triggered from ~Element, releasing the corresponding
*Observer and this dtor gets to run.

Why do we want to independently make that weak, when we already have the key as
being weak? I don't understand what you are asking/suggesting, I think.

[haraken, 2014/05/27 01:08:45]

Correct me if I'm wrong:
This code is not triggered from ~Element.

(a) When an Element dies, CustomElementUpgradeCandidateMap::remove is triggered
and the Element is unregistered from the ElementObserverMap.

(b) When an CustomElementObserver dies, ~CustomElementUpgradeCandidateMap is
triggered and the Elements observed by the CustomElementObserver are
unregistered from the ElementObserverMap.

Making the key of the ElementObserverMap weak takes care of the weakness for
(a), and making the value of the ElementObserverMap weak takes care of the
weakness for (b). Thus I think both key and value of the ElementObserverMap
needs to be weak.

What do you think?

[sof, 2014/05/27 05:29:51]

It is triggered by the call to wasDestroyed() in ~Element, which brings about
deletion
- Who holds onto the CustomElementObservers? ElementObserverMap.
- What is keeping it alive? The weak Element reference. 

Consequently, when the Element dies, the CustomElementObserver (which can be a
CustomElementUpgradeCandidateMap) is retired. i.e., a weak key in a hash table
will also remove its value.

I don't understand what it means to have weak value here or why you think it is
needed.

(This is triggered from ~Element in the non-Oilpan case.)

[haraken, 2014/05/27 05:47:30]

Sorry, I was misreading the call path to CustomElementUpgradeCandidateMap's
destructor. You're right; CustomElementUpgradeCandidateMap's destructor is
called in ~Element(), and then making the key of ElementObserverMap weak is
enough. Makes sense, and I'm sorry about the noise.

[sof, 2014/05/27 11:59:14]

That matches my understanding also. I've been comparing wrt ToT in terms of
leaks when doing this CL, and see no difference.

+#endif
 }
 
 void CustomElementUpgradeCandidateMap::add(const CustomElementDescriptor& descriptor, Element* element)
@@ -52,9 +61,9 @@ void CustomElementUpgradeCandidateMap::add(const CustomElementDescriptor& descri
     UnresolvedDefinitionMap::iterator it = m_unresolvedDefinitions.find(descriptor);
     ElementSet* elements;
     if (it == m_unresolvedDefinitions.end())
-        elements = &m_unresolvedDefinitions.add(descriptor, ElementSet()).storedValue->value;
+        elements = m_unresolvedDefinitions.add(descriptor, adoptPtrWillBeNoop(new ElementSet())).storedValue->value;
     else
-        elements = &it->value;
+        elements = it->value;
     elements->add(element);
 }
 
@@ -77,7 +86,7 @@ void CustomElementUpgradeCandidateMap::removeCommon(Element* element)
 
     UnresolvedDefinitionMap::iterator elements = m_unresolvedDefinitions.find(candidate->value);
     ASSERT_WITH_SECURITY_IMPLICATION(elements != m_unresolvedDefinitions.end());
-    elements->value.remove(element);
+    elements->value->remove(element);
     m_upgradeCandidates.remove(candidate);

[haraken, 2014/05/26 15:40:23]

Can this be candidate->remove() ? We won't want to look up the hash table twice.

[sof, 2014/05/26 16:41:08]

Can't be - this is removing it from another table.

 }
 
@@ -95,19 +104,28 @@ void CustomElementUpgradeCandidateMap::moveToEnd(Element* element)
 
     UnresolvedDefinitionMap::iterator elements = m_unresolvedDefinitions.find(candidate->value);
     ASSERT_WITH_SECURITY_IMPLICATION(elements != m_unresolvedDefinitions.end());
-    elements->value.appendOrMoveToLast(element);
+    elements->value->appendOrMoveToLast(element);
 }
 
-ListHashSet<Element*> CustomElementUpgradeCandidateMap::takeUpgradeCandidatesFor(const CustomElementDescriptor& descriptor)
+const CustomElementUpgradeCandidateMap::ElementSet* CustomElementUpgradeCandidateMap::takeUpgradeCandidatesFor(const CustomElementDescriptor& descriptor)
 {
-    const ListHashSet<Element*>& candidates = m_unresolvedDefinitions.take(descriptor);
+    const ElementSet* candidates = m_unresolvedDefinitions.take(descriptor);
+
+    if (!candidates)
+        return 0;

[haraken, 2014/05/26 15:40:23]

Can this happen? If no, we might want to keep ElementSet&.

[sof, 2014/05/26 15:46:18]

It will happen if you call registerElement() before there have been any uses of
the custom element.

[haraken, 2014/05/27 01:08:45]

Just help me understand: Before this CL, we were using ListHashSet&, which means
that it can't be 0. After this CL, it can be 0. Where does the difference come
from?

[sof, 2014/05/27 11:59:14]

hashSet.take()'s return type is HashTraits<ValueType>::PassOutType. If the
element is not present, it returns the value
HashTraits<ValueType>::emptyValue().

With HashMap<CustomElementDescriptor, ElementSet> that type is equal to
ElementSet and the value returned is ElementSet() (i.e, an empty hash set.)

With HashMap<CustomElementDescriptor, OwnPtrWillBeMember<ElementSet> > that type
is equal to either PassOwnPtr<ElementSet> or ElementSet*, returning nullptr.

 
-    for (ElementSet::const_iterator candidate = candidates.begin(); candidate != candidates.end(); ++candidate) {
+    for (ElementSet::const_iterator candidate = candidates->begin(); candidate != candidates->end(); ++candidate) {
         unobserve(*candidate);
         m_upgradeCandidates.remove(*candidate);
     }
-
     return candidates;
 }
 
+void CustomElementUpgradeCandidateMap::trace(Visitor* visitor)
+{
+    visitor->trace(m_upgradeCandidates);
+    visitor->trace(m_unresolvedDefinitions);
+    CustomElementObserver::trace(visitor);
+}
+
 }