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

[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)
 {
     observe(element);
 
     UpgradeCandidateMap::AddResult result = m_upgradeCandidates.add(element, descriptor);
     ASSERT_UNUSED(result, result.isNewEntry);
 
     UnresolvedDefinitionMap::iterator it = m_unresolvedDefinitions.find(descriptor);
@@ skipping 38 matching lines @@
 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);
 }
 
-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);
         m_upgradeCandidates.remove(*candidate);
     }
 
     return candidates;
 }
 
+void CustomElementUpgradeCandidateMap::trace(Visitor* visitor)
+{
+    visitor->trace(m_upgradeCandidates);
+    visitor->trace(m_unresolvedDefinitions);
+    CustomElementObserver::trace(visitor);
 }
+
+}