Push API: Don't wait for network when unsubscribing

pushSubscription.unsubscribe() used to delete local state then hang for
up to 10 minutes until we managed to tell GCM servers about the
unsubscription (the 10 minutes is due to exponential backoff retries on
desktop, if the device is offline or on a flaky connection).

So unsubscribing offline would never resolve the Promise if the
tab/Service Worker was closed before 10 minutes, as is common, however
the unsubscription would in fact have succeeded to the extent that
subscribe()/getSubscription() correctly appear unsubscribed, and
messages are no longer be delivered.

This patch resolves the unsubscribe() promise as soon as the local state
has been deleted, without waiting for network requests to GCM servers.

Since we no longer use the results of those network requests directly,
UMA logging is added to track them.

To allow testing offline behavior, FakeGCMProfileService can now provide
a coarse simulation of being offline, and I refactored it a little to
simplify things.

BUG=669095,448500

Review-Url: https://codereview.chromium.org/2675293003
Cr-Commit-Position: refs/heads/master@{#449343}
diff --git a/chrome/browser/gcm/fake_gcm_profile_service.cc b/chrome/browser/gcm/fake_gcm_profile_service.cc
index f2db5fb..58c18f0 100644
--- a/chrome/browser/gcm/fake_gcm_profile_service.cc
+++ b/chrome/browser/gcm/fake_gcm_profile_service.cc
@@ -10,20 +10,22 @@
 #include "base/format_macros.h"
 #include "base/location.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/gcm_driver/fake_gcm_client_factory.h"
+#include "components/gcm_driver/gcm_driver.h"
 #include "components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.h"
 #include "content/public/browser/browser_context.h"
 
 namespace gcm {
 
-namespace {
-
-class CustomFakeGCMDriver : public instance_id::FakeGCMDriverForInstanceID {
+class FakeGCMProfileService::CustomFakeGCMDriver
+    : public instance_id::FakeGCMDriverForInstanceID {
  public:
   explicit CustomFakeGCMDriver(FakeGCMProfileService* service);
   ~CustomFakeGCMDriver() override;
@@ -31,8 +33,6 @@
   void OnRegisterFinished(const std::string& app_id,
                           const std::string& registration_id,
                           GCMClient::Result result);
-  void OnUnregisterFinished(const std::string& app_id,
-                            GCMClient::Result result);
   void OnSendFinished(const std::string& app_id,
                       const std::string& message_id,
                       GCMClient::Result result);
@@ -41,7 +41,7 @@
                          const IncomingMessage& message);
 
  protected:
-  // FakeGCMDriverForInstanceID overrides:
+  // FakeGCMDriver overrides:
   void RegisterImpl(const std::string& app_id,
                     const std::vector<std::string>& sender_ids) override;
   void UnregisterImpl(const std::string& app_id) override;
@@ -51,72 +51,151 @@
                 const std::string& receiver_id,
                 const OutgoingMessage& message) override;
 
+  // FakeGCMDriverForInstanceID overrides:
+  void GetToken(const std::string& app_id,
+                const std::string& authorized_entity,
+                const std::string& scope,
+                const std::map<std::string, std::string>& options,
+                const GetTokenCallback& callback) override;
+  void DeleteToken(const std::string& app_id,
+                   const std::string& authorized_entity,
+                   const std::string& scope,
+                   const DeleteTokenCallback& callback) override;
+
  private:
+  void DoRegister(const std::string& app_id,
+                  const std::vector<std::string>& sender_ids,
+                  const std::string& registration_id);
+  void DoSend(const std::string& app_id,
+              const std::string& receiver_id,
+              const OutgoingMessage& message);
+
   FakeGCMProfileService* service_;
 
+  // Used to give each registration a unique registration id. Does not decrease
+  // when unregister is called.
+  int registration_count_ = 0;
+
+  base::WeakPtrFactory<CustomFakeGCMDriver> weak_factory_;  // Must be last.
+
   DISALLOW_COPY_AND_ASSIGN(CustomFakeGCMDriver);
 };
 
-CustomFakeGCMDriver::CustomFakeGCMDriver(FakeGCMProfileService* service)
+FakeGCMProfileService::CustomFakeGCMDriver::CustomFakeGCMDriver(
+    FakeGCMProfileService* service)
     : instance_id::FakeGCMDriverForInstanceID(
           base::ThreadTaskRunnerHandle::Get()),
-      service_(service) {}
+      service_(service),
+      weak_factory_(this) {}
 
-CustomFakeGCMDriver::~CustomFakeGCMDriver() {
-}
+FakeGCMProfileService::CustomFakeGCMDriver::~CustomFakeGCMDriver() {}
 
-void CustomFakeGCMDriver::RegisterImpl(
+void FakeGCMProfileService::CustomFakeGCMDriver::RegisterImpl(
     const std::string& app_id,
     const std::vector<std::string>& sender_ids) {
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&FakeGCMProfileService::RegisterFinished,
-                            base::Unretained(service_), app_id, sender_ids));
-}
+  if (service_->is_offline_)
+    return;  // Drop request.
 
-void CustomFakeGCMDriver::UnregisterImpl(const std::string& app_id) {
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&FakeGCMProfileService::UnregisterFinished,
-                            base::Unretained(service_), app_id));
-}
+  // Generate fake registration IDs, encoding the number of sender IDs (used by
+  // GcmApiTest.RegisterValidation), then an incrementing count (even for the
+  // same app_id - there's no caching) so tests can distinguish registrations.
+  std::string registration_id = base::StringPrintf(
+      "%" PRIuS "-%d", sender_ids.size(), registration_count_);
+  ++registration_count_;
 
