Summary: | Protect against misuse of EventListenerIterator. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||
Component: | UI Events | Assignee: | Andreas Kling <kling> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Andreas Kling
2011-09-19 08:29:07 PDT
Created attachment 107861 [details]
Proposed patch
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 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. Created attachment 107864 [details]
Proposed patch v2
Same patch, but simply counting the number of active iterators. Sorry about the noise.
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. Created attachment 108134 [details]
Patch for landing
Comment on attachment 108134 [details] Patch for landing Clearing flags on attachment: 108134 Committed r95619: <http://trac.webkit.org/changeset/95619> All reviewed patches have been landed. Closing bug. |