Push API: Fix unsubscribing from GCM on Android

We were calling Android GCM's unregister API, which clears all of an app's subscriptions (all sender ids and subtypes). Since from Android's point of view, the app is Chrome, if any webapp unsubscribed from push messaging, all webapps would be unsubscribed!

Instead now we call the unsubscribe API, which only unsubscribes a specific (sender_id,subtype) pair (where the subtype corresponds to GCMDriver's app_id). So now if a Service Worker calls pushSubscription.unsubscribe(), it'll only unsubscribe that SW's push subscription.

In order for this to work, we now also call the corresponding subscribe API (accepting a single sender_id) instead of register.

GoogleCloudMessagingV2.java is still temporary code, that will be removed once we switch to the Google Play Services client library.

BUG=457374

Review URL: https://codereview.chromium.org/914693002

Cr-Commit-Position: refs/heads/master@{#317062}

chrome/browser/services/gcm/push_messaging_browsertest.cc

@@ -89,15 +89,20 @@ class InfoBarResponder : public infobars::InfoBarManager::Observer {
 // FakeGCMProfileService::UnregisterCallback.
 class UnregistrationCallback {
  public:
-  UnregistrationCallback() : done_(false) {}
+  UnregistrationCallback() : done_(false), waiting_(false) {}
 
   void Run(const std::string& app_id) {
     app_id_ = app_id;
     done_ = true;
-    base::MessageLoop::current()->Quit();
+    if (waiting_)
+      base::MessageLoop::current()->Quit();
   }
 
   void WaitUntilSatisfied() {
+    if (done_)
+      return;
+
+    waiting_ = true;
     while (!done_)
       content::RunMessageLoop();
   }
@@ -108,6 +113,7 @@ class UnregistrationCallback {
 
  private:
   bool done_;
+  bool waiting_;
   std::string app_id_;
 };
 
@@ -458,6 +464,7 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, PushEventSuccess) {
   ASSERT_EQ("true - is controlled", script_result);
 
   GCMClient::IncomingMessage message;
+  message.sender_id = "1234567890";
   message.data["data"] = "testdata";
   push_service()->OnMessage(app_id.app_id_guid(), message);
   ASSERT_TRUE(RunScript("resultQueue.pop()", &script_result));
@@ -495,48 +502,7 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, PushEventNoServiceWorker) {
                                                   base::Unretained(&callback)));
 
   GCMClient::IncomingMessage message;
-  message.data["data"] = "testdata";
-  push_service()->OnMessage(app_id.app_id_guid(), message);
-
-  callback.WaitUntilSatisfied();
-  EXPECT_EQ(app_id.app_id_guid(), callback.app_id());
-
-  // No push data should have been received.
-  ASSERT_TRUE(RunScript("resultQueue.popImmediately()", &script_result));
-  EXPECT_EQ("null", script_result);
-}
-
-IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, PushEventNoPermission) {
-  if (!IsPushSupported())
-    return;
-
-  std::string script_result;
-
-  TryToRegisterSuccessfully("1-0" /* expected_push_registration_id */);
-
-  PushMessagingApplicationId app_id = GetServiceWorkerAppId(0LL);
-  EXPECT_EQ(app_id.app_id_guid(), gcm_service()->last_registered_app_id());
-  EXPECT_EQ("1234567890", gcm_service()->last_registered_sender_ids()[0]);
-
-  ASSERT_TRUE(RunScript("isControlled()", &script_result));
-  ASSERT_EQ("false - is not controlled", script_result);
-
-  LoadTestPage();  // Reload to become controlled.
-
-  ASSERT_TRUE(RunScript("isControlled()", &script_result));
-  ASSERT_EQ("true - is controlled", script_result);
-
-  // Revoke Push permission.
-  browser()->profile()->GetHostContentSettingsMap()->
-      ClearSettingsForOneType(CONTENT_SETTINGS_TYPE_PUSH_MESSAGING);
-
-  // When the push service will receive its next message, given that there is no
-  // SW available, it should unregister |app_id|.
-  UnregistrationCallback callback;
-  gcm_service()->SetUnregisterCallback(base::Bind(&UnregistrationCallback::Run,
-                                                  base::Unretained(&callback)));
-
-  GCMClient::IncomingMessage message;
+  message.sender_id = "1234567890";
   message.data["data"] = "testdata";
   push_service()->OnMessage(app_id.app_id_guid(), message);
 
@@ -581,6 +547,7 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
   // If the site is visible in an active tab, we should not force a notification
   // to be shown. Try it twice, since we allow one mistake per 10 push events.
   GCMClient::IncomingMessage message;
+  message.sender_id = "1234567890";
   for (int n = 0; n < 2; n++) {
     message.data["data"] = "testdata";
     push_service()->OnMessage(app_id.app_id_guid(), message);
@@ -671,6 +638,7 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
       base::Bind(&NotificationAddedCallback::Run, base::Unretained(&callback)));
 
   GCMClient::IncomingMessage message;
+  message.sender_id = "1234567890";
   message.data["data"] = "shownotification-without-waituntil";
   push_service()->OnMessage(app_id.app_id_guid(), message);
   ASSERT_TRUE(RunScript("resultQueue.pop()", &script_result, web_contents));

chrome/browser/services/gcm/push_messaging_service_impl.cc

@@ -179,6 +179,8 @@ void PushMessagingServiceImpl::ShutdownHandler() {
   // TODO(johnme): Do any necessary cleanup.
 }
 
+// OnMessage methods -----------------------------------------------------------
+
 void PushMessagingServiceImpl::OnMessage(
     const std::string& app_id,
     const GCMClient::IncomingMessage& message) {
@@ -234,42 +236,6 @@ void PushMessagingServiceImpl::OnMessage(
                  application_id.service_worker_registration_id(), message));
 }
 