-void CustomFakeGCMDriver::UnregisterWithSenderIdImpl(
-    const std::string& app_id,
-    const std::string& sender_id) {}
-
-void CustomFakeGCMDriver::SendImpl(const std::string& app_id,
-                                   const std::string& receiver_id,
-                                   const OutgoingMessage& message) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
-      base::Bind(&FakeGCMProfileService::SendFinished,
-                 base::Unretained(service_), app_id, receiver_id, message));
+      base::Bind(&CustomFakeGCMDriver::DoRegister, weak_factory_.GetWeakPtr(),
+                 app_id, sender_ids, registration_id));
 }
 
-void CustomFakeGCMDriver::OnRegisterFinished(
+void FakeGCMProfileService::CustomFakeGCMDriver::DoRegister(
     const std::string& app_id,
-    const std::string& registration_id,
-    GCMClient::Result result) {
-  RegisterFinished(app_id, registration_id, result);
+    const std::vector<std::string>& sender_ids,
+    const std::string& registration_id) {
+  if (service_->collect_) {
+    service_->last_registered_app_id_ = app_id;
+    service_->last_registered_sender_ids_ = sender_ids;
+  }
+  RegisterFinished(app_id, registration_id, GCMClient::SUCCESS);
 }
 
-void CustomFakeGCMDriver::OnUnregisterFinished(const std::string& app_id,
-                                               GCMClient::Result result) {
-  UnregisterFinished(app_id, result);
+void FakeGCMProfileService::CustomFakeGCMDriver::UnregisterImpl(
+    const std::string& app_id) {
+  if (service_->is_offline_)
+    return;  // Drop request.
+
+  GCMClient::Result result = GCMClient::SUCCESS;
+  if (!service_->unregister_responses_.empty()) {
+    result = service_->unregister_responses_.front();
+    service_->unregister_responses_.pop_front();
+  }
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(&CustomFakeGCMDriver::UnregisterFinished,
+                            weak_factory_.GetWeakPtr(), app_id, result));
 }
 
-void CustomFakeGCMDriver::OnSendFinished(const std::string& app_id,
-                                         const std::string& message_id,
-                                         GCMClient::Result result) {
-  SendFinished(app_id, message_id, result);
+void FakeGCMProfileService::CustomFakeGCMDriver::UnregisterWithSenderIdImpl(
+    const std::string& app_id,
+    const std::string& sender_id) {
+  NOTREACHED() << "This Android-specific method is not yet faked.";
 }
 
-void CustomFakeGCMDriver::OnDispatchMessage(const std::string& app_id,
-                                            const IncomingMessage& message) {
+void FakeGCMProfileService::CustomFakeGCMDriver::SendImpl(
+    const std::string& app_id,
+    const std::string& receiver_id,
+    const OutgoingMessage& message) {
+  if (service_->is_offline_)
+    return;  // Drop request.
+
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(&CustomFakeGCMDriver::DoSend, weak_factory_.GetWeakPtr(),
+                 app_id, receiver_id, message));
+}
+
+void FakeGCMProfileService::CustomFakeGCMDriver::DoSend(
+    const std::string& app_id,
+    const std::string& receiver_id,
+    const OutgoingMessage& message) {
+  if (service_->collect_) {
+    service_->last_sent_message_ = message;
+    service_->last_receiver_id_ = receiver_id;
+  }
+  SendFinished(app_id, message.id, GCMClient::SUCCESS);
+}
+
+void FakeGCMProfileService::CustomFakeGCMDriver::GetToken(
+    const std::string& app_id,
+    const std::string& authorized_entity,
+    const std::string& scope,
+    const std::map<std::string, std::string>& options,
+    const GetTokenCallback& callback) {
+  if (service_->is_offline_)
+    return;  // Drop request.
+
+  instance_id::FakeGCMDriverForInstanceID::GetToken(app_id, authorized_entity,
+                                                    scope, options, callback);
+}
+
+void FakeGCMProfileService::CustomFakeGCMDriver::DeleteToken(
+    const std::string& app_id,
+    const std::string& authorized_entity,
+    const std::string& scope,
+    const DeleteTokenCallback& callback) {
+  if (service_->is_offline_)
+    return;  // Drop request.
+
+  instance_id::FakeGCMDriverForInstanceID::DeleteToken(
+      app_id, authorized_entity, scope, callback);
+}
+
+void FakeGCMProfileService::CustomFakeGCMDriver::OnDispatchMessage(
+    const std::string& app_id,
+    const IncomingMessage& message) {
   DispatchMessage(app_id, message);
 }
 
-}  // namespace
-
 // static
 std::unique_ptr<KeyedService> FakeGCMProfileService::Build(
     content::BrowserContext* context) {
@@ -127,73 +206,15 @@
   return std::move(service);
 }
 
-FakeGCMProfileService::FakeGCMProfileService(Profile* profile)
-    : collect_(false),
-      registration_count_(0) {
-}
+FakeGCMProfileService::FakeGCMProfileService(Profile* profile) {}
 
 FakeGCMProfileService::~FakeGCMProfileService() {}
 
-void FakeGCMProfileService::RegisterFinished(
-    const std::string& app_id,
-    const std::vector<std::string>& sender_ids) {
-  if (collect_) {
-    last_registered_app_id_ = app_id;
-    last_registered_sender_ids_ = sender_ids;
-  }
-
-  // Generate fake registration IDs, encoding the number of sender IDs (used by
-  // GcmApiTest.RegisterValidation), then an incrementing count (even for the
-  // same app_id - there's no caching) so tests can distinguish registrations.
-  std::string registration_id = base::StringPrintf("%" PRIuS "-%d",
-                                                   sender_ids.size(),
-                                                   registration_count_);
-  ++registration_count_;
-
-  CustomFakeGCMDriver* custom_driver =
-      static_cast<CustomFakeGCMDriver*>(driver());
-  custom_driver->OnRegisterFinished(
-      app_id, registration_id, GCMClient::SUCCESS);
-}
-
-void FakeGCMProfileService::UnregisterFinished(const std::string& app_id) {
-  GCMClient::Result result = GCMClient::SUCCESS;
-  if (!unregister_responses_.empty()) {
-    result = unregister_responses_.front();
-    unregister_responses_.pop_front();
-  }
-
-  CustomFakeGCMDriver* custom_driver =
-      static_cast<CustomFakeGCMDriver*>(driver());
-  custom_driver->OnUnregisterFinished(app_id, result);
-
-  if (!unregister_callback_.is_null())
-    unregister_callback_.Run(app_id);
-}
-
-void FakeGCMProfileService::SendFinished(const std::string& app_id,
-                                         const std::string& receiver_id,
-                                         const OutgoingMessage& message) {
-  if (collect_) {
-    last_sent_message_ = message;
-    last_receiver_id_ = receiver_id;
-  }
-
-  CustomFakeGCMDriver* custom_driver =
-      static_cast<CustomFakeGCMDriver*>(driver());
-  custom_driver->OnSendFinished(app_id, message.id, GCMClient::SUCCESS);
-}
-
 void FakeGCMProfileService::AddExpectedUnregisterResponse(
     GCMClient::Result result) {
   unregister_responses_.push_back(result);
 }
 
-void FakeGCMProfileService::SetUnregisterCallback(
-    const UnregisterCallback& callback) {
-  unregister_callback_ = callback;
-}
-
 void FakeGCMProfileService::DispatchMessage(const std::string& app_id,
                                             const IncomingMessage& message) {
   CustomFakeGCMDriver* custom_driver =
diff --git a/chrome/browser/gcm/fake_gcm_profile_service.h b/chrome/browser/gcm/fake_gcm_profile_service.h
index 7b4db7b..2a5ab46 100644
--- a/chrome/browser/gcm/fake_gcm_profile_service.h
+++ b/chrome/browser/gcm/fake_gcm_profile_service.h
@@ -10,7 +10,8 @@
 #include <vector>
 
 #include "base/macros.h"
-#include "components/gcm_driver/gcm_driver.h"
+#include "components/gcm_driver/common/gcm_messages.h"
+#include "components/gcm_driver/gcm_client.h"
 #include "components/gcm_driver/gcm_profile_service.h"
 
 class Profile;
@@ -24,8 +25,6 @@
 // Acts as a bridge between GCM API and GCMClient layer for testing purposes.
 class FakeGCMProfileService : public GCMProfileService {
  public:
-  typedef base::Callback<void(const std::string&)> UnregisterCallback;
-
   // Helper function to be used with
   // KeyedService::SetTestingFactory().
   static std::unique_ptr<KeyedService> Build(content::BrowserContext* context);
@@ -33,17 +32,8 @@
   explicit FakeGCMProfileService(Profile* profile);
   ~FakeGCMProfileService() override;
 
-  void RegisterFinished(const std::string& app_id,
-                        const std::vector<std::string>& sender_ids);
-  void UnregisterFinished(const std::string& app_id);
-  void SendFinished(const std::string& app_id,
-                    const std::string& receiver_id,
-                    const OutgoingMessage& message);
-
   void AddExpectedUnregisterResponse(GCMClient::Result result);
 
-  void SetUnregisterCallback(const UnregisterCallback& callback);
-
   void DispatchMessage(const std::string& app_id,
                        const IncomingMessage& message);
 
@@ -63,23 +53,26 @@
     return last_registered_sender_ids_;
   }
 
-  void set_collect(bool collect) {
-    collect_ = collect;
-  }
+  // Set whether the service will collect parameters of the calls for further
+  // verification in tests.
+  void set_collect(bool collect) { collect_ = collect; }
+
+  // Crude offline simulation: requests fail and never run their callbacks (in
+  // reality, callbacks run within GetGCMBackoffPolicy().maximum_backoff_ms).
+  void set_offline(bool is_offline) { is_offline_ = is_offline; }
 
  private:
-  // Indicates whether the service will collect paramters of the calls for
-  // furter verification in tests.
-  bool collect_;
-  // Used to give each registration a unique registration id. Does not decrease
-  // when unregister is called.
-  int registration_count_;
+  class CustomFakeGCMDriver;
+  friend class CustomFakeGCMDriver;
+
+  bool collect_ = false;
+  bool is_offline_ = false;
+
   std::string last_registered_app_id_;
   std::vector<std::string> last_registered_sender_ids_;
   std::list<GCMClient::Result> unregister_responses_;
   OutgoingMessage last_sent_message_;
   std::string last_receiver_id_;
-  UnregisterCallback unregister_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeGCMProfileService);
 };
