Bug 74260 – SnowLeopard crashes due to thread-unsafe EventListenerIterator ASSERTs

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Geoffrey Garen]

I believe that at least some of the SnowLeopard debug tester crashes are caused by lack of thread-safety in ASSERT-related code in EventListenerIterator. E.g.: http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(WebKit2%20Tests)/r102544%20(3035)/editing/deleting/delete-br-013-stderr.txt.

This bug was introduced in http://trac.webkit.org/changeset/95372 ("Reduce EventTarget memory usage by deferring hash map allocation…").

The right fix is to put a mutex around modifications to m_activeIteratorCount. WTF::HashTable has a similar problem and solution, so you can copy from there.

EventListenerIterator is used on multiple threads during the GC mark phase, at least.

[Andreas Kling]

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?

[Thomas Sepez]

Downstreamed as http://code.google.com/p/chromium/issues/detail?id=107620

[David Kilzer (:ddkilzer)]

Geoff, is there a radar for this issue?

[Darin Adler]

> 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.

[Andreas Kling]

Created attachment 120328 [details]
Patch

Guard EventListenerMap::m_activeIteratorCount with a mutex.

[WebKit Review Bot]

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.

[Darin Adler]

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.

[Andreas Kling]

(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!

[Andreas Kling]

Created attachment 120333 [details]
Patch v2

[WebKit Review Bot]

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.

[Gyuyoung Kim]

Comment on attachment 120333 [details]
Patch v2

Attachment 120333 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11000632

[Darin Adler]

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.

[WebKit Review Bot]

Comment on attachment 120333 [details]
Patch v2

Attachment 120333 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11000636

[Gustavo Noronha (kov)]

Comment on attachment 120333 [details]
Patch v2

Attachment 120333 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10998563

[Andreas Kling]

Committed r103556: <http://trac.webkit.org/changeset/103556>