-void PushMessagingServiceImpl::OnContentSettingChanged(
-    const ContentSettingsPattern& primary_pattern,
-    const ContentSettingsPattern& secondary_pattern,
-    ContentSettingsType content_type,
-    std::string resource_identifier) {
-  if (content_type != CONTENT_SETTINGS_TYPE_PUSH_MESSAGING &&
-      content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {
-    return;
-  }
-
-  for (const auto& id : PushMessagingApplicationId::GetAll(profile_)) {
-    // If |primary_pattern| is not valid, we should always check for a
-    // permission change because it can happen for example when the entire
-    // Push or Notifications permissions are cleared.
-    // Otherwise, the permission should be checked if the pattern matches the
-    // origin.
-    if (primary_pattern.IsValid() && !primary_pattern.Matches(id.origin()))
-      continue;
-
-    if (HasPermission(id.origin()))
-      continue;
-
-    // Unregister the PushMessagingApplicationId with the push service.
-    Unregister(id.app_id_guid(), true /* retry */, UnregisterCallback());
-
-    // Clear the associated service worker push registration id.
-    PushMessagingService::ClearPushRegistrationID(
-        profile_, id.origin(), id.service_worker_registration_id());
-  }
-}
-
-void PushMessagingServiceImpl::SetProfileForTesting(Profile* profile) {
-  profile_ = profile;
-  profile_->GetHostContentSettingsMap()->AddObserver(this);
-}
-
 void PushMessagingServiceImpl::DeliverMessageCallback(
     const std::string& app_id_guid,
     const GURL& requesting_origin,
@@ -294,7 +260,8 @@ void PushMessagingServiceImpl::DeliverMessageCallback(
     case content::PUSH_DELIVERY_STATUS_UNKNOWN_APP_ID:
     case content::PUSH_DELIVERY_STATUS_PERMISSION_DENIED:
     case content::PUSH_DELIVERY_STATUS_NO_SERVICE_WORKER:
-      Unregister(app_id_guid, true /*retry_on_failure*/, UnregisterCallback());
+      Unregister(app_id_guid, message.sender_id, true /* retry_on_failure */,
+                 UnregisterCallback());
       break;
   }
 }
@@ -451,6 +418,8 @@ void PushMessagingServiceImpl::DidGetNotificationsShown(
   }
 }
 
+// Other GCMAppHandler methods -------------------------------------------------
+
 void PushMessagingServiceImpl::OnMessagesDeleted(const std::string& app_id) {
   // TODO(mvanouwerkerk): Fire push error event on the Service Worker
   // corresponding to app_id.
@@ -468,10 +437,14 @@ void PushMessagingServiceImpl::OnSendAcknowledged(
   NOTREACHED() << "The Push API shouldn't have sent messages upstream";
 }
 
+// GetPushEndpoint method ------------------------------------------------------
+
 GURL PushMessagingServiceImpl::GetPushEndpoint() {
   return GURL(std::string(kPushMessagingEndpoint));
 }
 
+// Register and GetPermissionStatus methods ------------------------------------
+
 void PushMessagingServiceImpl::RegisterFromDocument(
     const GURL& requesting_origin,
     int64 service_worker_registration_id,
@@ -636,9 +609,12 @@ void PushMessagingServiceImpl::DidRequestPermission(
                  application_id, register_callback));
 }
 
