Better handling of DocumentOrderedMap same-ID lookups during tree removals

third_party/WebKit/Source/core/dom/TreeScope.cpp

Index: third_party/WebKit/Source/core/dom/TreeScope.cpp
diff --git a/third_party/WebKit/Source/core/dom/TreeScope.cpp b/third_party/WebKit/Source/core/dom/TreeScope.cpp
index fe8f2ff9f4158bc2e2c17573f1389aacf4d19d1b..0c3f4e74c5f7ad9332d04f7fbc0a839513a4a61a 100644
--- a/third_party/WebKit/Source/core/dom/TreeScope.cpp
+++ b/third_party/WebKit/Source/core/dom/TreeScope.cpp
@@ -186,11 +186,44 @@ void TreeScope::addElementById(const AtomicString& elementId, Element* element)
     m_idTargetObserverRegistry->notifyObservers(elementId);
 }
 
+
+#if ENABLE(ASSERT)
+namespace {
+
+class RemovingElementIdScope {
+    STACK_ALLOCATED();
+public:
+    RemovingElementIdScope(DocumentOrderedMap* elementsById, const AtomicString& id)

[esprehn, 2015/12/18 00:42:26]

reference for elementsById

[sof, 2015/12/18 07:31:53]

Done.

+        : m_elementsById(elementsById)
+    {
+        m_elementsById->willBeRemovingId(&id);

[esprehn, 2015/12/18 00:42:26]

no ptr

[sof, 2015/12/18 07:31:53]

No; this has to be a Member<>.

[esprehn, 2015/12/18 07:53:40]

I was talking about the &id,  don't pass pointers to String or AtomicString.

+    }
+    ~RemovingElementIdScope()
+    {
+        m_elementsById->willBeRemovingId(nullptr);
+    }
+
+private:
+    RawPtrWillBeMember<DocumentOrderedMap> m_elementsById;
+};
+
+}
+#endif
+
 void TreeScope::removeElementById(const AtomicString& elementId, Element* element)
 {
     if (!m_elementsById)
         return;
     m_elementsById->remove(elementId, element);
+#if ENABLE(ASSERT)
+    // Register 'elementId' as being removed. This is done should observers
+    // attempt to also look it up, something that the underlying DocumentOrderedMap
+    // is incapable of answering precisely while an element (and its
+    // children) are being removed from the tree. This is _only_ done to avoid
+    // an assert in DocumentOrderedMap::get() from falsely triggering for such
+    // unusual and unexpected lookups.

[esprehn, 2015/12/18 00:42:26]

this doesn't seem right, can we notify observers afterwards instead?

[sof, 2015/12/18 07:31:53]

You would have to batch up all IDs having been dropped as a result of removing
element E (and its children), coordinated with mutation event dispatching
somehow, so not an easy thing to get right. The complexity isn't worth it.

+    RemovingElementIdScope removalScope(m_elementsById.get(), elementId);
+#endif
     m_idTargetObserverRegistry->notifyObservers(elementId);
 }