Bug 68364 - Protect against misuse of EventListenerIterator.
Summary: Protect against misuse of EventListenerIterator.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-19 08:29 PDT by Andreas Kling
Modified: 2011-09-21 06:07 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (5.13 KB, patch)
2011-09-19 08:35 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch v2 (5.01 KB, patch)
2011-09-19 08:56 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch for landing (5.33 KB, patch)
2011-09-21 04:51 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2011-09-19 08:29:07 PDT
Spin-off from bug 68105, let's catch people modifying EventListenerMap while an EventListenerIterator is active (in debug mode.)
Comment 1 Andreas Kling 2011-09-19 08:35:15 PDT
Created attachment 107861 [details]
Proposed patch
Comment 2 WebKit Review Bot 2011-09-19 08:37:24 PDT
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.
Comment 3 Andreas Kling 2011-09-19 08:39:47 PDT
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.
Comment 4 Andreas Kling 2011-09-19 08:56:07 PDT
Created attachment 107864 [details]
Proposed patch v2

Same patch, but simply counting the number of active iterators. Sorry about the noise.
Comment 5 Darin Adler 2011-09-19 16:32:50 PDT
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.
Comment 6 Andreas Kling 2011-09-21 04:51:07 PDT
Created attachment 108134 [details]
Patch for landing
Comment 7 WebKit Review Bot 2011-09-21 06:07:25 PDT
Comment on attachment 108134 [details]
Patch for landing

Clearing flags on attachment: 108134

Committed r95619: <http://trac.webkit.org/changeset/95619>
Comment 8 WebKit Review Bot 2011-09-21 06:07:29 PDT
All reviewed patches have been landed.  Closing bug.