REGRESSION(r150187): updateIdForTreeScope may not be called inside shadow trees

https://bugs.webkit.org/show_bug.cgi?id=149364

Reviewed by Antti Koivisto.

Since the tree scope is set to that of Document's inside removeBetween when a node is removed from a shadow tree,
oldScope != &treeScope() was already true inside Element::removedFrom. This can introduce an inconsistency in
DocumentOrderedMap which could result in a crash. Fixed the bug by checking it against document(), which is the
behavior we had prior to r150187.

Also added a consistency check in DocumentOrderedMap to catch bugs like this.

No new tests. New assertions fail in existing tests without this fix.

* dom/DocumentOrderedMap.cpp:
(WebCore::DocumentOrderedMap::add):
(WebCore::DocumentOrderedMap::remove):
(WebCore::DocumentOrderedMap::get):
* dom/DocumentOrderedMap.h:
* dom/Element.cpp:
(WebCore::Element::removedFrom):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@190007 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 28321edc2e0a3dcd0e5a867a724aa43bab289809..23a0622760cb4409bf6125d55a50ad097f9b5ec0 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,27 @@
+2015-09-18  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION(r150187): updateIdForTreeScope may not be called inside shadow trees
+        https://bugs.webkit.org/show_bug.cgi?id=149364
+
+        Reviewed by Antti Koivisto.
+
+        Since the tree scope is set to that of Document's inside removeBetween when a node is removed from a shadow tree,
+        oldScope != &treeScope() was already true inside Element::removedFrom. This can introduce an inconsistency in
+        DocumentOrderedMap which could result in a crash. Fixed the bug by checking it against document(), which is the
+        behavior we had prior to r150187.
+
+        Also added a consistency check in DocumentOrderedMap to catch bugs like this.
+
+        No new tests. New assertions fail in existing tests without this fix.
+
+        * dom/DocumentOrderedMap.cpp:
+        (WebCore::DocumentOrderedMap::add):
+        (WebCore::DocumentOrderedMap::remove):
+        (WebCore::DocumentOrderedMap::get):
+        * dom/DocumentOrderedMap.h:
+        * dom/Element.cpp:
+        (WebCore::Element::removedFrom):
+
 2015-09-18  Antti Koivisto  <antti@apple.com>
 
         Don't create renderers for children of shadow host

diff --git a/Source/WebCore/dom/DocumentOrderedMap.cpp b/Source/WebCore/dom/DocumentOrderedMap.cpp
index b0883d42ed98131983b0a2e2a700808cc9311bad..6ceb576c8db0d95fb4e506cb9596e691e572e6b6 100644 (file)
--- a/Source/WebCore/dom/DocumentOrderedMap.cpp
+++ b/Source/WebCore/dom/DocumentOrderedMap.cpp
@@ -51,13 +51,20 @@ void DocumentOrderedMap::add(const AtomicStringImpl& key, Element& element, cons
     UNUSED_PARAM(treeScope);
     ASSERT_WITH_SECURITY_IMPLICATION(element.isInTreeScope());
     ASSERT_WITH_SECURITY_IMPLICATION(treeScope.rootNode().containsIncludingShadowDOM(&element));
+
     if (!element.isInTreeScope())
         return;
     Map::AddResult addResult = m_map.add(&key, MapEntry(&element));
+    MapEntry& entry = addResult.iterator->value;
+
+#ifndef NDEBUG
+    ASSERT_WITH_SECURITY_IMPLICATION(!entry.registeredElements.contains(&element));
+    entry.registeredElements.add(&element);
+#endif
+
     if (addResult.isNewEntry)
         return;
 
-    MapEntry& entry = addResult.iterator->value;
     ASSERT_WITH_SECURITY_IMPLICATION(entry.count);
     entry.element = nullptr;
     entry.count++;
@@ -68,11 +75,13 @@ void DocumentOrderedMap::remove(const AtomicStringImpl& key, Element& element)
 {
     m_map.checkConsistency();
     auto it = m_map.find(&key);
+
     ASSERT_WITH_SECURITY_IMPLICATION(it != m_map.end());
     if (it == m_map.end())
         return;
-    MapEntry& entry = it->value;
 
+    MapEntry& entry = it->value;
+    ASSERT_WITH_SECURITY_IMPLICATION(entry.registeredElements.remove(&element));
     ASSERT_WITH_SECURITY_IMPLICATION(entry.count);
     if (entry.count == 1) {
         ASSERT_WITH_SECURITY_IMPLICATION(!entry.element || entry.element == &element);
@@ -99,6 +108,7 @@ inline Element* DocumentOrderedMap::get(const AtomicStringImpl& key, const TreeS
     if (entry.element) {
         ASSERT_WITH_SECURITY_IMPLICATION(entry.element->isInTreeScope());
         ASSERT_WITH_SECURITY_IMPLICATION(&entry.element->treeScope() == &scope);
+        ASSERT_WITH_SECURITY_IMPLICATION(entry.registeredElements.contains(entry.element));
         return entry.element;
     }
 
@@ -109,6 +119,7 @@ inline Element* DocumentOrderedMap::get(const AtomicStringImpl& key, const TreeS
         entry.element = &element;
         ASSERT_WITH_SECURITY_IMPLICATION(element.isInTreeScope());
         ASSERT_WITH_SECURITY_IMPLICATION(&element.treeScope() == &scope);
+        ASSERT_WITH_SECURITY_IMPLICATION(entry.registeredElements.contains(entry.element));
         return &element;
     }
     ASSERT_NOT_REACHED();

diff --git a/Source/WebCore/dom/DocumentOrderedMap.h b/Source/WebCore/dom/DocumentOrderedMap.h
index af1b48ea797209fb0ed016ec43fe6ce659ed833e..5a70f223cfe1b3c6feb3fb61199500481c92b5a1 100644 (file)
--- a/Source/WebCore/dom/DocumentOrderedMap.h
+++ b/Source/WebCore/dom/DocumentOrderedMap.h
@@ -33,6 +33,7 @@
 
 #include <wtf/HashCountedSet.h>
 #include <wtf/HashMap.h>
+#include <wtf/HashSet.h>
 #include <wtf/Vector.h>
 #include <wtf/text/AtomicStringImpl.h>
 
@@ -84,6 +85,9 @@ private:
         Element* element;
         unsigned count;
         Vector<Element*> orderedList;
+#ifndef NDEBUG
+        HashSet<Element*> registeredElements;
+#endif
     };
 
     typedef HashMap<const AtomicStringImpl*, MapEntry> Map;

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
index 9f416a6dd8a47ce5a55f7eba1483d0468d2d6475..5f22934a9a47922826564ac2c826f19d9adf3475 100644 (file)
--- a/Source/WebCore/dom/Element.cpp
+++ b/Source/WebCore/dom/Element.cpp
@@ -1566,7 +1566,7 @@ void Element::removedFrom(ContainerNode& insertionPoint)
     if (insertionPoint.isInTreeScope()) {
         TreeScope* oldScope = &insertionPoint.treeScope();
         HTMLDocument* oldDocument = inDocument() && is<HTMLDocument>(oldScope->documentScope()) ? &downcast<HTMLDocument>(oldScope->documentScope()) : nullptr;
-        if (oldScope != &treeScope() || !isInTreeScope())
+        if (!isInTreeScope() || &treeScope() != &document())
             oldScope = nullptr;
 
         const AtomicString& idValue = getIdAttribute();