Bug 1403377 - Destroy hash table iterator before modifying hash table. r=smaug
nsDOMAttributeMap::BlastSubtreeToPieces() uses a hash table iterator
to get an arbitrary element from a hash table. This marks the hash
table as being in use. It then modifies the hash table before the
iterator is destroyed, triggering an assertion. The fix is to restrict
the scope of the iterator so that it is destroyed before the hashtable
is modified.
The existing code is safe because the iterator is never used after the
hash table has been modified.
MozReview-Commit-ID: DUjqewvwEe7
new file mode 100644
--- /dev/null
+++ b/dom/base/crashtests/1403377.html
@@ -0,0 +1,18 @@
+<script>
+function fzfn(e){
+ let nc=window.frames[0].document.body.childElementCount;
+ let o=window.frames[0].document.body.childNodes[1%nc];
+ document.getElementById('b').appendChild(o.parentNode.removeChild(o));
+}
+window.onload=function(){
+ document.body.onerror=fzfn;
+ document.getElementById('c').addEventListener('DOMNodeRemoved', fzfn, true);
+ document.getElementById('c').attributes[0].name=='id';
+ window.frames[0].document.body.appendChild(document.getElementById('c'));
+ document.getElementById('a').innerHTML=document.createElement('multicol').outerHTML;
+}
+</script>
+<iframe></iframe>
+<script id='a'></script>
+<script id='b'></script>
+<script id='c'></script>
--- a/dom/base/crashtests/crashtests.list
+++ b/dom/base/crashtests/crashtests.list
@@ -220,8 +220,9 @@ load 1377826.html
skip-if(stylo&&isDebugBuild&&winWidget) load structured_clone_container_throws.html # Bug 1383845
load xhr_empty_datauri.html
load xhr_html_nullresponse.html
load 1383478.html
load 1383780.html
pref(clipboard.autocopy,true) load 1385272-1.html
load 1393806.html
load 1400701.html
+load 1403377.html
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -7786,25 +7786,29 @@ nsIDocument::GetCompatMode(nsString& aCo
void
nsDOMAttributeMap::BlastSubtreeToPieces(nsINode *aNode)
{
if (aNode->IsElement()) {
Element *element = aNode->AsElement();
const nsDOMAttributeMap *map = element->GetAttributeMap();
if (map) {
- // This non-standard style of iteration is presumably used because some
- // of the code in the loop body can trigger element removal, which
- // invalidates the iterator.
while (true) {
- auto iter = map->mAttributeCache.ConstIter();
- if (iter.Done()) {
- break;
+ nsCOMPtr<nsIAttribute> attr;
+ {
+ // Use an iterator to get an arbitrary attribute from the
+ // cache. The iterator must be destroyed before any other
+ // operations on mAttributeCache, to avoid hash table
+ // assertions.
+ auto iter = map->mAttributeCache.ConstIter();
+ if (iter.Done()) {
+ break;
+ }
+ attr = iter.UserData();
}
- nsCOMPtr<nsIAttribute> attr = iter.UserData();
NS_ASSERTION(attr.get(),
"non-nsIAttribute somehow made it into the hashmap?!");
BlastSubtreeToPieces(attr);
DebugOnly<nsresult> rv =
element->UnsetAttr(attr->NodeInfo()->NamespaceID(),
attr->NodeInfo()->NameAtom(),