Summary: | Crash in WebCore::NotificationCenter::stop() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, ddkilzer, kling, koivisto | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Chris Dumez
2015-03-07 18:23:41 PST
Created attachment 248171 [details]
Patch
Comment on attachment 248171 [details]
Patch
r=me
Comment on attachment 248171 [details] Patch Clearing flags on attachment: 248171 Committed r181219: <http://trac.webkit.org/changeset/181219> All reviewed patches have been landed. Closing bug. 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? 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. (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. 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. Reopening to attach new patch. Created attachment 248190 [details]
Patch
Created attachment 248191 [details]
Patch
(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. (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(). Comment on attachment 248191 [details] Patch Clearing flags on attachment: 248191 Committed r181256: <http://trac.webkit.org/changeset/181256> All reviewed patches have been landed. Closing bug. |