diff --git a/chrome/browser/push_messaging/push_messaging_browsertest.cc b/chrome/browser/push_messaging/push_messaging_browsertest.cc
index 5112e20..024accd 100644
--- a/chrome/browser/push_messaging/push_messaging_browsertest.cc
+++ b/chrome/browser/push_messaging/push_messaging_browsertest.cc
@@ -569,7 +569,6 @@
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
-  EXPECT_NE(push_service(), GetAppHandler());
 }
 
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
@@ -615,7 +614,6 @@
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
-  EXPECT_NE(push_service(), GetAppHandler());
 
   // After unsubscribing, subscribe again from the worker with no key.
   // The sender id should again be read from the datastore, so the
@@ -628,7 +626,6 @@
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
-  EXPECT_NE(push_service(), GetAppHandler());
 }
 
 IN_PROC_BROWSER_TEST_F(
@@ -667,7 +664,6 @@
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
-  EXPECT_NE(push_service(), GetAppHandler());
 
   // After unsubscribing, try to resubscribe again without a key.
   // This should again fail.
@@ -770,7 +766,6 @@
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
-  EXPECT_NE(push_service(), GetAppHandler());
 
   // After unsubscribing, subscribe again from the worker with no key.
   // The sender id should again be read from the datastore, so the
@@ -783,7 +778,6 @@
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
-  EXPECT_NE(push_service(), GetAppHandler());
 }
 
 IN_PROC_BROWSER_TEST_F(
@@ -829,7 +823,6 @@
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
-  EXPECT_NE(push_service(), GetAppHandler());
 
   // After unsubscribing, subscribe again from the worker with no key.
   // The sender id should again be read from the datastore, so the
@@ -842,7 +835,6 @@
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
-  EXPECT_NE(push_service(), GetAppHandler());
 }
 
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, ResubscribeWithMismatchedKey) {
@@ -885,7 +877,6 @@
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
-  EXPECT_NE(push_service(), GetAppHandler());
 
   // Resubscribe with a different key after unsubscribing.
   // Should succeed, and we should get a new subscription token.
@@ -898,7 +889,6 @@
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
-  EXPECT_NE(push_service(), GetAppHandler());
 }
 
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, SubscribePersisted) {
@@ -983,10 +973,17 @@
   ASSERT_NO_FATAL_FAILURE(RestartPushService());
   EXPECT_EQ(push_service(), GetAppHandler());
 
-  // Unsubscribe.
   std::string script_result;
+
+  // Unsubscribe.
+  base::RunLoop run_loop;
+  push_service()->SetUnsubscribeCallbackForTesting(run_loop.QuitClosure());
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
+  // The app handler is only guaranteed to be unregistered once the unsubscribe
+  // callback for testing has been run (PushSubscription.unsubscribe() usually
+  // resolves before that, in order to avoid blocking on network retries etc).
+  run_loop.Run();
 
   EXPECT_NE(push_service(), GetAppHandler());
   ASSERT_NO_FATAL_FAILURE(RestartPushService());
@@ -1695,6 +1692,33 @@
   EXPECT_EQ("unsubscribe result: false", script_result);
 }
 
+IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, UnsubscribeOffline) {
+  std::string script_result;
+
+  EXPECT_NE(push_service(), GetAppHandler());
+
+  std::string token;
+  ASSERT_NO_FATAL_FAILURE(SubscribeSuccessfully(true /* use_key */, &token));
+
+  gcm_service_->set_offline(true);
+
+  // Should quickly resolve true after deleting local state (rather than waiting
+  // until unsubscribing over the network exceeds the maximum backoff duration).
+  ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
+  EXPECT_EQ("unsubscribe result: true", script_result);
+  histogram_tester_.ExpectUniqueSample(
+      "PushMessaging.UnregistrationReason",
+      content::PUSH_UNREGISTRATION_REASON_JAVASCRIPT_API, 1);
+
+  // Since the service is offline, the network request to GCM is still being
+  // retried, so the app handler shouldn't have been unregistered yet.
+  EXPECT_EQ(push_service(), GetAppHandler());
+  // But restarting the push service will unregister the app handler, since the
+  // subscription is no longer stored in the PushMessagingAppIdentifier map.
+  ASSERT_NO_FATAL_FAILURE(RestartPushService());
+  EXPECT_NE(push_service(), GetAppHandler());
+}
+
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
                        UnregisteringServiceWorkerUnsubscribes) {
   std::string script_result;
@@ -2130,8 +2154,14 @@
 
   // After dropping the last subscription it is still inactive.
   std::string script_result;
+  base::RunLoop run_loop;
+  push_service()->SetUnsubscribeCallbackForTesting(run_loop.QuitClosure());
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
+  // Background mode is only guaranteed to have updated once the unsubscribe
+  // callback for testing has been run (PushSubscription.unsubscribe() usually
+  // resolves before that, in order to avoid blocking on network retries etc).
+  run_loop.Run();
   ASSERT_FALSE(background_mode_manager->IsBackgroundModeActive());
 }
 
