Push API: Refactor and fix unsubscribe API

chrome/browser/push_messaging/push_messaging_service_impl.cc

Index: chrome/browser/push_messaging/push_messaging_service_impl.cc
diff --git a/chrome/browser/push_messaging/push_messaging_service_impl.cc b/chrome/browser/push_messaging/push_messaging_service_impl.cc
index 1cc200aaa054c6c024e07709450e1b4831dab4c3..fbe657c04e8b434d1f0b076f65d9b8e5d6d5cd69 100644
--- a/chrome/browser/push_messaging/push_messaging_service_impl.cc
+++ b/chrome/browser/push_messaging/push_messaging_service_impl.cc
@@ -83,6 +83,11 @@ void RecordDeliveryStatus(content::PushDeliveryStatus status) {
                             content::PUSH_DELIVERY_STATUS_LAST + 1);
 }
 
+void RecordUnsubscribeReason(content::PushUnregistrationReason reason) {
+  UMA_HISTOGRAM_ENUMERATION("PushMessaging.UnregistrationReason", reason,
+                            content::PUSH_UNREGISTRATION_REASON_LAST + 1);
+}
+
 blink::WebPushPermissionStatus ToPushPermission(
     blink::mojom::PermissionStatus permission_status) {
   switch (permission_status) {
@@ -98,8 +103,49 @@ blink::WebPushPermissionStatus ToPushPermission(
   }
 }
 
+content::PushUnregistrationStatus ToUnregisterStatus(
+    InstanceID::Result result) {
+  switch (result) {
+    case InstanceID::SUCCESS:
+      return content::PUSH_UNREGISTRATION_STATUS_SUCCESS_UNREGISTERED;
+    case InstanceID::INVALID_PARAMETER:
+    case InstanceID::DISABLED:
+    case InstanceID::SERVER_ERROR:
+    case InstanceID::UNKNOWN_ERROR:
+      return content::PUSH_UNREGISTRATION_STATUS_PENDING_SERVICE_ERROR;
+    case InstanceID::ASYNC_OPERATION_PENDING:
+    case InstanceID::NETWORK_ERROR:
+      return content::PUSH_UNREGISTRATION_STATUS_PENDING_NETWORK_ERROR;
+  }

[Peter Beverloo, 2016/09/30 14:26:06]

Most compilers will want you to return a default value after the switch(), in
case |result| for whatever reason contains an invalid value anyway.

[johnme, 2016/09/30 17:02:09]

No, default is evil! It prevents compilers from warning when you omit enum
values. But yeah, I've added NOTREACHED and a fallback return (CHECK(false)
would be neater, but probably bloats code size more? We need a better abort
macro for such cases...).

+}
+
+content::PushUnregistrationStatus ToUnregisterStatus(
+    gcm::GCMClient::Result gcm_result) {
+  switch (gcm_result) {
+    case gcm::GCMClient::SUCCESS:
+      return content::PUSH_UNREGISTRATION_STATUS_SUCCESS_UNREGISTERED;
+    case gcm::GCMClient::INVALID_PARAMETER:
+    case gcm::GCMClient::GCM_DISABLED:
+    case gcm::GCMClient::SERVER_ERROR:
+    case gcm::GCMClient::UNKNOWN_ERROR:
+      return content::PUSH_UNREGISTRATION_STATUS_PENDING_SERVICE_ERROR;
+    case gcm::GCMClient::ASYNC_OPERATION_PENDING:
+    case gcm::GCMClient::NETWORK_ERROR:
+    case gcm::GCMClient::TTL_EXCEEDED:
+      return content::PUSH_UNREGISTRATION_STATUS_PENDING_NETWORK_ERROR;
+  }
+}
+
+void GCMCallbackToUnregisterCallback(
+    const content::PushMessagingService::UnregisterCallback& callback,
+    gcm::GCMClient::Result result) {
+  DCHECK(!callback.is_null());
+  callback.Run(ToUnregisterStatus(result));
+}
+
 void UnregisterCallbackToClosure(const base::Closure& closure,
                                  content::PushUnregistrationStatus status) {
+  DCHECK(!closure.is_null());
   closure.Run();
 }
 