+// Unregister methods ----------------------------------------------------------
+
 void PushMessagingServiceImpl::Unregister(
     const GURL& requesting_origin,
     int64 service_worker_registration_id,
+    const std::string& sender_id,
     bool retry_on_failure,
     const content::PushMessagingService::UnregisterCallback& callback) {
   DCHECK(gcm_profile_service_->driver());
@@ -653,11 +629,13 @@ void PushMessagingServiceImpl::Unregister(
     return;
   }
 
-  Unregister(application_id.app_id_guid(), retry_on_failure, callback);
+  Unregister(application_id.app_id_guid(), sender_id, retry_on_failure,
+             callback);
 }
 
 void PushMessagingServiceImpl::Unregister(
     const std::string& app_id_guid,
+    const std::string& sender_id,
     bool retry_on_failure,
     const content::PushMessagingService::UnregisterCallback& callback) {
   DCHECK(gcm_profile_service_->driver());
@@ -673,11 +651,17 @@ void PushMessagingServiceImpl::Unregister(
       application_id.DeleteFromDisk(profile_);
   }
 
-  gcm_profile_service_->driver()->Unregister(
-      app_id_guid,
+  const auto& unregister_callback =
       base::Bind(&PushMessagingServiceImpl::DidUnregister,
                  weak_factory_.GetWeakPtr(),
-                 app_id_guid, retry_on_failure, callback));
+                 app_id_guid, retry_on_failure, callback);
+#if defined(OS_ANDROID)
+  // On Android the backend is different, and requires the original sender_id.
+  gcm_profile_service_->driver()->UnregisterWithSenderId(app_id_guid, sender_id,
+                                                         unregister_callback);
+#else
+  gcm_profile_service_->driver()->Unregister(app_id_guid, unregister_callback);
+#endif
 }
 
 void PushMessagingServiceImpl::DidUnregister(
@@ -729,6 +713,52 @@ void PushMessagingServiceImpl::DidUnregister(
   }
 }
 
+// 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_PUSH_MESSAGING &&
+      content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {
+    return;
+  }
+
+  for (const auto& id : PushMessagingApplicationId::GetAll(profile_)) {
+    // If |primary_pattern| is not valid, we should always check for a
+    // permission change because it can happen for example when the entire
+    // Push or Notifications permissions are cleared.
+    // Otherwise, the permission should be checked if the pattern matches the
+    // origin.
+    if (primary_pattern.IsValid() && !primary_pattern.Matches(id.origin()))
+      continue;
+
+    if (HasPermission(id.origin()))
+      continue;
+
+    PushMessagingService::GetSenderId(
+        profile_, id.origin(), id.service_worker_registration_id(),
+        base::Bind(
+            &PushMessagingServiceImpl::UnregisterBecausePermissionRevoked,
+            weak_factory_.GetWeakPtr(), id));
+  }
+}
+
+void PushMessagingServiceImpl::UnregisterBecausePermissionRevoked(
+    const PushMessagingApplicationId& id,
+    const std::string& sender_id, bool success, bool not_found) {
+  // Unregister the PushMessagingApplicationId with the push service.
+  Unregister(id.app_id_guid(), sender_id, true /* retry_on_failure */,
+             UnregisterCallback());
+
+  // Clear the associated service worker push registration id.
+  PushMessagingService::ClearPushRegistrationID(
+      profile_, id.origin(), id.service_worker_registration_id());
+}
+
+// Helper methods --------------------------------------------------------------
+
 bool PushMessagingServiceImpl::HasPermission(const GURL& origin) {
   gcm::PushMessagingPermissionContext* permission_context =
       gcm::PushMessagingPermissionContextFactory::GetForProfile(profile_);
@@ -738,4 +768,9 @@ bool PushMessagingServiceImpl::HasPermission(const GURL& origin) {
       CONTENT_SETTING_ALLOW;
 }
 
+void PushMessagingServiceImpl::SetProfileForTesting(Profile* profile) {
+  profile_ = profile;
+  profile_->GetHostContentSettingsMap()->AddObserver(this);
+}
+
 }  // namespace gcm

chrome/browser/services/gcm/push_messaging_service_impl.h

@@ -69,6 +69,7 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
   void Unregister(
       const GURL& requesting_origin,
       int64 service_worker_registration_id,
+      const std::string& sender_id,
       bool retry_on_failure,
       const content::PushMessagingService::UnregisterCallback&) override;
   blink::WebPushPermissionStatus GetPermissionStatus(
@@ -88,6 +89,8 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
   void IncreasePushRegistrationCount(int add, bool is_pending);
   void DecreasePushRegistrationCount(int subtract, bool was_pending);
 
+  // OnMessage methods ---------------------------------------------------------
+
   void DeliverMessageCallback(const std::string& app_id_guid,
                               const GURL& requesting_origin,
                               int64 service_worker_registration_id,
@@ -109,6 +112,8 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
       bool success,
       bool not_found);
 
+  // Register methods ----------------------------------------------------------
+
   void RegisterEnd(
       const content::PushMessagingService::RegisterCallback& callback,
       const std::string& registration_id,
@@ -126,7 +131,10 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
       const content::PushMessagingService::RegisterCallback& callback,
       bool allow);
 
+  // Unregister methods --------------------------------------------------------
+
   void Unregister(const std::string& app_id_guid,
+                  const std::string& sender_id,
                   bool retry_on_failure,
                   const content::PushMessagingService::UnregisterCallback&);
 
@@ -135,7 +143,15 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
                      const content::PushMessagingService::UnregisterCallback&,
                      GCMClient::Result result);
 