@@ -2161,8 +2191,14 @@
 
   // Dropping the last subscription deactivates background mode.
   std::string script_result;
+  base::RunLoop run_loop;
+  push_service()->SetUnsubscribeCallbackForTesting(run_loop.QuitClosure());
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
+  // Background mode is only guaranteed to have updated once the unsubscribe
+  // callback for testing has been run (PushSubscription.unsubscribe() usually
+  // resolves before that, in order to avoid blocking on network retries etc).
+  run_loop.Run();
   ASSERT_FALSE(background_mode_manager->IsBackgroundModeActive());
 }
 
@@ -2192,8 +2228,14 @@
 
   // After dropping the last subscription background mode is still inactive.
   std::string script_result;
+  base::RunLoop run_loop;
+  push_service()->SetUnsubscribeCallbackForTesting(run_loop.QuitClosure());
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
+  // Background mode is only guaranteed to have updated once the unsubscribe
+  // callback for testing has been run (PushSubscription.unsubscribe() usually
+  // resolves before that, in order to avoid blocking on network retries etc).
+  run_loop.Run();
   ASSERT_FALSE(background_mode_manager->IsBackgroundModeActive());
 }
 #endif  // BUILDFLAG(ENABLE_BACKGROUND) && !defined(OS_CHROMEOS)
