Bug 68364 – Protect against misuse of EventListenerIterator.

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Andreas Kling]

Spin-off from bug 68105, let's catch people modifying EventListenerMap while an EventListenerIterator is active (in debug mode.)

[Andreas Kling]

Created attachment 107861 [details]
Proposed patch

[WebKit Review Bot]

Attachment 107861 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/EventListenerMap.h:98:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 107861 [details]
Proposed patch

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

> Source/WebCore/dom/EventListenerMap.h:85
> +    HashSet<EventListenerIterator*> m_iterators;

I suppose this could simply be an int tracking the number of iterators, actually.

[Andreas Kling]

Created attachment 107864 [details]
Proposed patch v2

Same patch, but simply counting the number of active iterators. Sorry about the noise.

[Darin Adler]

Comment on attachment 107864 [details]
Proposed patch v2

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.

[Andreas Kling]

Created attachment 108134 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 108134 [details]
Patch for landing

Clearing flags on attachment: 108134

Committed r95619: <http://trac.webkit.org/changeset/95619>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.