Changeset 181256 in webkit

Timestamp:

Mar 8, 2015 7:55:30 PM (9 years ago)

Author:

Chris Dumez

Message:

Crash in WebCore::NotificationCenter::stop()
​https://bugs.webkit.org/show_bug.cgi?id=142444
<rdar://problem/20082520>

Reviewed by Darin Adler.

Rework the patch in r181219 so that we do not need a Ref<NotificationCenter> protector
in NotificationCenter::stop(). Instead, we put the client in a local variable and null
out m_client *before* calling NotificationClient::clearNotifications().

No new tests, already covered by:
http/tests/notifications/event-listener-crash.html

• Modules/notifications/NotificationCenter.cpp:

(WebCore::NotificationCenter::stop):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/notifications/NotificationCenter.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-03-08  Chris Dumez  <cdumez@apple.com>
+
+        Crash in WebCore::NotificationCenter::stop()
+        https://bugs.webkit.org/show_bug.cgi?id=142444
+        <rdar://problem/20082520>
+
+        Reviewed by Darin Adler.
+
+        Rework the patch in r181219 so that we do not need a Ref<NotificationCenter> protector
+        in NotificationCenter::stop(). Instead, we put the client in a local variable and null
+        out m_client *before* calling NotificationClient::clearNotifications().
+
+        No new tests, already covered by:
+        http/tests/notifications/event-listener-crash.html
+
+        * Modules/notifications/NotificationCenter.cpp:
+        (WebCore::NotificationCenter::stop):
+
 2015-03-08  Simon Fraser  <simon.fraser@apple.com>
 

trunk/Source/WebCore/Modules/notifications/NotificationCenter.cpp

@@ -102 +102 @@
         return;
 
-    // The call to clearNotifications() below will destroy the notifications. The notifications will
-    // unref the NotificationCenter when destroyed so we need to protect the NotificationCenter here
-    // in case no-one else holds a ref to the NotificationCenter at this point besides Notifications.
-    Ref<NotificationCenter> protect(*this);
-    m_client->cancelRequestsForPermission(scriptExecutionContext());
-    m_client->clearNotifications(scriptExecutionContext());
+    // Clear m_client now because the call to NotificationClient::clearNotifications() below potentially
+    // destroy the NotificationCenter. This is because the notifications will be destroyed and unref the
+    // NotificationCenter.
+    auto& client = *m_client;
     m_client = nullptr;
+
+    client.cancelRequestsForPermission(scriptExecutionContext());
+    client.clearNotifications(scriptExecutionContext());
+    // Do not attempt the access |this|, the NotificationCenter may be destroyed at this point.
 }