diff --git a/chrome/browser/push_messaging/push_messaging_service_impl.cc b/chrome/browser/push_messaging/push_messaging_service_impl.cc
index 1abf01d..fbc0c0e 100644
--- a/chrome/browser/push_messaging/push_messaging_service_impl.cc
+++ b/chrome/browser/push_messaging/push_messaging_service_impl.cc
@@ -90,6 +90,16 @@
                             content::PUSH_UNREGISTRATION_REASON_LAST + 1);
 }
 
+void RecordUnsubscribeGCMResult(gcm::GCMClient::Result result) {
+  UMA_HISTOGRAM_ENUMERATION("PushMessaging.UnregistrationGCMResult", result,
+                            gcm::GCMClient::LAST_RESULT + 1);
+}
+
+void RecordUnsubscribeIIDResult(InstanceID::Result result) {
+  UMA_HISTOGRAM_ENUMERATION("PushMessaging.UnregistrationIIDResult", result,
+                            InstanceID::LAST_RESULT + 1);
+}
+
 blink::WebPushPermissionStatus ToPushPermission(
     blink::mojom::PermissionStatus permission_status) {
   switch (permission_status) {
@@ -104,50 +114,6 @@
   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;
-  }
-  NOTREACHED();
-  return content::PUSH_UNREGISTRATION_STATUS_SERVICE_NOT_AVAILABLE;
-}
-
-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;
-  }
-  NOTREACHED();
-  return content::PUSH_UNREGISTRATION_STATUS_SERVICE_NOT_AVAILABLE;
-}
-
-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());
@@ -761,18 +727,27 @@
   if (was_subscribed)
     app_identifier.DeleteFromPrefs(profile_);
 
+  // Run the unsubscribe callback *before* asking the InstanceIDDriver/GCMDriver
+  // to unsubscribe, since that's a slow process involving network retries, and
+  // by this point enough local state has been deleted that the subscription is
+  // inactive. Note that DeliverMessageCallback automatically unsubscribes if
+  // messages are later received for a subscription that was locally deleted,
+  // so as long as messages keep getting sent to it, the unsubscription should
+  // eventually reach GCM servers even if this particular attempt fails.
+  callback.Run(
+      was_subscribed
+          ? content::PUSH_UNREGISTRATION_STATUS_SUCCESS_UNREGISTERED
+          : content::PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);
+
   if (PushMessagingAppIdentifier::UseInstanceID(app_id)) {
-    GetInstanceIDDriver()->GetInstanceID(app_id)->DeleteID(base::Bind(
-        &PushMessagingServiceImpl::DidDeleteID, weak_factory_.GetWeakPtr(),
-        app_id, was_subscribed, callback));
+    GetInstanceIDDriver()->GetInstanceID(app_id)->DeleteID(
+        base::Bind(&PushMessagingServiceImpl::DidDeleteID,
+                   weak_factory_.GetWeakPtr(), app_id, was_subscribed));
 
   } else {
     auto unregister_callback =
-        base::Bind(&GCMCallbackToUnregisterCallback,
-                   base::Bind(&PushMessagingServiceImpl::DidUnsubscribe,
-                              weak_factory_.GetWeakPtr(),
-                              std::string() /* app_id_when_instance_id */,
-                              was_subscribed, callback));
+        base::Bind(&PushMessagingServiceImpl::DidUnregister,
+                   weak_factory_.GetWeakPtr(), was_subscribed);
 #if defined(OS_ANDROID)
     // On Android the backend is different, and requires the original sender_id.
     // UnsubscribeBecausePermissionRevoked and
@@ -789,38 +764,42 @@
   }
 }
 
+void PushMessagingServiceImpl::DidUnregister(bool was_subscribed,
+                                             gcm::GCMClient::Result result) {
+  RecordUnsubscribeGCMResult(result);
+  DidUnsubscribe(std::string() /* app_id_when_instance_id */, was_subscribed);
+}
+
 void PushMessagingServiceImpl::DidDeleteID(const std::string& app_id,
                                            bool was_subscribed,
-                                           const UnregisterCallback& callback,
                                            InstanceID::Result result) {
+  RecordUnsubscribeIIDResult(result);
   // DidUnsubscribe must be run asynchronously when passing a non-empty
   // |app_id_when_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::DidUnsubscribe,
-                            weak_factory_.GetWeakPtr(), app_id, was_subscribed,
-                            callback, ToUnregisterStatus(result)));
+      FROM_HERE,
+      base::Bind(&PushMessagingServiceImpl::DidUnsubscribe,
+                 weak_factory_.GetWeakPtr(), app_id, was_subscribed));
 }
 
 void PushMessagingServiceImpl::DidUnsubscribe(
     const std::string& app_id_when_instance_id,
-    bool was_subscribed,
-    const UnregisterCallback& callback,
-    content::PushUnregistrationStatus status) {
+    bool was_subscribed) {
   if (!app_id_when_instance_id.empty())
     GetInstanceIDDriver()->RemoveInstanceID(app_id_when_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);
-  }
+
+  if (!unsubscribe_callback_for_testing_.is_null())
+    unsubscribe_callback_for_testing_.Run();
+}
+
+void PushMessagingServiceImpl::SetUnsubscribeCallbackForTesting(
+    const base::Closure& callback) {
+  unsubscribe_callback_for_testing_ = callback;
 }
 
 // DidDeleteServiceWorkerRegistration methods ----------------------------------
