Summary: | SnowLeopard crashes due to thread-unsafe EventListenerIterator ASSERTs | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Geoffrey Garen <ggaren> | ||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Critical | CC: | darin, ddkilzer, dglazkov, gustavo, kling, sam, tsepez, webkit.review.bot, xan.lopez | ||||||
Priority: | P1 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Geoffrey Garen
2011-12-11 17:19:40 PST
So you're saying that EventListenerIterator is used on other threads while WebCore chugs along merrily on the main thread. Doesn't this mean that we have a race condition where the EventListenerMap could be jumping from single-type to multi-type mode while an iterator is active, leaving the iterator pointing to deleted data? If so, I'm not entirely sure what the correct solution is. Perhaps generating a list of RefPtr<EventListener> when the visitChildren() pass starts and iterating over that instead? Downstreamed as http://code.google.com/p/chromium/issues/detail?id=107620 Geoff, is there a radar for this issue? > So you're saying that EventListenerIterator is used on other threads while WebCore chugs along merrily on the main thread.
He is not saying that. JavaScript marks using multiple threads. The main thread is also doing marking. No other WebCore code is running during that marking process. So multiple threads can be reading the event listeners, but there is a guarantee that no thread is writing to it at the same time.
All we need to resolve the problem with the new debugging assertions is to add a mutex around the debug machinery we recently added.
There may be some sort of subtle thread-safety issue, but I don’t think so.
Created attachment 120328 [details]
Patch
Guard EventListenerMap::m_activeIteratorCount with a mutex.
Attachment 120328 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit + 4e5cbb7...83cc6dc master -> origin/master (forced update) M Tools/TestWebKitAPI/TestsController.cpp M Tools/Scripts/run-api-tests M Tools/Scripts/webkitpy/test/main.py M Tools/Scripts/webkitpy/thirdparty/__init__.py M Tools/ChangeLog r103547 = 10bde81bae001a4da66ddaa326e03b261872898e (refs/remotes/origin/master) M LayoutTests/platform/gtk/Skipped M LayoutTests/platform/mac/accessibility/aria-describedby-on-input-expected.txt M LayoutTests/accessibility/aria-describedby-on-input.html M LayoutTests/ChangeLog M Source/WebCore/ChangeLog M Source/WebCore/accessibility/AccessibilityRenderObject.cpp r103548 = 0639b9de03475c2b49464647c607d0249c7445d1 (refs/remotes/origin/master) First, rewinding head to replay your work on top of it... Applying: run-api-tests: dumpAllTests() should not use global variables Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Failed to merge in the changes. Patch failed at 0001 run-api-tests: dumpAllTests() should not use global variables When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 158. If any of these errors are false positives, please file a bug against check-webkit-style. Comment on attachment 120328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120328&action=review > Source/WebCore/dom/EventListenerMap.h:85 > +#ifdef NDEBUG > + void assertNoActiveIterators() { } > +#else > + void assertNoActiveIterators(); Sometimes I like to put the declaration in unconditionally and put the inline empty implementation outside the class definition. > Source/WebCore/dom/EventListenerMap.h:87 > + Mutex m_activeIteratorCountMutex; Since this is for assertion purposes only, we could consider just sharing one global mutex rather than having a separate one for each map. That could keep down the number of #if statements in the header. (In reply to comment #7) > (From update of attachment 120328 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120328&action=review > > > Source/WebCore/dom/EventListenerMap.h:85 > > +#ifdef NDEBUG > > + void assertNoActiveIterators() { } > > +#else > > + void assertNoActiveIterators(); > > Sometimes I like to put the declaration in unconditionally and put the inline empty implementation outside the class definition. > > > Source/WebCore/dom/EventListenerMap.h:87 > > + Mutex m_activeIteratorCountMutex; > > Since this is for assertion purposes only, we could consider just sharing one global mutex rather than having a separate one for each map. That could keep down the number of #if statements in the header. Great suggestions, will revise! Created attachment 120333 [details]
Patch v2
Attachment 120333 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9
Updating OpenSource
Index mismatch: e2cbe978fd74a95efeede79908d082d70edf7254 != 61426a582f9075041358d45255f438344a0219d4
rereading 3ff8cc11dd87e8ac3a4acaee763d8549687e5da1
M Tools/TestWebKitAPI/TestsController.cpp
M Tools/ChangeLog
103546 = 4e5cbb711dbbaa35b7a064791c6a6e3e2556787c already exists! Why are we refetching it?
at /usr/lib/git-core/git-svn line 5210
Died at Tools/Scripts/update-webkit line 158.
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 120333 [details] Patch v2 Attachment 120333 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11000632 Comment on attachment 120333 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=120333&action=review > Source/WebCore/dom/EventListenerMap.h:104 > +void EventListenerMap::assertNoActiveIterators() { } You forgot the inline keyword here. That’s why the EFL builds are failing. Comment on attachment 120333 [details] Patch v2 Attachment 120333 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11000636 Comment on attachment 120333 [details] Patch v2 Attachment 120333 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10998563 Committed r103556: <http://trac.webkit.org/changeset/103556> |