NotificationController::clientFrom() should return a reference

Source/modules/notifications/Notification.cpp

Index: Source/modules/notifications/Notification.cpp
diff --git a/Source/modules/notifications/Notification.cpp b/Source/modules/notifications/Notification.cpp
index 5114d8a625d39fef2f6226b9b7ae49de7b2052e3..e5bcc1cfbcd5e05bfe9462b262ae511944500d4d 100644
--- a/Source/modules/notifications/Notification.cpp
+++ b/Source/modules/notifications/Notification.cpp
@@ -43,7 +43,7 @@ namespace WebCore {
 
 Notification* Notification::create(ExecutionContext* context, const String& title, const Dictionary& options)
 {
-    NotificationClient* client = NotificationController::clientFrom(toDocument(context)->frame());
+    NotificationClient& client = NotificationController::clientFrom(toDocument(context)->frame());
     Notification* notification = adoptRefCountedGarbageCollected(new Notification(title, context, client));
 
     String argument;
@@ -65,7 +65,7 @@ Notification* Notification::create(ExecutionContext* context, const String& titl
     return notification;
 }
 
-Notification::Notification(const String& title, ExecutionContext* context, NotificationClient* client)
+Notification::Notification(const String& title, ExecutionContext* context, NotificationClient& client)
     : ActiveDOMObject(context)
     , m_title(title)
     , m_dir("auto")
@@ -73,7 +73,6 @@ Notification::Notification(const String& title, ExecutionContext* context, Notif
     , m_client(client)
     , m_asyncRunner(adoptPtr(new AsyncMethodRunner<Notification>(this, &Notification::show)))
 {
-    ASSERT(m_client);
     ScriptWrappable::init(this);
 
     m_asyncRunner->runAsync();
@@ -89,12 +88,12 @@ void Notification::show()
     if (!toDocument(executionContext())->page())
         return;
 
-    if (NotificationController::from(toDocument(executionContext())->frame())->client()->checkPermission(executionContext()) != NotificationClient::PermissionAllowed) {
+    if (NotificationController::from(toDocument(executionContext())->frame())->client().checkPermission(executionContext()) != NotificationClient::PermissionAllowed) {

[Peter Beverloo, 2014/05/08 12:59:34]

You can use m_client.checkPermission(executionContext()) here.

[gyuyoung-inactive, 2014/05/08 15:50:27]

Done.

         dispatchErrorEvent();
         return;
     }
 
-    if (m_client->show(this))
+    if (m_client.show(this))
         m_state = Showing;
 }
 
@@ -104,7 +103,7 @@ void Notification::close()
     case Idle:
         break;
     case Showing:
-        m_client->close(this);
+        m_client.close(this);
         break;
     case Closed:
         break;
@@ -164,13 +163,13 @@ const String& Notification::permission(ExecutionContext* context)
     ASSERT(toDocument(context)->page());
 
     UseCounter::count(context, UseCounter::NotificationPermission);
-    return permissionString(NotificationController::from(toDocument(context)->frame())->client()->checkPermission(context));
+    return permissionString(NotificationController::from(toDocument(context)->frame())->client().checkPermission(context));

[Peter Beverloo, 2014/05/08 12:59:34]

NotificationController::clientFrom()?

[gyuyoung-inactive, 2014/05/08 15:50:27]

Done.

 }
 
 void Notification::requestPermission(ExecutionContext* context, PassOwnPtr<NotificationPermissionCallback> callback)
 {
     ASSERT(toDocument(context)->page());
-    NotificationController::from(toDocument(context)->frame())->client()->requestPermission(context, callback);
+    NotificationController::from(toDocument(context)->frame())->client().requestPermission(context, callback);

[Peter Beverloo, 2014/05/08 12:59:34]

NotificationController::clientFrom()?

[gyuyoung-inactive, 2014/05/08 15:50:27]

Done.

 }
 
 bool Notification::dispatchEvent(PassRefPtrWillBeRawPtr<Event> event)
@@ -187,13 +186,11 @@ const AtomicString& Notification::interfaceName() const
 
 void Notification::stop()
 {
-    if (m_client)
-        m_client->notificationObjectDestroyed(this);
+    m_client.notificationObjectDestroyed(this);
 
     if (m_asyncRunner)
         m_asyncRunner->stop();
 
-    m_client = 0;

[haraken, 2014/05/08 05:12:04]

I don't understand why you can remove this. I agree that m_client should not be
0 when the Notification object is created, but m_client should become 0 when the
Notification stops.

[Peter Beverloo, 2014/05/08 12:59:34]

Are you worried about the case where LocalFrame gets destroyed, thus
NotificationController gets destroyed, freeing the NotificationClient, which
would leave Notification (for a very brief time) with a stale reference to freed
memory?

The class carefully guards against this by checking |m_state| where it matters,
so part of the risk is mitigated. However, we shouldn't refer to such memory in
the first place, which means Notification::m_client can't be a reference.

[gyuyoung-inactive, 2014/05/08 15:50:27]

Though I'm not 100% sure, it looks you guys worry about the case m_client can't
be a reference because of some situations(to become 0 when the Notificaiton stop
and so on). So, I continue to keep Notification::m_client as a pointer.
Notification just refers to a pointer of NotificationClient in
NotificationController. If I'm still wrong, please let me know.

[gyuyoung-inactive, 2014/05/09 02:28:49]

Peter and Kentaro, any comment ?

     m_state = Closed;
 }