diff --git a/chrome/browser/push_messaging/push_messaging_service_impl.h b/chrome/browser/push_messaging/push_messaging_service_impl.h
index b1faa72..50416f9 100644
--- a/chrome/browser/push_messaging/push_messaging_service_impl.h
+++ b/chrome/browser/push_messaging/push_messaging_service_impl.h
@@ -113,6 +113,7 @@
   void OnMenuClick() override;
 
   void SetMessageCallbackForTesting(const base::Closure& callback);
+  void SetUnsubscribeCallbackForTesting(const base::Closure& callback);
   void SetContentSettingChangedCallbackForTesting(
       const base::Closure& callback);
   void SetServiceWorkerUnregisteredCallbackForTesting(
@@ -192,15 +193,12 @@
                                   const std::string& sender_id,
                                   const UnregisterCallback& callback);
 
+  void DidUnregister(bool was_subscribed, gcm::GCMClient::Result result);
   void DidDeleteID(const std::string& app_id,
                    bool was_subscribed,
-                   const UnregisterCallback&,
                    instance_id::InstanceID::Result result);
-
   void DidUnsubscribe(const std::string& app_id_when_instance_id,
-                      bool was_subscribed,
-                      const UnregisterCallback& callback,
-                      content::PushUnregistrationStatus status);
+                      bool was_subscribed);
 
   // OnContentSettingChanged methods -------------------------------------------
 
@@ -252,6 +250,7 @@
   int pending_push_subscription_count_;
 
   base::Closure message_callback_for_testing_;
+  base::Closure unsubscribe_callback_for_testing_;
   base::Closure content_setting_changed_callback_for_testing_;
   base::Closure service_worker_unregistered_callback_for_testing_;
 
diff --git a/components/gcm_driver/gcm_client.h b/components/gcm_driver/gcm_client.h
index c4dd12a..95a56d9d 100644
--- a/components/gcm_driver/gcm_client.h
+++ b/components/gcm_driver/gcm_client.h
@@ -51,6 +51,7 @@
     IMMEDIATE_START
   };
 
