Bug 134869 – Document::unregisterNodeListforInvalidation() and Document::unregisterCollection() have incorrect assertions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

Document::unregisterNodeListForInvalidation() incorrectly cooperates with Document::invalidateNodeListAndCollectionCaches()

[Zan Dobersek]

Created attachment 234829 [details]
Patch

[Darin Adler]

Comment on attachment 234829 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234829&action=review

> Source/WebCore/dom/Document.cpp:3498
> +    ASSERT(m_inInvalidateNodeListAndCollectionCaches && !m_listsInvalidatedAtDocument.size()
> +        || m_listsInvalidatedAtDocument.contains(&list));
> +    // There's no harm in removing an item that perhaps is not in the HashSet.
> +    m_listsInvalidatedAtDocument.remove(&list);

This looks OK, but I think it’s confusing. I would write this instead:

    ASSERT(m_inInvalidateNodeListAndCollectionCaches ? m_listsInvalidatedAtDocument.isEmpty() : m_listsInvalidatedAtDocument.contains(&list));
    m_listsInvalidatedAtDocument.remove(&list);

For some people the ?: might make it harder to read the assertion, but to me it makes it easier.

It also seems that unregisterCollection has the same bug. The two functions should be basically identical, but there are many arbitrary differences between which assertions they have and how the functions are structured.

Please fix the bug in unregisterCollection too.

[Darin Adler]

Comment on attachment 234829 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234829&action=review

> Source/WebCore/ChangeLog:31
> +        Adding move constructor and assignment operator for HashTable (r170995, rolled out in r170999)
> +        revealed that Document::unregisterNodeListForInvalidation() contains incorrect assertions when
> +        unregistering node lists for invalidations originating from
> +        Document::invalidateNodeListAndCollectionCaches().
> +
> +        Once HashTable gained proper support for move semantics the WTF::move() call in
> +        Document::invalidateNodeListAndCollectionCaches() left the m_listsInvalidatedAtDocument HashSet
> +        empty, while previously it would remain intact and WTF::move() would perform a copy of the
> +        underlying HashTable.
> +
> +        Previously Document::unregisterNodeListForInvalidation() would return early if that HashSet
> +        member was empty, asserting that Document::invalidateNodeListAndCollectionCaches() was
> +        somewhere up the call chain. I'm fairly certain this assertion and the early return were
> +        never made for a LiveNodeList argument that was registered for an invalidation. The following
> +        assertion, that the m_listsInvalidatedAtDocument does contain the pointer to the given
> +        LiveNodeList, was thus always true, so that element was removed from the HashSet and the list
> +        was unregistered.
> +
> +        With the support for move semantics in HashTable and the m_listsInvalidatedAtDocument HashSet
> +        being properly zeroed out, the assertion in Document::unregisterNodeListForInvalidation() should
> +        check that the unregistration originates from Document::invalidateNodeListAndCollectionCaches()
> +        and the m_listsInvalidatedAtDocument HashSet is empty, or that there is indeed a pointer to the
> +        given LiveNodeList object in that HashSet. The LiveNodeList is unregistered either way, and the
> +        corresponding item in the HashSet is removed.

Change log entry is *way* too long.

[Darin Adler]

Comment on attachment 234829 [details]
Patch

Hmm, on reflection I still don’t get what was going wrong even after reading all your comments. I presume we hit the m_inInvalidateNodeListAndCollectionCaches assertion. With what call stack?

[Zan Dobersek]

Created attachment 235121 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 235121 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235121&action=review

> Source/WebCore/dom/Document.cpp:3499
> +    m_listsInvalidatedAtDocument.remove(&list);

Why are we calling remove() on the list in the case the map is empty!?

[Darin Adler]

Comment on attachment 235121 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235121&action=review

>> Source/WebCore/dom/Document.cpp:3499
>> +    m_listsInvalidatedAtDocument.remove(&list);
> 
> Why are we calling remove() on the list in the case the map is empty!?

It’s harmless to remove something from an empty set; the boolean m_inInvalidateNodeListAndCollectionCaches is only compiled into assertions-enabled builds.

Let me turn the question around on you. Is avoiding this an important optimization?

[Ryosuke Niwa]

Comment on attachment 235121 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235121&action=review

>>> Source/WebCore/dom/Document.cpp:3499
>>> +    m_listsInvalidatedAtDocument.remove(&list);
>> 
>> Why are we calling remove() on the list in the case the map is empty!?
> 
> It’s harmless to remove something from an empty set; the boolean m_inInvalidateNodeListAndCollectionCaches is only compiled into assertions-enabled builds.
> 
> Let me turn the question around on you. Is avoiding this an important optimization?

Huh, I guess it's different from HashMap which, if I remember correctly, used to randomly crash or something.
I don't think this function needs to be fast since it's only called when a NodeList is removed which incurs some memory deallocation.

[Darin Adler]

(In reply to comment #8)
> Huh, I guess it's different from HashMap which, if I remember correctly, used to randomly crash or something.

Maybe you are thinking of trying to remove a null pointer? It’s safe to try to remove something that’s not in a map or set. No crash.

[Zan Dobersek]

Comment on attachment 235121 [details]
Patch

Clearing flags on attachment: 235121

Committed r171261: <http://trac.webkit.org/changeset/171261>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

I am seeing the ASSERT in Document::unregisterCollection loading apple.com.

[Joseph Pecoraro]

Filed bug 135168.