Push API: Refactor and fix unsubscribe API

chrome/browser/push_messaging/push_messaging_service_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/push_messaging/push_messaging_service_impl.h"
 
 #include <vector>
 
 #include "base/barrier_closure.h"
 #include "base/base64url.h"
@@ skipping 65 matching lines @@
     "Chrome currently only supports the Push API for subscriptions that will "
     "result in user-visible messages. You can indicate this by calling "
     "pushManager.subscribe({userVisibleOnly: true}) instead. See "
     "https://goo.gl/yqv4Q4 for more details.";
 
 void RecordDeliveryStatus(content::PushDeliveryStatus status) {
   UMA_HISTOGRAM_ENUMERATION("PushMessaging.DeliveryStatus", 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) {
     case blink::mojom::PermissionStatus::GRANTED:
       return blink::WebPushPermissionStatusGranted;
     case blink::mojom::PermissionStatus::DENIED:
       return blink::WebPushPermissionStatusDenied;
     case blink::mojom::PermissionStatus::ASK:
       return blink::WebPushPermissionStatusPrompt;
     default:
       NOTREACHED();
       return blink::WebPushPermissionStatusDenied;
   }
 }
 
+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();
 }
 
 #if BUILDFLAG(ENABLE_BACKGROUND)
 bool UseBackgroundMode() {
   // Note: if push is ever enabled in incognito, the background mode integration
   // should not be enabled for it.
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(switches::kDisablePushApiBackgroundMode))
     return false;
@@ skipping 116 matching lines @@
   }
 #endif
 
   base::Closure message_handled_closure =
       message_callback_for_testing_.is_null() ? base::Bind(&base::DoNothing)
                                               : message_callback_for_testing_;
   PushMessagingAppIdentifier app_identifier =
       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;
   }
   // Drop message and unregister if |origin| has lost push permission.
   if (!IsPermissionSet(app_identifier.origin())) {
     DeliverMessageCallback(app_id, app_identifier.origin(),
                            app_identifier.service_worker_registration_id(),
                            message, message_handled_closure,
                            content::PUSH_DELIVERY_STATUS_PERMISSION_DENIED);
     return;
@@ skipping 29 matching lines @@
 
 void PushMessagingServiceImpl::DeliverMessageCallback(
     const std::string& app_id,
     const GURL& requesting_origin,
     int64_t service_worker_registration_id,
     const gcm::IncomingMessage& message,
     const base::Closure& message_handled_closure,
     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);
   // The completion_closure should run by default at the end of this function,
   // 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.
   switch (status) {
     // Call EnforceUserVisibleOnlyRequirements if the message was delivered to
     // the Service Worker JavaScript, even if the website's event handler failed
     // (to prevent sites deliberately failing in order to avoid having to show
     // notifications).
     case content::PUSH_DELIVERY_STATUS_SUCCESS:
     case content::PUSH_DELIVERY_STATUS_EVENT_WAITUNTIL_REJECTED:
 #if defined(ENABLE_NOTIFICATIONS)
       // Only enforce the user visible requirements if this is currently running
       // as the delivery callback for the last in-flight message, and silent
       // push has not been enabled through a command line flag.
       if (in_flight_message_deliveries_.count(app_id) == 1 &&
           !base::CommandLine::ForCurrentProcess()->HasSwitch(
               switches::kAllowSilentPush)) {
         notification_manager_.EnforceUserVisibleOnlyRequirements(
             requesting_origin, service_worker_registration_id,
             completion_closure_runner.Release());
       }
 #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(
     const std::string& app_id,
     const base::Closure& message_handled_closure) {
   auto in_flight_iterator = in_flight_message_deliveries_.find(app_id);
   DCHECK(in_flight_iterator != in_flight_message_deliveries_.end());
 
   // Remove a single in-flight delivery for |app_id|. This has to be done using
   // an iterator rather than by value, as the latter removes all entries.
@@ skipping 265 matching lines @@
 // Unsubscribe methods ---------------------------------------------------------
 
 void PushMessagingServiceImpl::Unsubscribe(
     const GURL& requesting_origin,
     int64_t service_worker_registration_id,
     const std::string& sender_id,
     const content::PushMessagingService::UnregisterCallback& callback) {
   PushMessagingAppIdentifier app_identifier =
       PushMessagingAppIdentifier::FindByServiceWorker(
           profile_, requesting_origin, service_worker_registration_id);
-  if (app_identifier.is_null()) {
+
+  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;
+  }
+  ClearPushSubscriptionId(
+      profile_, origin, service_worker_registration_id,
+      base::Bind(&PushMessagingServiceImpl::DidClearPushSubscriptionId,
+                 weak_factory_.GetWeakPtr(), reason, app_id, sender_id,
+                 callback));
+}
+
+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;
   }
 
-  Unsubscribe(app_identifier.app_id(), sender_id, callback);
-}
-
-void PushMessagingServiceImpl::Unsubscribe(
-    const std::string& app_id,
-    const std::string& sender_id,
-    const content::PushMessagingService::UnregisterCallback& callback) {
   // 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
   // retry unregistration if it fails due to network errors (crbug.com/465399).
   PushMessagingAppIdentifier app_identifier =
       PushMessagingAppIdentifier::FindByAppId(profile_, app_id);
   bool was_subscribed = !app_identifier.is_null();
   if (was_subscribed)
     app_identifier.DeleteFromPrefs(profile_);
 
   if (PushMessagingAppIdentifier::UseInstanceID(app_id)) {
     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.
     if (sender_id.empty()) {
       unregister_callback.Run(gcm::GCMClient::INVALID_PARAMETER);
     } else {
       GetGCMDriver()->UnregisterWithSenderId(
           app_id, NormalizeSenderInfo(sender_id), unregister_callback);
     }
 #else
     GetGCMDriver()->Unregister(app_id, unregister_callback);
 #endif
   }
 }
 
 void PushMessagingServiceImpl::DidDeleteID(
     const std::string& app_id,
     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));
