Bug 142444 – Crash in WebCore::NotificationCenter::stop()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

We sometimes crash in NotificationCenter::stop() with the following trace:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000001038a472a WebCore::NotificationCenter::stop() + 42
1   com.apple.WebCore             	0x0000000102e28bc7 WebCore::ScriptExecutionContext::stopActiveDOMObjects() + 359
2   com.apple.WebCore             	0x0000000102e2877e WebCore::Document::prepareForDestruction() + 366
3   com.apple.WebCore             	0x0000000103327afe WebCore::Frame::setView(WTF::RefPtr<WebCore::FrameView>&&) + 62
4   com.apple.WebKitLegacy        	0x0000000104808f80 WebFrameLoaderClient::transitionToCommittedForNewPage() + 432
5   com.apple.WebCore             	0x000000010333055c WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*) + 636
6   com.apple.WebCore             	0x0000000102dbd2ae WebCore::FrameLoader::commitProvisionalLoad() + 366
7   com.apple.WebCore             	0x0000000102dbcedf WebCore::DocumentLoader::finishedLoading(double) + 383
8   com.apple.WebCore             	0x0000000102dbcb25 WebCore::DocumentLoader::maybeLoadEmpty() + 709
9   com.apple.WebCore             	0x0000000102dbc20b WebCore::DocumentLoader::startLoadingMainResource() + 219

I can reproduce the crash by doing:
Tools/Scripts/run-webkit-tests -1 --debug --repeat-each=30 -g LayoutTests/http/tests/notifications/event-listener-crash.html

Radar: <rdar://problem/20082520>

[Chris Dumez]

Created attachment 248171 [details]
Patch

[Andreas Kling]

Comment on attachment 248171 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 248171 [details]
Patch

Clearing flags on attachment: 248171

Committed r181219: <http://trac.webkit.org/changeset/181219>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 248171 [details]
Patch

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

> Source/WebCore/Modules/notifications/NotificationCenter.cpp:109
>      m_client->clearNotifications(scriptExecutionContext());

What guarantees m_client has not become null at this point?

[Darin Adler]

Comment on attachment 248171 [details]
Patch

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

> Source/WebCore/Modules/notifications/NotificationCenter.cpp:110
>      m_client = nullptr;

Another way to fix this would be to put the client into a local variable and null out m_client *before* calling clearNotifications.

[Chris Dumez]

(In reply to comment #6)
> Comment on attachment 248171 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248171&action=review
> 
> > Source/WebCore/Modules/notifications/NotificationCenter.cpp:110
> >      m_client = nullptr;
> 
> Another way to fix this would be to put the client into a local variable and
> null out m_client *before* calling clearNotifications.

Good idea, I'll re-upload a patch.

BTW, I think we are leaking the client. provideNotification() is called like this:
WebCore::provideNotification(m_page.get(), new WebNotificationClient(this));

We then keep a NotificationClient& as a member of NotificationController but I don't see us destroying the NotificationClient anywhere. This is not specific to Notifications though (Geolocation is the same for e.g.) so this may be done on purpose.

[Chris Dumez]

Comment on attachment 248171 [details]
Patch

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

>> Source/WebCore/Modules/notifications/NotificationCenter.cpp:109
>>      m_client->clearNotifications(scriptExecutionContext());
> 
> What guarantees m_client has not become null at this point?

m_client is only set to null on the next line, nowhere else. The client pointer has to be valid too as we seem to leak the client.

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 248190 [details]
Patch

[Chris Dumez]

Created attachment 248191 [details]
Patch

[Darin Adler]

(In reply to comment #7)
> BTW, I think we are leaking the client. provideNotification() is called like
> this:
> WebCore::provideNotification(m_page.get(), new WebNotificationClient(this));
> 
> We then keep a NotificationClient& as a member of NotificationController but
> I don't see us destroying the NotificationClient anywhere. This is not
> specific to Notifications though (Geolocation is the same for e.g.) so this
> may be done on purpose.

The client is destroyed when notificationControllerDestroyed is called. So we will indeed leak it if we set m_client to null without calling notificationControllerDestroyed on that client.

[Chris Dumez]

(In reply to comment #12)
> (In reply to comment #7)
> > BTW, I think we are leaking the client. provideNotification() is called like
> > this:
> > WebCore::provideNotification(m_page.get(), new WebNotificationClient(this));
> > 
> > We then keep a NotificationClient& as a member of NotificationController but
> > I don't see us destroying the NotificationClient anywhere. This is not
> > specific to Notifications though (Geolocation is the same for e.g.) so this
> > may be done on purpose.
> 
> The client is destroyed when notificationControllerDestroyed is called. So
> we will indeed leak it if we set m_client to null without calling
> notificationControllerDestroyed on that client.

Oh, I missed that. We are not leaking then. ~NotificationController() does call notificationControllerDestroyed() on the client. m_client is never reset to null in the NotificationController, we only null out NotificationCenter::m_client in NotificationCenter::stop().

[WebKit Commit Bot]

Comment on attachment 248191 [details]
Patch

Clearing flags on attachment: 248191

Committed r181256: <http://trac.webkit.org/changeset/181256>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.