+  // Used for UMA. Can add enum values, but never renumber or delete and reuse.
   enum Result {
     // Successful operation.
     SUCCESS,
@@ -68,7 +69,10 @@
     // Exceeded the specified TTL during message sending.
     TTL_EXCEEDED,
     // Other errors.
-    UNKNOWN_ERROR
+    UNKNOWN_ERROR,
+
+    // Used for UMA. Keep LAST_RESULT up to date and sync with histograms.xml.
+    LAST_RESULT = UNKNOWN_ERROR
   };
 
   enum ChromePlatform {
diff --git a/components/gcm_driver/gcm_client_impl.cc b/components/gcm_driver/gcm_client_impl.cc
index a18b585..5420646 100644
--- a/components/gcm_driver/gcm_client_impl.cc
+++ b/components/gcm_driver/gcm_client_impl.cc
@@ -961,14 +961,17 @@
   Result result;
   PendingRegistrationRequests::const_iterator iter =
       pending_registration_requests_.find(registration_info);
-  if (iter == pending_registration_requests_.end())
+  if (iter == pending_registration_requests_.end()) {
     result = UNKNOWN_ERROR;
-  else if (status == RegistrationRequest::INVALID_SENDER)
+  } else if (status == RegistrationRequest::INVALID_SENDER) {
     result = INVALID_PARAMETER;
-  else if (registration_id.empty())
+  } else if (registration_id.empty()) {
+    // All other errors are currently treated as SERVER_ERROR (including
+    // REACHED_MAX_RETRIES due to the device being offline!).
     result = SERVER_ERROR;
-  else
+  } else {
     result = SUCCESS;
+  }
 
   if (result == SUCCESS) {
     // Cache it.
@@ -1114,7 +1117,8 @@
       result = INVALID_PARAMETER;
       break;
     default:
-      // All other errors are treated as SERVER_ERROR.
+      // All other errors are currently treated as SERVER_ERROR (including
+      // REACHED_MAX_RETRIES due to the device being offline!).
       result = SERVER_ERROR;
       break;
   }
diff --git a/components/gcm_driver/instance_id/instance_id.h b/components/gcm_driver/instance_id/instance_id.h
index 52c1adb..ccf16b7 100644
--- a/components/gcm_driver/instance_id/instance_id.h
+++ b/components/gcm_driver/instance_id/instance_id.h
@@ -27,21 +27,26 @@
 // Instance ID is managed by the InstanceIDDriver.
 class InstanceID {
  public:
+  // Used in UMA. Can add enum values, but never renumber or delete and reuse.
   enum Result {
     // Successful operation.
-    SUCCESS,
+    SUCCESS = 0,
     // Invalid parameter.
-    INVALID_PARAMETER,
+    INVALID_PARAMETER = 1,
     // Instance ID is disabled.
-    DISABLED,
+    DISABLED = 2,
     // Previous asynchronous operation is still pending to finish.
-    ASYNC_OPERATION_PENDING,
+    ASYNC_OPERATION_PENDING = 3,
     // Network socket error.
-    NETWORK_ERROR,
+    NETWORK_ERROR = 4,
     // Problem at the server.
-    SERVER_ERROR,
+    SERVER_ERROR = 5,
+    // 6 is omitted, in case we ever merge this enum with GCMClient::Result.
     // Other errors.
-    UNKNOWN_ERROR
+    UNKNOWN_ERROR = 7,
+
+    // Used for UMA. Keep LAST_RESULT up to date and sync with histograms.xml.
+    LAST_RESULT = UNKNOWN_ERROR
   };
 
   // Asynchronous callbacks. Must not synchronously delete |this| (using
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index b5ebd73..fe56fac 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -52762,6 +52762,24 @@
   </summary>
 </histogram>
 
+<histogram name="PushMessaging.UnregistrationGCMResult" enum="GCMClientResult">
+  <owner>johnme@google.com</owner>
+  <summary>
+    When unregistering a legacy non-InstanceID push messaging subscription, this
+    records the result returned by the GCMDriver (note that exceeding the
+    maximum number of retries due to network errors is logged as SERVER_ERROR).
+  </summary>
+</histogram>
+
+<histogram name="PushMessaging.UnregistrationIIDResult" enum="InstanceIDResult">
+  <owner>johnme@google.com</owner>
+  <summary>
+    When unregistering an InstanceID push messaging subscription, this records
+    the result returned from deleting the InstanceID (note that exceeding the
+    maximum number of retries due to network errors is logged as SERVER_ERROR).
+  </summary>
+</histogram>
+
 <histogram name="PushMessaging.UnregistrationReason"
     enum="PushUnregistrationReason">
   <owner>johnme@google.com</owner>
@@ -92811,6 +92829,17 @@
   <int value="6" label="Zero ID or token"/>
 </enum>
 
+<enum name="GCMClientResult" type="int">
+  <int value="0" label="Success"/>
+  <int value="1" label="Invalid parameter"/>
+  <int value="2" label="GCM disabled"/>
+  <int value="3" label="Async operation pending"/>
+  <int value="4" label="Network error"/>
+  <int value="5" label="Server error"/>
+  <int value="6" label="TTL exceeded"/>
+  <int value="7" label="Unknown error"/>
+</enum>
+
 <enum name="GCMConnectionResetReason" type="int">
   <int value="0" label="Login failure"/>
   <int value="1" label="Close command"/>
@@ -94759,6 +94788,18 @@
   <int value="63" label="DELETE_OLD_VERSIONS_TOO_MANY_ATTEMPTS"/>
 </enum>
 
+<enum name="InstanceIDResult" type="int">
+  <int value="0" label="Success"/>
+  <int value="1" label="Invalid parameter"/>
+  <int value="2" label="GCM disabled"/>
+  <int value="3" label="Async operation pending"/>
+  <int value="4" label="Network error"/>
+  <int value="5" label="Server error"/>
+<!-- 6 is omitted, in case we ever merge this enum with GCMClientResult. -->
+
+  <int value="7" label="Unknown error"/>
+</enum>
+
 <enum name="InstantAppsCallSource" type="int">
   <int value="1" label="Loading screen"/>
   <int value="2" label="Notification"/>