-  // Helper method that checks if a given origin is allowed to use Push.
+  // OnContentSettingChanged methods -------------------------------------------
+
+  void UnregisterBecausePermissionRevoked(const PushMessagingApplicationId& id,
+                                          const std::string& sender_id,
+                                          bool success, bool not_found);
+
+  // Helper methods ------------------------------------------------------------
+
+  // Checks if a given origin is allowed to use Push.
   bool HasPermission(const GURL& origin);
 
   GCMProfileService* gcm_profile_service_;  // It owns us.

components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java

@@ -37,10 +37,12 @@ public class GCMDriver {
 
     private long mNativeGCMDriverAndroid;
     private final Context mContext;
+    private final GoogleCloudMessagingV2 mGcm;
 
     private GCMDriver(long nativeGCMDriverAndroid, Context context) {
         mNativeGCMDriverAndroid = nativeGCMDriverAndroid;
         mContext = context;
+        mGcm = new GoogleCloudMessagingV2(context);
     }
 
     /**
@@ -72,18 +74,16 @@ public class GCMDriver {
     }
 
     @CalledByNative
-    private void register(final String appId, final String[] senderIds) {
+    private void register(final String appId, final String senderId) {
         new AsyncTask<Void, Void, String>() {
             @Override
             protected String doInBackground(Void... voids) {
-                // TODO(johnme): Should check if GMS is installed on the device first. Ditto below.
                 try {
                     String subtype = appId;
-                    GoogleCloudMessagingV2 gcm = new GoogleCloudMessagingV2(mContext);
-                    String registrationId = gcm.register(subtype, senderIds);
+                    String registrationId = mGcm.subscribe(senderId, subtype, null);
                     return registrationId;
                 } catch (IOException ex) {
-                    Log.w(TAG, "GCMv2 registration failed for " + appId, ex);
+                    Log.w(TAG, "GCM subscription failed for " + appId + ", " + senderId, ex);
                     return "";
                 }
             }
@@ -96,17 +96,16 @@ public class GCMDriver {
     }
 
     @CalledByNative
-    private void unregister(final String appId) {
+    private void unregister(final String appId, final String senderId) {
         new AsyncTask<Void, Void, Boolean>() {
             @Override
             protected Boolean doInBackground(Void... voids) {
                 try {
                     String subtype = appId;
-                    GoogleCloudMessagingV2 gcm = new GoogleCloudMessagingV2(mContext);
-                    gcm.unregister(subtype);
+                    mGcm.unsubscribe(senderId, subtype, null);
                     return true;
                 } catch (IOException ex) {
-                    Log.w(TAG, "GCMv2 unregistration failed for " + appId, ex);
+                    Log.w(TAG, "GCM unsubscription failed for " + appId + ", " + senderId, ex);
                     return false;
                 }
             }

components/gcm_driver/gcm_driver.cc

@@ -70,6 +70,20 @@ void GCMDriver::Register(const std::string& app_id,
 
 void GCMDriver::Unregister(const std::string& app_id,
                            const UnregisterCallback& callback) {
+  UnregisterInternal(app_id, nullptr /* sender_id */, callback);
+}
+
+void GCMDriver::UnregisterWithSenderId(
+    const std::string& app_id,
+    const std::string& sender_id,
+    const UnregisterCallback& callback) {
+  DCHECK(!sender_id.empty());
+  UnregisterInternal(app_id, &sender_id, callback);
+}
+
+void GCMDriver::UnregisterInternal(const std::string& app_id,
+                                   const std::string* sender_id,
+                                   const UnregisterCallback& callback) {
   DCHECK(!app_id.empty());
   DCHECK(!callback.is_null());
 
@@ -88,7 +102,10 @@ void GCMDriver::Unregister(const std::string& app_id,
 
   unregister_callbacks_[app_id] = callback;
 
-  UnregisterImpl(app_id);
+  if (sender_id)
+    UnregisterWithSenderIdImpl(app_id, *sender_id);
+  else
+    UnregisterImpl(app_id);
 }
 
 void GCMDriver::Send(const std::string& app_id,
@@ -117,6 +134,11 @@ void GCMDriver::Send(const std::string& app_id,
   SendImpl(app_id, receiver_id, message);
 }
 
+void GCMDriver::UnregisterWithSenderIdImpl(const std::string& app_id,
+                                           const std::string& sender_id) {
+  NOTREACHED();
+}
+
 void GCMDriver::RegisterFinished(const std::string& app_id,
                                  const std::string& registration_id,
                                  GCMClient::Result result) {

components/gcm_driver/gcm_driver.h

@@ -37,8 +37,9 @@ class GCMDriver {
   GCMDriver();
   virtual ~GCMDriver();
 
-  // Registers |sender_id| for an app. A registration ID will be returned by
-  // the GCM server.
+  // Registers |sender_ids| for an app. A registration ID will be returned by
+  // the GCM server. On Android, only a single sender ID is supported, but
+  // instead multiple simultaneous registrations are allowed.
   // |app_id|: application ID.
   // |sender_ids|: list of IDs of the servers that are allowed to send the
   //               messages to the application. These IDs are assigned by the
@@ -48,12 +49,22 @@ class GCMDriver {
                 const std::vector<std::string>& sender_ids,
                 const RegisterCallback& callback);
 
-  // Unregisters an app from using GCM.
+  // Unregisters all sender_ids for an app. Only works on non-Android.
   // |app_id|: application ID.
   // |callback|: to be called once the asynchronous operation is done.
   void Unregister(const std::string& app_id,
                   const UnregisterCallback& callback);
 
+  // Unregisters an (app_id, sender_id) pair from using GCM. Only works on
+  // Android.
+  // TODO(jianli): Switch to using GCM's unsubscribe API.
+  // |app_id|: application ID.
+  // |sender_id|: the sender ID that was passed when registering.
+  // |callback|: to be called once the asynchronous operation is done.
+  void UnregisterWithSenderId(const std::string& app_id,
+                              const std::string& sender_id,
+                              const UnregisterCallback& callback);
+
   // Sends a message to a given receiver.
   // |app_id|: application ID.
   // |receiver_id|: registration ID of the receiver party.
@@ -146,6 +157,10 @@ class GCMDriver {
   // Platform-specific implementation of Unregister.
   virtual void UnregisterImpl(const std::string& app_id) = 0;
 
+  // Platform-specific implementation of UnregisterWithSenderId.
+  virtual void UnregisterWithSenderIdImpl(const std::string& app_id,
+                                          const std::string& sender_id);
+
   // Platform-specific implementation of Send.
   virtual void SendImpl(const std::string& app_id,
                         const std::string& receiver_id,
@@ -170,6 +185,11 @@ class GCMDriver {
   void ClearCallbacks();
 
  private:
+  // Common code shared by Unregister and UnregisterWithSenderId.
+  void UnregisterInternal(const std::string& app_id,
+                          const std::string* sender_id,
+                          const UnregisterCallback& callback);
+
   // Called after unregistration completes in order to trigger the pending
   // registration.
   void RegisterAfterUnregister(

components/gcm_driver/gcm_driver_android.cc

@@ -169,20 +169,26 @@ GCMClient::Result GCMDriverAndroid::EnsureStarted(
 }
 
 void GCMDriverAndroid::RegisterImpl(
-    const std::string& app_id,
-    const std::vector<std::string>& sender_ids) {
+    const std::string& app_id, const std::vector<std::string>& sender_ids) {
+  DCHECK_EQ(1u, sender_ids.size());
   JNIEnv* env = AttachCurrentThread();
   Java_GCMDriver_register(
       env, java_ref_.obj(),
       ConvertUTF8ToJavaString(env, app_id).Release(),
-      ToJavaArrayOfStrings(env, sender_ids).obj());
+      ConvertUTF8ToJavaString(env, sender_ids[0]).Release());
 }
 
 void GCMDriverAndroid::UnregisterImpl(const std::string& app_id) {
+  NOTREACHED();
+}
+
+void GCMDriverAndroid::UnregisterWithSenderIdImpl(
+    const std::string& app_id, const std::string& sender_id) {
   JNIEnv* env = AttachCurrentThread();
   Java_GCMDriver_unregister(
       env, java_ref_.obj(),
-      ConvertUTF8ToJavaString(env, app_id).Release());
+      ConvertUTF8ToJavaString(env, app_id).Release(),
+      ConvertUTF8ToJavaString(env, sender_id).Release());
 }
 
 void GCMDriverAndroid::SendImpl(const std::string& app_id,

components/gcm_driver/gcm_driver_android.h

@@ -71,6 +71,8 @@ class GCMDriverAndroid : public GCMDriver {
   void RegisterImpl(const std::string& app_id,
                     const std::vector<std::string>& sender_ids) override;
   void UnregisterImpl(const std::string& app_id) override;
+  void UnregisterWithSenderIdImpl(const std::string& app_id,
+                                  const std::string& sender_id) override;
   void SendImpl(const std::string& app_id,
                 const std::string& receiver_id,
                 const GCMClient::OutgoingMessage& message) override;

components/gcm_driver/gcm_driver_desktop_unittest.cc

@@ -295,8 +295,8 @@ void GCMDriverTest::Unregister(const std::string& app_id,
   base::RunLoop run_loop;
   async_operation_completed_callback_ = run_loop.QuitClosure();
   driver_->Unregister(app_id,
-                       base::Bind(&GCMDriverTest::UnregisterCompleted,
-                                  base::Unretained(this)));
+                      base::Bind(&GCMDriverTest::UnregisterCompleted,
+                                 base::Unretained(this)));
   if (wait_to_finish == WAIT)
     run_loop.Run();
 }

content/browser/push_messaging/push_messaging_message_filter.cc

@@ -24,6 +24,7 @@
 
 namespace content {
 
+const char kPushSenderIdServiceWorkerKey[] = "push_sender_id";
 const char kPushRegistrationIdServiceWorkerKey[] = "push_registration_id";
 
 namespace {
@@ -38,8 +39,6 @@ void RecordRegistrationStatus(PushRegistrationStatus status) {
                             PUSH_REGISTRATION_STATUS_LAST + 1);
 }
 
-const char kSenderIdServiceWorkerKey[] = "push_sender_id";
-
 }  // namespace
 
 struct PushMessagingMessageFilter::RegisterData {
@@ -71,7 +70,8 @@ class PushMessagingMessageFilter::Core {
   // Called via PostTask from IO thread.
   void UnregisterFromService(int request_id,
                              int64 service_worker_registration_id,
-                             const GURL& requesting_origin);
+                             const GURL& requesting_origin,
+                             const std::string& sender_id);
 
   // Public GetPermission methods on UI thread ---------------------------------
 
@@ -212,7 +212,7 @@ void PushMessagingMessageFilter::OnRegisterFromDocument(
   service_worker_context_->context()->storage()->StoreUserData(
       service_worker_registration_id,
       data.requesting_origin,
-      kSenderIdServiceWorkerKey,
+      kPushSenderIdServiceWorkerKey,
       sender_id,
       base::Bind(&PushMessagingMessageFilter::DidPersistSenderId,
                  weak_factory_io_to_io_.GetWeakPtr(),
@@ -286,7 +286,7 @@ void PushMessagingMessageFilter::DidCheckForExistingRegistration(
   } else {
     service_worker_context_->context()->storage()->GetUserData(
         data.service_worker_registration_id,
-        kSenderIdServiceWorkerKey,
+        kPushSenderIdServiceWorkerKey,
         base::Bind(&PushMessagingMessageFilter::DidGetSenderIdFromStorage,
                    weak_factory_io_to_io_.GetWeakPtr(), data));
   }
@@ -436,18 +436,46 @@ void PushMessagingMessageFilter::OnUnregister(
   service_worker_context_->context()->storage()->GetUserData(
       service_worker_registration_id,
       kPushRegistrationIdServiceWorkerKey,
-      base::Bind(&PushMessagingMessageFilter::DoUnregister,
-                 weak_factory_io_to_io_.GetWeakPtr(),
-                 request_id,
-                 service_worker_registration_id,
-                 service_worker_registration->pattern().GetOrigin()));
+      base::Bind(
+          &PushMessagingMessageFilter::UnregisterHavingGottenPushRegistrationId,
+          weak_factory_io_to_io_.GetWeakPtr(), request_id,
+          service_worker_registration_id,
+          service_worker_registration->pattern().GetOrigin()));
 }
 
-void PushMessagingMessageFilter::DoUnregister(
+void PushMessagingMessageFilter::UnregisterHavingGottenPushRegistrationId(
     int request_id,
     int64 service_worker_registration_id,
     const GURL& requesting_origin,
-    const std::string& push_registration_id,
+    const std::string& push_registration_id,  // Unused, we just want the status
+    ServiceWorkerStatusCode service_worker_status) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  if (service_worker_status == SERVICE_WORKER_OK) {
+    service_worker_context_->context()->storage()->GetUserData(
+        service_worker_registration_id,
+        kPushSenderIdServiceWorkerKey,
+        base::Bind(
+            &PushMessagingMessageFilter::UnregisterHavingGottenSenderId,
+            weak_factory_io_to_io_.GetWeakPtr(),
+            request_id,
+            service_worker_registration_id,
+            requesting_origin));
+  } else {
+    // Errors are handled the same, whether we were trying to get the
+    // push_registration_id or the sender_id.
+    UnregisterHavingGottenSenderId(request_id, service_worker_registration_id,
+                                   requesting_origin,
+                                   std::string() /* sender_id */,
+                                   service_worker_status);
+  }
+}
+
+void PushMessagingMessageFilter::UnregisterHavingGottenSenderId(
+    int request_id,
+    int64 service_worker_registration_id,
+    const GURL& requesting_origin,
+    const std::string& sender_id,
     ServiceWorkerStatusCode service_worker_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -457,7 +485,8 @@ void PushMessagingMessageFilter::DoUnregister(
           BrowserThread::UI, FROM_HERE,
           base::Bind(&Core::UnregisterFromService,
                      base::Unretained(ui_core_.get()), request_id,
-                     service_worker_registration_id, requesting_origin));
+                     service_worker_registration_id, requesting_origin,
+                     sender_id));
       return;
     case SERVICE_WORKER_ERROR_NOT_FOUND:
       // We did not find a registration, stop here and notify the renderer that
@@ -490,7 +519,8 @@ void PushMessagingMessageFilter::DoUnregister(
 void PushMessagingMessageFilter::Core::UnregisterFromService(
     int request_id,
     int64 service_worker_registration_id,
-    const GURL& requesting_origin) {
+    const GURL& requesting_origin,
+    const std::string& sender_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   PushMessagingService* push_service = service();
   if (!push_service) {
@@ -502,7 +532,7 @@ void PushMessagingMessageFilter::Core::UnregisterFromService(
   }
 
   push_service->Unregister(
-      requesting_origin, service_worker_registration_id,
+      requesting_origin, service_worker_registration_id, sender_id,
       false /* retry_on_failure */,
       base::Bind(&Core::DidUnregisterFromService,
                  weak_factory_ui_to_ui_.GetWeakPtr(),

content/browser/push_messaging/push_messaging_message_filter.h

@@ -22,6 +22,7 @@ namespace content {
 class PushMessagingService;
 class ServiceWorkerContextWrapper;
 
+extern const char kPushSenderIdServiceWorkerKey[];
 extern const char kPushRegistrationIdServiceWorkerKey[];
 
 class PushMessagingMessageFilter : public BrowserMessageFilter {
@@ -95,11 +96,19 @@ class PushMessagingMessageFilter : public BrowserMessageFilter {
 
   void OnUnregister(int request_id, int64 service_worker_registration_id);
 
-  void DoUnregister(int request_id,
-                    int64 service_worker_registration_id,
-                    const GURL& requesting_origin,
-                    const std::string& push_registration_id,
-                    ServiceWorkerStatusCode service_worker_status);
+  void UnregisterHavingGottenPushRegistrationId(
+      int request_id,
+      int64 service_worker_registration_id,
+      const GURL& requesting_origin,
+      const std::string& push_registration_id,
+      ServiceWorkerStatusCode service_worker_status);
+
+  void UnregisterHavingGottenSenderId(
+      int request_id,
+      int64 service_worker_registration_id,
+      const GURL& requesting_origin,
+      const std::string& sender_id,
+      ServiceWorkerStatusCode service_worker_status);
 
   // Called via PostTask from UI thread.
   void ClearRegistrationData(int request_id,

content/public/browser/push_messaging_service.cc

@@ -17,8 +17,8 @@ namespace {
 const char kNotificationsShownServiceWorkerKey[] =
     "notifications_shown_by_last_few_pushes";
 
-void CallGetNotificationsShownCallbackFromIO(
-    const PushMessagingService::GetNotificationsShownCallback& callback,
+void CallStringCallbackFromIO(
+    const PushMessagingService::StringCallback& callback,
     const std::string& data,
     ServiceWorkerStatusCode service_worker_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -37,14 +37,15 @@ void CallResultCallbackFromIO(
                           base::Bind(callback, success));
 }
 
-void GetNotificationsShownOnIO(
+void GetUserDataOnIO(
     scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_wrapper,
     int64 service_worker_registration_id,
-    const PushMessagingService::GetNotificationsShownCallback& callback) {
+    const std::string& key,
+    const PushMessagingService::StringCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   service_worker_context_wrapper->context()->storage()->GetUserData(
-      service_worker_registration_id, kNotificationsShownServiceWorkerKey,
-      base::Bind(&CallGetNotificationsShownCallbackFromIO, callback));
+      service_worker_registration_id, key,
+      base::Bind(&CallStringCallbackFromIO, callback));
 }
 
 void SetNotificationsShownOnIO(
@@ -73,20 +74,30 @@ void ClearPushRegistrationIDOnIO(
       base::Bind(&OnClearPushRegistrationServiceWorkerKey));
 }
 
+scoped_refptr<ServiceWorkerContextWrapper> GetServiceWorkerContext(
+    BrowserContext* browser_context, const GURL& origin) {
+  StoragePartition* partition =
+      BrowserContext::GetStoragePartitionForSite(browser_context, origin);
+  return make_scoped_refptr(
+      static_cast<ServiceWorkerContextWrapper*>(
+          partition->GetServiceWorkerContext()));
+}
+
 }  // anonymous namespace
 
 // static
 void PushMessagingService::GetNotificationsShownByLastFewPushes(
     ServiceWorkerContext* service_worker_context,
     int64 service_worker_registration_id,
-    const GetNotificationsShownCallback& callback) {
+    const StringCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   scoped_refptr<ServiceWorkerContextWrapper> wrapper =
       static_cast<ServiceWorkerContextWrapper*>(service_worker_context);
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                          base::Bind(&GetNotificationsShownOnIO,
+                          base::Bind(&GetUserDataOnIO,
                                      wrapper,
                                      service_worker_registration_id,
+                                     kNotificationsShownServiceWorkerKey,
                                      callback));
 }
 
@@ -109,23 +120,33 @@ void PushMessagingService::SetNotificationsShownByLastFewPushes(
                                      callback));
 }
 
+// static
+void PushMessagingService::GetSenderId(BrowserContext* browser_context,
+                                       const GURL& origin,
+                                       int64 service_worker_registration_id,
+                                       const StringCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&GetUserDataOnIO,
+                 GetServiceWorkerContext(browser_context, origin),
+                 service_worker_registration_id,
+                 kPushSenderIdServiceWorkerKey,
+                 callback));
+}
+
 // static
 void PushMessagingService::ClearPushRegistrationID(
     BrowserContext* browser_context,
     const GURL& origin,
     int64 service_worker_registration_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  StoragePartition* partition =
-      BrowserContext::GetStoragePartitionForSite(browser_context, origin);
-  scoped_refptr<ServiceWorkerContextWrapper> service_worker_context =
-      static_cast<ServiceWorkerContextWrapper*>(
-          partition->GetServiceWorkerContext());
-
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(&ClearPushRegistrationIDOnIO,
-                 service_worker_context,
+                 GetServiceWorkerContext(browser_context, origin),
                  service_worker_registration_id));
 }
 

content/public/browser/push_messaging_service.h

@@ -22,17 +22,17 @@ class ServiceWorkerContext;
 // push messaging services like GCM. Must only be used on the UI thread.
 class CONTENT_EXPORT PushMessagingService {
  public:
-  using GetNotificationsShownCallback =
-      base::Callback<void(const std::string& notifications_shown,
-                          bool success, bool not_found)>;
-
-  using ResultCallback = base::Callback<void(bool success)>;
-
   using RegisterCallback =
       base::Callback<void(const std::string& /* registration_id */,
                           PushRegistrationStatus /* status */)>;
   using UnregisterCallback = base::Callback<void(PushUnregistrationStatus)>;
 
+  using StringCallback = base::Callback<void(const std::string& data,
+                                             bool success,
+                                             bool not_found)>;
+
+  using ResultCallback = base::Callback<void(bool success)>;
+
   virtual ~PushMessagingService() {}
 
   // Returns the absolute URL exposed by the push server where the webapp server
@@ -63,6 +63,7 @@ class CONTENT_EXPORT PushMessagingService {
   // the push service.
   virtual void Unregister(const GURL& requesting_origin,
                           int64 service_worker_registration_id,
+                          const std::string& sender_id,
                           bool retry_on_failure,
                           const UnregisterCallback& callback) = 0;
 
@@ -81,7 +82,7 @@ class CONTENT_EXPORT PushMessagingService {
   static void GetNotificationsShownByLastFewPushes(
       ServiceWorkerContext* service_worker_context,
       int64 service_worker_registration_id,
-      const GetNotificationsShownCallback& callback);
+      const StringCallback& callback);
   static void SetNotificationsShownByLastFewPushes(
       ServiceWorkerContext* service_worker_context,
       int64 service_worker_registration_id,
@@ -89,6 +90,11 @@ class CONTENT_EXPORT PushMessagingService {
       const std::string& notifications_shown,
       const ResultCallback& callback);
 
+  static void GetSenderId(BrowserContext* browser_context,
+                          const GURL& origin,
+                          int64 service_worker_registration_id,
+                          const StringCallback& callback);
+
   // Clear the push registration id stored in the service worker with the given
   // |service_worker_registration_id| for the given |origin|.
   static void ClearPushRegistrationID(BrowserContext* browser_context,

content/shell/browser/layout_test/layout_test_push_messaging_service.cc

@@ -67,6 +67,7 @@ LayoutTestPushMessagingService::GetPermissionStatus(
 void LayoutTestPushMessagingService::Unregister(
     const GURL& requesting_origin,
     int64 service_worker_registration_id,
+    const std::string& sender_id,
     bool retry_on_failure,
     const UnregisterCallback& callback) {
   callback.Run(PUSH_UNREGISTRATION_STATUS_SUCCESS_UNREGISTER);

content/shell/browser/layout_test/layout_test_push_messaging_service.h

@@ -42,6 +42,7 @@ class LayoutTestPushMessagingService : public PushMessagingService {
       const GURL& embedding_origin) override;
   void Unregister(const GURL& requesting_origin,
                   int64 service_worker_registration_id,
+                  const std::string& sender_id,
                   bool retry_on_failure,
                   const UnregisterCallback& callback) override;