[webkit-reviews] review granted: [Bug 68364] Protect against misuse of EventListenerIterator. : [Attachment 107864] Proposed patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 68364: Protect against misuse of EventListenerIterator.
https://bugs.webkit.org/show_bug.cgi?id=68364

Attachment 107864: Proposed patch v2
https://bugs.webkit.org/attachment.cgi?id=107864&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=107864&action=review


This approach will possibly assert too often. For hash tables we found that we
needed to invalidate the *iterators* when the hash map changed and complain if
they were used later when invalid. Disallowing modification of the map is a
stricter policy. For hash tables it was common to to have an outstanding
iterator that later falls out of scope and we didn’t want to have to re-code to
avoid that. But if we can live with this greater strictness here, that’s fine.

> Source/WebCore/dom/EventListenerMap.cpp:48
> +    : m_activeIterators(0)

Something that’s a count should not be named “iterators” it should be named
“count” or “number”.

> Source/WebCore/dom/EventListenerMap.h:98
> +#ifndef NDEBUG
> +    ~EventListenerIterator()
> +    {
> +	   if (m_map)
> +	       m_map->m_activeIterators--;
> +    }
> +#endif

It would be better to not have this function body be in the header.