Oilpan: move custom element objects to the heap.

Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 17 matching lines @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "config.h"
 #include "core/dom/custom/CustomElementUpgradeCandidateMap.h"
 
 #include "core/dom/Element.h"
 
 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)
 {
     observe(element);
 
     UpgradeCandidateMap::AddResult result = m_upgradeCandidates.add(element, descriptor);
     ASSERT_UNUSED(result, result.isNewEntry);
 
     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);
 }
 
 void CustomElementUpgradeCandidateMap::remove(Element* element)

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

Who calls this method? As far as I see the code search, I couldn't find any
caller. If so, we can remove this method.

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

You're quite right, unused. Nice, removed and simplified.

 {
     unobserve(element);
     removeCommon(element);
 }
 
 void CustomElementUpgradeCandidateMap::elementWasDestroyed(Element* element)
 {
     CustomElementObserver::elementWasDestroyed(element);
     removeCommon(element);
 }
 
 void CustomElementUpgradeCandidateMap::removeCommon(Element* element)
 {
     UpgradeCandidateMap::iterator candidate = m_upgradeCandidates.find(element);
     ASSERT_WITH_SECURITY_IMPLICATION(candidate != m_upgradeCandidates.end());
 
     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.

 }
 
 void CustomElementUpgradeCandidateMap::elementDidFinishParsingChildren(Element* element)
 {
     // An upgrade candidate finished parsing; reorder so that eventual
     // upgrade order matches finished-parsing order.
     moveToEnd(element);
 }
 
 void CustomElementUpgradeCandidateMap::moveToEnd(Element* element)
 {
     UpgradeCandidateMap::iterator candidate = m_upgradeCandidates.find(element);
     ASSERT_WITH_SECURITY_IMPLICATION(candidate != m_upgradeCandidates.end());
 
     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);
 
-    for (ElementSet::const_iterator candidate = candidates.begin(); candidate != candidates.end(); ++candidate) {
+    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) {
         unobserve(*candidate);
         m_upgradeCandidates.remove(*candidate);
     }
-
     return candidates;
 }
 
+void CustomElementUpgradeCandidateMap::trace(Visitor* visitor)
+{
+    visitor->trace(m_upgradeCandidates);
+    visitor->trace(m_unresolvedDefinitions);
+    CustomElementObserver::trace(visitor);
 }
+
+}