Bug 134869 - Document::unregisterNodeListforInvalidation() and Document::unregisterCollection() have incorrect assertions
Summary: Document::unregisterNodeListforInvalidation() and Document::unregisterCollect...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 130772
  Show dependency treegraph
 
Reported: 2014-07-13 04:14 PDT by Zan Dobersek
Modified: 2014-07-22 12:24 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.67 KB, patch)
2014-07-13 04:45 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (3.15 KB, patch)
2014-07-18 03:41 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-07-13 04:14:15 PDT
Document::unregisterNodeListForInvalidation() incorrectly cooperates with Document::invalidateNodeListAndCollectionCaches()
Comment 1 Zan Dobersek 2014-07-13 04:45:14 PDT
Created attachment 234829 [details]
Patch
Comment 2 Darin Adler 2014-07-13 10:00:19 PDT
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.
Comment 3 Darin Adler 2014-07-13 10:00:35 PDT
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.
Comment 4 Darin Adler 2014-07-13 10:04:05 PDT
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?
Comment 5 Zan Dobersek 2014-07-18 03:41:32 PDT
Created attachment 235121 [details]
Patch
Comment 6 Ryosuke Niwa 2014-07-18 11:30:10 PDT
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!?
Comment 7 Darin Adler 2014-07-18 12:19:13 PDT
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?
Comment 8 Ryosuke Niwa 2014-07-18 13:03:50 PDT
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.
Comment 9 Darin Adler 2014-07-18 17:47:14 PDT
(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.
Comment 10 Zan Dobersek 2014-07-19 03:09:38 PDT
Comment on attachment 235121 [details]
Patch

Clearing flags on attachment: 235121

Committed r171261: <http://trac.webkit.org/changeset/171261>
Comment 11 Zan Dobersek 2014-07-19 03:10:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Joseph Pecoraro 2014-07-22 12:13:50 PDT
I am seeing the ASSERT in Document::unregisterCollection loading apple.com.
Comment 13 Joseph Pecoraro 2014-07-22 12:24:26 PDT
Filed bug 135168.