REGRESSION (r173801): Use after free in WebCore::NotificationCenter::~NotificationCenter
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 26 Apr 2015 21:18:15 +0000 (21:18 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 26 Apr 2015 21:18:15 +0000 (21:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=137163

Reviewed by Andy Estes.

Source/WebCore:

Test: fast/notifications/request-notification-permission-while-reloading.html

The test doesn't crash under WebKit2, but that's still OK for our purposes.

* Modules/notifications/NotificationCenter.cpp:
(WebCore::NotificationCenter::NotificationCenter): Initialize m_timer.
(WebCore::NotificationCenter::createNotification): Moved here from the header.
(WebCore::NotificationCenter::requestPermission): Start the timer and ref the notification
center when we need to defer a callback. Also use a lambda for the callback and changed
the argument to match what the bindings actually pass. Before we said PassRefPtr, but the
bindings were not transferring ownership of the VoidCallback. The new type is a little
strange but it's consistent with how the bindings work right now.
(WebCore::NotificationCenter::timerFired): Added. Calls all the callbacks, then does a deref
to match the ref we did above.
(WebCore::NotificationCenter::requestTimedOut): Deleted.
(WebCore::NotificationCenter::NotificationRequestCallback::createAndStartTimer): Deleted.
(WebCore::NotificationCenter::NotificationRequestCallback::NotificationRequestCallback): Deleted.
(WebCore::NotificationCenter::NotificationRequestCallback::startTimer): Deleted.
(WebCore::NotificationCenter::NotificationRequestCallback::timerFired): Deleted.

* Modules/notifications/NotificationCenter.h: Reorganized the header to be a bit more tidy.
Changed the argument type for requestPermission to match the reality of what's passed by the
bindings. Removed NotificationRequestCallback and replaced the m_callbacks HashSet with a
vector of std::function.

LayoutTests:

* fast/notifications/request-notification-permission-while-reloading-expected.txt: Added.
* fast/notifications/request-notification-permission-while-reloading.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@183365 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/notifications/request-notification-permission-while-reloading-expected.txt [new file with mode: 0644]
LayoutTests/fast/notifications/request-notification-permission-while-reloading.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/notifications/NotificationCenter.cpp
Source/WebCore/Modules/notifications/NotificationCenter.h

index 5043fffbcc888f2cd89346e2c5e9eccc83e472db..655effb09048fcd160525a88c48c438f739b1846 100644 (file)
@@ -1,3 +1,13 @@
+2015-04-26  Darin Adler  <darin@apple.com>
+
+        REGRESSION (r173801): Use after free in WebCore::NotificationCenter::~NotificationCenter
+        https://bugs.webkit.org/show_bug.cgi?id=137163
+
+        Reviewed by Andy Estes.
+
+        * fast/notifications/request-notification-permission-while-reloading-expected.txt: Added.
+        * fast/notifications/request-notification-permission-while-reloading.html: Added.
+
 2015-04-26  Benjamin Poulain  <benjamin@webkit.org>
 
         [JSC] Implement Math.clz32(), remove Number.clz()
diff --git a/LayoutTests/fast/notifications/request-notification-permission-while-reloading-expected.txt b/LayoutTests/fast/notifications/request-notification-permission-while-reloading-expected.txt
new file mode 100644 (file)
index 0000000..2df3aa3
--- /dev/null
@@ -0,0 +1,3 @@
+This tests that notification permission requests during page reload do not cause a crash.
+
+PASS: Test passed if we saw no crash.
diff --git a/LayoutTests/fast/notifications/request-notification-permission-while-reloading.html b/LayoutTests/fast/notifications/request-notification-permission-while-reloading.html
new file mode 100644 (file)
index 0000000..c93eb63
--- /dev/null
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<body>
+<p>This tests that notification permission requests during page reload do not cause a crash.</p>
+<p id="message">FAIL: Test did not run to completion yet.</p>
+<script>
+
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+
+var tries = location.hash.substring(1) >>> 0;
+if (tries < 10) {
+    location.hash = "#" + (tries + 1);
+    location.reload();
+
+    setTimeout(function() {
+        webkitNotifications.requestPermission();
+    }, 0);
+} else {
+    document.getElementById("message").textContent = "PASS: Test passed if we saw no crash.";
+    if (window.testRunner)
+        testRunner.notifyDone();
+}
+
+</script>
+</body>
index 09ec66d716e358ad9732c3163477bd075fc9ae97..91f7cd202c42a82f82dda6c34f27b0586bb3865c 100644 (file)
@@ -1,3 +1,35 @@
+2015-04-26  Darin Adler  <darin@apple.com>
+
+        REGRESSION (r173801): Use after free in WebCore::NotificationCenter::~NotificationCenter
+        https://bugs.webkit.org/show_bug.cgi?id=137163
+
+        Reviewed by Andy Estes.
+
+        Test: fast/notifications/request-notification-permission-while-reloading.html
+
+        The test doesn't crash under WebKit2, but that's still OK for our purposes.
+
+        * Modules/notifications/NotificationCenter.cpp:
+        (WebCore::NotificationCenter::NotificationCenter): Initialize m_timer.
+        (WebCore::NotificationCenter::createNotification): Moved here from the header.
+        (WebCore::NotificationCenter::requestPermission): Start the timer and ref the notification
+        center when we need to defer a callback. Also use a lambda for the callback and changed
+        the argument to match what the bindings actually pass. Before we said PassRefPtr, but the
+        bindings were not transferring ownership of the VoidCallback. The new type is a little
+        strange but it's consistent with how the bindings work right now.
+        (WebCore::NotificationCenter::timerFired): Added. Calls all the callbacks, then does a deref
+        to match the ref we did above.
+        (WebCore::NotificationCenter::requestTimedOut): Deleted.
+        (WebCore::NotificationCenter::NotificationRequestCallback::createAndStartTimer): Deleted.
+        (WebCore::NotificationCenter::NotificationRequestCallback::NotificationRequestCallback): Deleted.
+        (WebCore::NotificationCenter::NotificationRequestCallback::startTimer): Deleted.
+        (WebCore::NotificationCenter::NotificationRequestCallback::timerFired): Deleted.
+
+        * Modules/notifications/NotificationCenter.h: Reorganized the header to be a bit more tidy.
+        Changed the argument type for requestPermission to match the reality of what's passed by the
+        bindings. Removed NotificationRequestCallback and replaced the m_callbacks HashSet with a
+        vector of std::function.
+
 2015-04-26  Simon Fraser  <simon.fraser@apple.com>
 
         Modernize animations code
index 6eb81b9c478011a561c94ccdf724dcf326468472..18b0093afb4323da5b4351676dfa94d26faeefd7 100644 (file)
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
- * Copyright (C) 2012 Apple Inc. All rights reserved.
+ * Copyright (C) 2012, 2015 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
 
 #include "NotificationCenter.h"
 
-#include "Document.h"
-#include "NotificationClient.h"
+#include "Notification.h"
+#include "ScriptExecutionContext.h"
 #include "SecurityOrigin.h"
-#include "WorkerGlobalScope.h"
 
 namespace WebCore {
 
 Ref<NotificationCenter> NotificationCenter::create(ScriptExecutionContext* context, NotificationClient* client)
 {
     auto notificationCenter = adoptRef(*new NotificationCenter(context, client));
-    notificationCenter.get().suspendIfNeeded();
+    notificationCenter->suspendIfNeeded();
     return notificationCenter;
 }
 
 NotificationCenter::NotificationCenter(ScriptExecutionContext* context, NotificationClient* client)
     : ActiveDOMObject(context)
     , m_client(client)
+    , m_timer([this]() { timerFired(); })
 {
 }
 
 #if ENABLE(LEGACY_NOTIFICATIONS)
+
+RefPtr<Notification> NotificationCenter::createNotification(const String& iconURI, const String& title, const String& body, ExceptionCode& ec)
+{
+    if (!m_client) {
+        ec = INVALID_STATE_ERR;
+        return nullptr;
+    }
+    return Notification::create(title, body, iconURI, scriptExecutionContext(), ec, this);
+}
+
 int NotificationCenter::checkPermission()
 {
-    if (!client() || !scriptExecutionContext())
+    if (!m_client || !scriptExecutionContext())
         return NotificationClient::PermissionDenied;
 
     switch (scriptExecutionContext()->securityOrigin()->canShowNotifications()) {
@@ -73,27 +83,33 @@ int NotificationCenter::checkPermission()
     ASSERT_NOT_REACHED();
     return m_client->checkPermission(scriptExecutionContext());
 }
-#endif
 
-#if ENABLE(LEGACY_NOTIFICATIONS)
-void NotificationCenter::requestPermission(PassRefPtr<VoidCallback> callback)
+void NotificationCenter::requestPermission(const RefPtr<VoidCallback>& callback)
 {
-    if (!client() || !scriptExecutionContext())
+    if (!m_client || !scriptExecutionContext())
         return;
 
     switch (scriptExecutionContext()->securityOrigin()->canShowNotifications()) {
     case SecurityOrigin::AlwaysAllow:
-    case SecurityOrigin::AlwaysDeny: {
-        m_callbacks.add(NotificationRequestCallback::createAndStartTimer(this, callback));
+    case SecurityOrigin::AlwaysDeny:
+        if (m_callbacks.isEmpty()) {
+            ref(); // Balanced by the derefs in NotificationCenter::stop and NotificationCenter::timerFired.
+            m_timer.startOneShot(0);
+        }
+        m_callbacks.append([callback]() {
+            if (callback)
+                callback->handleEvent();
+        });
         return;
-    }
     case SecurityOrigin::Ask:
-        return m_client->requestPermission(scriptExecutionContext(), callback);
+        m_client->requestPermission(scriptExecutionContext(), callback.get());
+        return;
     }
 
     ASSERT_NOT_REACHED();
-    m_client->requestPermission(scriptExecutionContext(), callback);
+    m_client->requestPermission(scriptExecutionContext(), callback.get());
 }
+
 #endif
 
 void NotificationCenter::stop()
@@ -101,15 +117,19 @@ void NotificationCenter::stop()
     if (!m_client)
         return;
 
-    // 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;
+    // Clear m_client immediately to guarantee reentrant calls to NotificationCenter do nothing.
+    // Also protect |this| so it's not indirectly destroyed under us by work done by the client.
+    auto& client = *std::exchange(m_client, nullptr);
+    Ref<NotificationCenter> protector(*this);
+
+    if (!m_callbacks.isEmpty())
+        deref(); // Balanced by the ref in NotificationCenter::requestPermission.
+
+    m_timer.stop();
+    m_callbacks.clear();
 
     client.cancelRequestsForPermission(scriptExecutionContext());
     client.clearNotifications(scriptExecutionContext());
-    // Do not attempt the access |this|, the NotificationCenter may be destroyed at this point.
 }
 
 const char* NotificationCenter::activeDOMObjectName() const
@@ -120,40 +140,17 @@ const char* NotificationCenter::activeDOMObjectName() const
 bool NotificationCenter::canSuspendForPageCache() const
 {
     // We don't need to worry about Notifications because those are ActiveDOMObject too.
-    // The NotificationCenter can safely be suspended if there are no pending permission
-    // requests.
+    // The NotificationCenter can safely be suspended if there are no pending permission requests.
     return m_callbacks.isEmpty() && (!m_client || !m_client->hasPendingPermissionRequests(scriptExecutionContext()));
 }
 
-void NotificationCenter::requestTimedOut(NotificationCenter::NotificationRequestCallback* request)
-{
-    m_callbacks.remove(request);
-}
-
-PassRefPtr<NotificationCenter::NotificationRequestCallback> NotificationCenter::NotificationRequestCallback::createAndStartTimer(NotificationCenter* center, PassRefPtr<VoidCallback> callback)
-{
-    RefPtr<NotificationCenter::NotificationRequestCallback> requestCallback = adoptRef(new NotificationCenter::NotificationRequestCallback(center, callback));
-    requestCallback->startTimer();
-    return requestCallback.release();
-}
-
-NotificationCenter::NotificationRequestCallback::NotificationRequestCallback(NotificationCenter* center, PassRefPtr<VoidCallback> callback)
-    : m_notificationCenter(center)
-    , m_timer(*this, &NotificationCenter::NotificationRequestCallback::timerFired)
-    , m_callback(callback)
-{
-}
-
-void NotificationCenter::NotificationRequestCallback::startTimer()
-{
-    m_timer.startOneShot(0);
-}
-
-void NotificationCenter::NotificationRequestCallback::timerFired()
+void NotificationCenter::timerFired()
 {
-    if (m_callback)
-        m_callback->handleEvent();
-    m_notificationCenter->requestTimedOut(this);
+    ASSERT(m_client);
+    auto callbacks = WTF::move(m_callbacks);
+    for (auto& callback : callbacks)
+        callback();
+    deref(); // Balanced by the ref in NotificationCenter::requestPermission.
 }
 
 } // namespace WebCore
index cd6210b462693fe1c7320faed1a404966efad69b..3581f451a1ac03ab5183cd243c014267a7bd300e 100644 (file)
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
- * Copyright (C) 2011, 2012 Apple Inc. All rights reserved.
+ * Copyright (C) 2011, 2012, 2015 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
 #ifndef NotificationCenter_h
 #define NotificationCenter_h
 
+#include "ActiveDOMObject.h"
 #include "ExceptionCode.h"
-#include "Notification.h"
-#include "ScriptExecutionContext.h"
 #include "Timer.h"
-#include "VoidCallback.h"
-#include <wtf/PassRefPtr.h>
 #include <wtf/RefCounted.h>
-#include <wtf/RefPtr.h>
 
 #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
 
 namespace WebCore {
 
+class Notification;
 class NotificationClient;
 class VoidCallback;
 
-class NotificationCenter : public RefCounted<NotificationCenter>, public ActiveDOMObject {
+class NotificationCenter : public RefCounted<NotificationCenter>, private ActiveDOMObject {
 public:
     static Ref<NotificationCenter> create(ScriptExecutionContext*, NotificationClient*);
 
 #if ENABLE(LEGACY_NOTIFICATIONS)
-    PassRefPtr<Notification> createNotification(const String& iconURI, const String& title, const String& body, ExceptionCode& ec)
-    {
-        if (!client()) {
-            ec = INVALID_STATE_ERR;
-            return 0;
-        }
-        return Notification::create(title, body, iconURI, scriptExecutionContext(), ec, this);
-    }
+    RefPtr<Notification> createNotification(const String& iconURI, const String& title, const String& body, ExceptionCode&);
+
+    int checkPermission();
+    void requestPermission(const RefPtr<VoidCallback>&);
 #endif
 
     NotificationClient* client() const { return m_client; }
 
-#if ENABLE(LEGACY_NOTIFICATIONS)
-    int checkPermission();
-    void requestPermission(PassRefPtr<VoidCallback> = 0);
-#endif
+    using ActiveDOMObject::hasPendingActivity;
 
 private:
     NotificationCenter(ScriptExecutionContext*, NotificationClient*);
 
-    // ActiveDOMObject API.
     void stop() override;
     const char* activeDOMObjectName() const override;
     bool canSuspendForPageCache() const override;
 
-    class NotificationRequestCallback : public RefCounted<NotificationRequestCallback> {
-    public:
-        static PassRefPtr<NotificationRequestCallback> createAndStartTimer(NotificationCenter*, PassRefPtr<VoidCallback>);
-        void startTimer();
-        void timerFired();
-    private:
-        NotificationRequestCallback(NotificationCenter*, PassRefPtr<VoidCallback>);
-
-        RefPtr<NotificationCenter> m_notificationCenter;
-        Timer m_timer;
-        RefPtr<VoidCallback> m_callback;
-    };
-
-    void requestTimedOut(NotificationRequestCallback*);
+    void timerFired();
 
     NotificationClient* m_client;
-    HashSet<RefPtr<NotificationRequestCallback>> m_callbacks;
+    Vector<std::function<void ()>> m_callbacks;
+    Timer m_timer;
 };
 
 } // namespace WebCore