@@ -236,8 +282,9 @@ void PushMessagingServiceImpl::OnMessage(const std::string& app_id,
       PushMessagingAppIdentifier::FindByAppId(profile_, app_id);
   // Drop message and unregister if app_id was unknown (maybe recently deleted).
   if (app_identifier.is_null()) {
-    DeliverMessageCallback(app_id, GURL::EmptyGURL(), -1, message,
-                           message_handled_closure,
+    DeliverMessageCallback(app_id, GURL::EmptyGURL(),
+                           -1 /* kInvalidServiceWorkerRegistrationId */,
+                           message, message_handled_closure,
                            content::PUSH_DELIVERY_STATUS_UNKNOWN_APP_ID);
     return;
   }
@@ -287,6 +334,8 @@ void PushMessagingServiceImpl::DeliverMessageCallback(
     content::PushDeliveryStatus status) {
   DCHECK_GE(in_flight_message_deliveries_.count(app_id), 1u);
 
+  RecordDeliveryStatus(status);
+
   base::Closure completion_closure =
       base::Bind(&PushMessagingServiceImpl::DidHandleMessage,
                  weak_factory_.GetWeakPtr(), app_id, message_handled_closure);
@@ -294,6 +343,13 @@ void PushMessagingServiceImpl::DeliverMessageCallback(
   // unless it is explicitly passed to another function.
   base::ScopedClosureRunner completion_closure_runner(completion_closure);
 
+  // Dummy reason meaning this function should not automatically unsubscribe.
+  const content::PushUnregistrationReason DO_NOT_UNSUBSCRIBE =
+      content::PUSH_UNREGISTRATION_REASON_JAVASCRIPT_API;

[Peter Beverloo, 2016/09/30 14:26:06]

What about defining a PUSH_UNREGISTRATION_REASON_UNKNOWN (or NONE) enum instead?
Minor overhead in the enum/histogram, but it makes this code *a lot* nicer.

[johnme, 2016/09/30 17:02:10]

Done.

+
+  // Whether and why automatic unsubscription is necessary.
+  content::PushUnregistrationReason unsubscribe_reason = DO_NOT_UNSUBSCRIBE;
+
   // TODO(mvanouwerkerk): Show a warning in the developer console of the
   // Service Worker corresponding to app_id (and/or on an internals page).
   // See https://crbug.com/508516 for options.
@@ -318,17 +374,35 @@ void PushMessagingServiceImpl::DeliverMessageCallback(
 #endif
       break;
     case content::PUSH_DELIVERY_STATUS_SERVICE_WORKER_ERROR:
+      // Do nothing, and hope the error is transient.
       break;
     case content::PUSH_DELIVERY_STATUS_UNKNOWN_APP_ID:
+      unsubscribe_reason =
+          content::PUSH_UNREGISTRATION_REASON_DELIVERY_UNKNOWN_APP_ID;
+      break;
     case content::PUSH_DELIVERY_STATUS_PERMISSION_DENIED:
+      unsubscribe_reason =
+          content::PUSH_UNREGISTRATION_REASON_DELIVERY_PERMISSION_DENIED;
+      break;
     case content::PUSH_DELIVERY_STATUS_NO_SERVICE_WORKER:
-      Unsubscribe(app_id, message.sender_id,
-                  base::Bind(&UnregisterCallbackToClosure,
-                             completion_closure_runner.Release()));
+      unsubscribe_reason =
+          content::PUSH_UNREGISTRATION_REASON_DELIVERY_NO_SERVICE_WORKER;
       break;
   }
 
-  RecordDeliveryStatus(status);
+  if (unsubscribe_reason != DO_NOT_UNSUBSCRIBE) {
+    PushMessagingAppIdentifier app_identifier =
+        PushMessagingAppIdentifier::FindByAppId(profile_, app_id);
+    UnsubscribeInternal(
+        unsubscribe_reason,
+        app_identifier.is_null() ? GURL::EmptyGURL() : app_identifier.origin(),
+        app_identifier.is_null()
+            ? -1 /* kInvalidServiceWorkerRegistrationId */
+            : app_identifier.service_worker_registration_id(),
+        app_id, message.sender_id,
+        base::Bind(&UnregisterCallbackToClosure,
+                   completion_closure_runner.Release()));
+  }
 }
 
 void PushMessagingServiceImpl::DidHandleMessage(
@@ -614,19 +688,54 @@ void PushMessagingServiceImpl::Unsubscribe(
   PushMessagingAppIdentifier app_identifier =
       PushMessagingAppIdentifier::FindByServiceWorker(
           profile_, requesting_origin, service_worker_registration_id);
-  if (app_identifier.is_null()) {
-    callback.Run(
-        content::PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);
+
+  UnsubscribeInternal(
+      content::PUSH_UNREGISTRATION_REASON_JAVASCRIPT_API, requesting_origin,
+      service_worker_registration_id,
+      app_identifier.is_null() ? std::string() : app_identifier.app_id(),
+      sender_id, callback);
+}
+
+void PushMessagingServiceImpl::UnsubscribeInternal(
+    content::PushUnregistrationReason reason,
+    const GURL& origin,
+    int64_t service_worker_registration_id,
+    const std::string& app_id,
+    const std::string& sender_id,
+    const content::PushMessagingService::UnregisterCallback& callback) {
+  DCHECK(!app_id.empty() || (!origin.is_empty() &&
+                             service_worker_registration_id !=
+                                 -1 /* kInvalidServiceWorkerRegistrationId */))
+      << "Need an app_id and/or origin+service_worker_registration_id";
+
+  RecordUnsubscribeReason(reason);
+
+  if (origin.is_empty() ||
+      service_worker_registration_id ==
+          -1 /* kInvalidServiceWorkerRegistrationId */) {
+    // Can't clear Service Worker database.
+    DidClearPushSubscriptionId(reason, app_id, sender_id, callback);
     return;
   }
-
-  Unsubscribe(app_identifier.app_id(), sender_id, callback);
+  ClearPushSubscriptionId(
+      profile_, origin, service_worker_registration_id,
+      base::Bind(&PushMessagingServiceImpl::DidClearPushSubscriptionId,
+                 weak_factory_.GetWeakPtr(), reason, app_id, sender_id,
+                 callback));
 }
 
-void PushMessagingServiceImpl::Unsubscribe(
+void PushMessagingServiceImpl::DidClearPushSubscriptionId(
+    content::PushUnregistrationReason reason,
     const std::string& app_id,
     const std::string& sender_id,
     const content::PushMessagingService::UnregisterCallback& callback) {

[Peter Beverloo, 2016/09/30 14:26:06]

Specific to UnregisterCallback, since this is the PushMessagingServiceImpl we
could import the interface's type in the header w/ a

  using UnregisterCallback = PushMessagingService::UnregisterCallback;

I wish that'd happen by default anyway, since the entire
"content::PushMessagingService::" prefix adds no value.

[Peter Beverloo, 2016/09/30 14:26:06]

We probably want to have a `using` statement in this file for
PushMessagingService.

[johnme, 2016/09/30 17:02:10]

I just removed all occurrences of (content::)?PushMessagingService:: except for
one in an anonymous function =D

[johnme, 2016/09/30 17:02:10]

I just removed all occurrences of (content::)?PushMessagingService:: except for
one in an anonymous function =D

+  if (app_id.empty()) {
+    // There's nothing more we can do.

[Peter Beverloo, 2016/09/30 14:26:06]

// We cannot unsubscribe with the GCM Driver without an |app_id|.

(The reader now has to parse the rest of the call flow to figure out what
possibly could have happened if we had one.)

[johnme, 2016/09/30 17:02:10]

Done (expanded to cover PushMessagingAppIdentifier map).

+    callback.Run(
+        content::PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);
+    return;
+  }
+
   // Delete the mapping for this app_id, to guarantee that no messages get
   // delivered in future (even if unregistration fails).
   // TODO(johnme): Instead of deleting these app ids, store them elsewhere, and
@@ -641,10 +750,14 @@ void PushMessagingServiceImpl::Unsubscribe(
     GetInstanceIDDriver()->GetInstanceID(app_id)->DeleteID(base::Bind(
         &PushMessagingServiceImpl::DidDeleteID, weak_factory_.GetWeakPtr(),
         app_id, was_subscribed, callback));
+
   } else {
     auto unregister_callback =
-        base::Bind(&PushMessagingServiceImpl::DidUnsubscribe,
-                   weak_factory_.GetWeakPtr(), was_subscribed, callback);
+        base::Bind(&GCMCallbackToUnregisterCallback,
+                   base::Bind(&PushMessagingServiceImpl::DidUnsubscribe,
+                              weak_factory_.GetWeakPtr(),
+                              std::string() /* app_id_if_was_instance_id */,
+                              was_subscribed, callback));
 #if defined(OS_ANDROID)
     // On Android the backend is different, and requires the original sender_id.
     // UnsubscribeBecausePermissionRevoked sometimes calls us with an empty one.
@@ -665,78 +778,33 @@ void PushMessagingServiceImpl::DidDeleteID(
     bool was_subscribed,
     const content::PushMessagingService::UnregisterCallback& callback,
     InstanceID::Result result) {
-  // DidUnsubscribeInstanceID must be run asynchronously, since it calls
+  // DidUnsubscribe must be run asynchronously when passing a non-empty
+  // |app_id_if_was_instance_id|, since it calls
   // InstanceIDDriver::RemoveInstanceID which deletes the InstanceID itself.
   // Calling that immediately would cause a use-after-free in our caller.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&PushMessagingServiceImpl::DidUnsubscribeInstanceID,
+      FROM_HERE, base::Bind(&PushMessagingServiceImpl::DidUnsubscribe,
                             weak_factory_.GetWeakPtr(), app_id, was_subscribed,
-                            callback, result));
-}
-
-void PushMessagingServiceImpl::DidUnsubscribeInstanceID(
-    const std::string& app_id,
-    bool was_subscribed,
-    const content::PushMessagingService::UnregisterCallback& callback,
-    InstanceID::Result result) {
-  GetInstanceIDDriver()->RemoveInstanceID(app_id);
-
-  if (was_subscribed)
-    DecreasePushSubscriptionCount(1, false /* was_pending */);
-
-  DCHECK(!callback.is_null());
-
-  if (!was_subscribed) {
-    callback.Run(
-        content::PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);
-    return;
-  }
-  switch (result) {
-    case InstanceID::SUCCESS:
-      callback.Run(content::PUSH_UNREGISTRATION_STATUS_SUCCESS_UNREGISTERED);
-      break;
-    case InstanceID::INVALID_PARAMETER:
-    case InstanceID::DISABLED:
-    case InstanceID::SERVER_ERROR:
-    case InstanceID::UNKNOWN_ERROR:
-      callback.Run(content::PUSH_UNREGISTRATION_STATUS_PENDING_SERVICE_ERROR);
-      break;
-    case InstanceID::ASYNC_OPERATION_PENDING:
-    case InstanceID::NETWORK_ERROR:
-      callback.Run(content::PUSH_UNREGISTRATION_STATUS_PENDING_NETWORK_ERROR);
-      break;
-  }
+                            callback, ToUnregisterStatus(result)));
 }
 
 void PushMessagingServiceImpl::DidUnsubscribe(
+    const std::string& app_id_if_was_instance_id,
     bool was_subscribed,
     const content::PushMessagingService::UnregisterCallback& callback,
-    gcm::GCMClient::Result gcm_result) {
-  if (was_subscribed)
-    DecreasePushSubscriptionCount(1, false /* was_pending */);
+    content::PushUnregistrationStatus status) {
+  if (!app_id_if_was_instance_id.empty())
+    GetInstanceIDDriver()->RemoveInstanceID(app_id_if_was_instance_id);
 
   DCHECK(!callback.is_null());
 
-  if (!was_subscribed) {
+  if (was_subscribed) {
+    DecreasePushSubscriptionCount(1, false /* was_pending */);
+    callback.Run(status);
+  } else {
+    // Override status since there was no record of an existing subscription.
     callback.Run(
         content::PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);
-    return;
-  }
-  switch (gcm_result) {
-    case gcm::GCMClient::SUCCESS:
-      callback.Run(content::PUSH_UNREGISTRATION_STATUS_SUCCESS_UNREGISTERED);
-      break;
-    case gcm::GCMClient::INVALID_PARAMETER:
-    case gcm::GCMClient::GCM_DISABLED:
-    case gcm::GCMClient::SERVER_ERROR:
-    case gcm::GCMClient::UNKNOWN_ERROR:
-      callback.Run(content::PUSH_UNREGISTRATION_STATUS_PENDING_SERVICE_ERROR);
-      break;
-    case gcm::GCMClient::ASYNC_OPERATION_PENDING:
-    case gcm::GCMClient::NETWORK_ERROR:
-    case gcm::GCMClient::TTL_EXCEEDED:
-      callback.Run(content::PUSH_UNREGISTRATION_STATUS_PENDING_NETWORK_ERROR);
-      break;
   }
 }
 
@@ -787,38 +855,36 @@ void PushMessagingServiceImpl::OnContentSettingChanged(
           app_identifier.service_worker_registration_id(),
           base::Bind(
               &PushMessagingServiceImpl::UnsubscribeBecausePermissionRevoked,
-              weak_factory_.GetWeakPtr(), app_identifier, barrier_closure));
+              weak_factory_.GetWeakPtr(), app_identifier,
+              base::Bind(&UnregisterCallbackToClosure, barrier_closure)));
     } else {
-      UnsubscribeBecausePermissionRevoked(
-          app_identifier, barrier_closure, "" /* sender_id */,
-          true /* success */, true /* not_found */);
+      UnsubscribeInternal(
+          content::PUSH_UNREGISTRATION_REASON_PERMISSION_REVOKED,
+          app_identifier.origin(),
+          app_identifier.service_worker_registration_id(),
+          app_identifier.app_id(), std::string() /* sender_id */,
+          base::Bind(&UnregisterCallbackToClosure, barrier_closure));
     }
   }
 }
 
 void PushMessagingServiceImpl::UnsubscribeBecausePermissionRevoked(
     const PushMessagingAppIdentifier& app_identifier,
-    const base::Closure& closure,
+    const content::PushMessagingService::UnregisterCallback& callback,
     const std::string& sender_id,
     bool success,
     bool not_found) {
-  base::Closure barrier_closure = base::BarrierClosure(2, closure);
-
   // Unsubscribe the PushMessagingAppIdentifier with the push service.
   // It's possible for GetSenderId to have failed and sender_id to be empty, if
   // cookies (and the SW database) for an origin got cleared before permissions
   // are cleared for the origin. In that case for legacy GCM registrations on
   // Android, Unsubscribe will just delete the app identifier to block future
   // messages.
-  // TODO(johnme): Auto-unregister before SW DB is cleared
-  // (https://crbug.com/402458).
-  Unsubscribe(app_identifier.app_id(), sender_id,
-              base::Bind(&UnregisterCallbackToClosure, barrier_closure));
-
-  // Clear the associated service worker push registration id.
-  ClearPushSubscriptionID(profile_, app_identifier.origin(),
-                          app_identifier.service_worker_registration_id(),
-                          barrier_closure);
+  // TODO(johnme): Auto-unregister before SW DB is cleared (crbug.com/402458).
+  UnsubscribeInternal(content::PUSH_UNREGISTRATION_REASON_PERMISSION_REVOKED,
+                      app_identifier.origin(),
+                      app_identifier.service_worker_registration_id(),
+                      app_identifier.app_id(), sender_id, callback);
 }
 
 void PushMessagingServiceImpl::SetContentSettingChangedCallbackForTesting(