NotificationController::clientFrom() should return a reference

Source/modules/notifications/Notification.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 25 matching lines @@
 #include "core/dom/Document.h"
 #include "core/frame/UseCounter.h"
 #include "core/page/WindowFocusAllowedIndicator.h"
 #include "modules/notifications/NotificationClient.h"
 #include "modules/notifications/NotificationController.h"
 
 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;
     if (options.get("body", argument))
         notification->setBody(argument);
     if (options.get("tag", argument))
         notification->setTag(argument);
     if (options.get("lang", argument))
         notification->setLang(argument);
     if (options.get("dir", argument))
         notification->setDir(argument);
     if (options.get("icon", argument)) {
         KURL iconUrl = argument.isEmpty() ? KURL() : context->completeURL(argument);
         if (!iconUrl.isEmpty() && iconUrl.isValid())
             notification->setIconUrl(iconUrl);
     }
 
     notification->suspendIfNeeded();
     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")
     , m_state(Idle)
     , m_client(client)
     , m_asyncRunner(adoptPtr(new AsyncMethodRunner<Notification>(this, &Notification::show)))
 {
-    ASSERT(m_client);
     ScriptWrappable::init(this);
 
     m_asyncRunner->runAsync();
 }
 
 Notification::~Notification()
 {
 }
 
 void Notification::show()
 {
     ASSERT(m_state == Idle);
     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;
 }
 
 void Notification::close()
 {
     switch (m_state) {
     case Idle:
         break;
     case Showing:
-        m_client->close(this);
+        m_client.close(this);
         break;
     case Closed:
         break;
     }
 }
 
 void Notification::dispatchShowEvent()
 {
     dispatchEvent(Event::create(EventTypeNames::show));
 }
@@ skipping 39 matching lines @@
 
     ASSERT_NOT_REACHED();
     return deniedPermission;
 }
 
 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)
 {
     ASSERT(m_state != Closed);
 
     return EventTarget::dispatchEvent(event);
 }
 
 const AtomicString& Notification::interfaceName() const
 {
     return EventTargetNames::Notification;
 }
 
 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;
 }
 
 bool Notification::hasPendingActivity() const
 {
     return m_state == Showing || (m_asyncRunner && m_asyncRunner->isActive());
 }
 
 } // namespace WebCore