- Revision
- 183365
- Author
- [email protected]
- Date
- 2015-04-26 14:18:15 -0700 (Sun, 26 Apr 2015)
Log Message
REGRESSION (r173801): Use after free in WebCore::NotificationCenter::~NotificationCenter
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (183364 => 183365)
--- trunk/LayoutTests/ChangeLog 2015-04-26 20:38:20 UTC (rev 183364)
+++ trunk/LayoutTests/ChangeLog 2015-04-26 21:18:15 UTC (rev 183365)
@@ -1,3 +1,13 @@
+2015-04-26 Darin Adler <[email protected]>
+
+ 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 <[email protected]>
[JSC] Implement Math.clz32(), remove Number.clz()
Added: trunk/LayoutTests/fast/notifications/request-notification-permission-while-reloading-expected.txt (0 => 183365)
--- trunk/LayoutTests/fast/notifications/request-notification-permission-while-reloading-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/notifications/request-notification-permission-while-reloading-expected.txt 2015-04-26 21:18:15 UTC (rev 183365)
@@ -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.
Property changes on: trunk/LayoutTests/fast/notifications/request-notification-permission-while-reloading-expected.txt
___________________________________________________________________
Added: svn:eol-style
Added: trunk/LayoutTests/fast/notifications/request-notification-permission-while-reloading.html (0 => 183365)
--- trunk/LayoutTests/fast/notifications/request-notification-permission-while-reloading.html (rev 0)
+++ trunk/LayoutTests/fast/notifications/request-notification-permission-while-reloading.html 2015-04-26 21:18:15 UTC (rev 183365)
@@ -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>
Property changes on: trunk/LayoutTests/fast/notifications/request-notification-permission-while-reloading.html
___________________________________________________________________
Added: svn:mime-type
Added: svn:eol-style
Modified: trunk/Source/WebCore/ChangeLog (183364 => 183365)
--- trunk/Source/WebCore/ChangeLog 2015-04-26 20:38:20 UTC (rev 183364)
+++ trunk/Source/WebCore/ChangeLog 2015-04-26 21:18:15 UTC (rev 183365)
@@ -1,3 +1,35 @@
+2015-04-26 Darin Adler <[email protected]>
+
+ 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 <[email protected]>
Modernize animations code
Modified: trunk/Source/WebCore/Modules/notifications/NotificationCenter.cpp (183364 => 183365)
--- trunk/Source/WebCore/Modules/notifications/NotificationCenter.cpp 2015-04-26 20:38:20 UTC (rev 183364)
+++ trunk/Source/WebCore/Modules/notifications/NotificationCenter.cpp 2015-04-26 21:18:15 UTC (rev 183365)
@@ -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
@@ -35,30 +35,40 @@
#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 @@
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 @@
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,42 +140,19 @@
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)
+void NotificationCenter::timerFired()
{
- m_callbacks.remove(request);
+ ASSERT(m_client);
+ auto callbacks = WTF::move(m_callbacks);
+ for (auto& callback : callbacks)
+ callback();
+ deref(); // Balanced by the ref in NotificationCenter::requestPermission.
}
-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()
-{
- if (m_callback)
- m_callback->handleEvent();
- m_notificationCenter->requestTimedOut(this);
-}
-
} // namespace WebCore
#endif // ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
Modified: trunk/Source/WebCore/Modules/notifications/NotificationCenter.h (183364 => 183365)
--- trunk/Source/WebCore/Modules/notifications/NotificationCenter.h 2015-04-26 20:38:20 UTC (rev 183364)
+++ trunk/Source/WebCore/Modules/notifications/NotificationCenter.h 2015-04-26 21:18:15 UTC (rev 183365)
@@ -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
@@ -32,69 +32,46 @@
#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>);
+ void timerFired();
- RefPtr<NotificationCenter> m_notificationCenter;
- Timer m_timer;
- RefPtr<VoidCallback> m_callback;
- };
-
- void requestTimedOut(NotificationRequestCallback*);
-
NotificationClient* m_client;
- HashSet<RefPtr<NotificationRequestCallback>> m_callbacks;
+ Vector<std::function<void ()>> m_callbacks;
+ Timer m_timer;
};
} // namespace WebCore