+                            callback, ToUnregisterStatus(result)));
 }
 
-void PushMessagingServiceImpl::DidUnsubscribeInstanceID(
-    const std::string& app_id,
+void PushMessagingServiceImpl::DidUnsubscribe(
+    const std::string& app_id_if_was_instance_id,
     bool was_subscribed,
     const content::PushMessagingService::UnregisterCallback& callback,
-    InstanceID::Result result) {
-  GetInstanceIDDriver()->RemoveInstanceID(app_id);
-
-  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 (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;
   }
 }
 
-void PushMessagingServiceImpl::DidUnsubscribe(
-    bool was_subscribed,
-    const content::PushMessagingService::UnregisterCallback& callback,
-    gcm::GCMClient::Result gcm_result) {
-  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 (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;
-  }
-}
-
 // OnContentSettingChanged methods ---------------------------------------------
 
 void PushMessagingServiceImpl::OnContentSettingChanged(
     const ContentSettingsPattern& primary_pattern,
     const ContentSettingsPattern& secondary_pattern,
     ContentSettingsType content_type,
     std::string resource_identifier) {
   if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS)
     return;
 
@@ skipping 27 matching lines @@
 #if defined(OS_ANDROID)
     need_sender_id =
         !PushMessagingAppIdentifier::UseInstanceID(app_identifier.app_id());
 #endif
     if (need_sender_id) {
       GetSenderId(
           profile_, app_identifier.origin(),
           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(
     const base::Closure& callback) {
   content_setting_changed_callback_for_testing_ = callback;
 }
 
 // KeyedService methods -------------------------------------------------------
 
 void PushMessagingServiceImpl::Shutdown() {
@@ skipping 61 matching lines @@
 }
 
 instance_id::InstanceIDDriver* PushMessagingServiceImpl::GetInstanceIDDriver()
     const {
   instance_id::InstanceIDProfileService* instance_id_profile_service =
       instance_id::InstanceIDProfileServiceFactory::GetForProfile(profile_);
   CHECK(instance_id_profile_service);
   CHECK(instance_id_profile_service->driver());
   return instance_id_profile_service->driver();
 }