Push API: Prevent DCHECK when clear permissions after clear SW DB

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 <bitset>
 #include <vector>
 
 #include "base/barrier_closure.h"
@@ skipping 631 matching lines @@
   bool was_registered = application_id.IsValid();
   if (was_registered)
     application_id.DeleteFromDisk(profile_);
 
   const auto& unregister_callback =
       base::Bind(&PushMessagingServiceImpl::DidUnregister,
                  weak_factory_.GetWeakPtr(),
                  was_registered, callback);
 #if defined(OS_ANDROID)
   // On Android the backend is different, and requires the original sender_id.
-  GetGCMDriver()->UnregisterWithSenderId(app_id_guid, sender_id,
-                                         unregister_callback);
+  if (sender_id.empty())
+    unregister_callback.Run(GCMClient::INVALID_PARAMETER);

[mlamouri (slow - plz ping), 2015/03/13 16:25:49]

Is there any chance you could handle that case where you've added the comment?
Even me with a diff, I wondered why you made that check here. Someone looking at
the entire file would have no clue.

[johnme, 2015/03/13 17:21:12]

I want to re-use the logic for getting the PMAI, calling DeleteFromDisk, then
calling DecreasePushRegistrationCount. It would be possible to duplicate those
in UnregisterBecausePermissionRevoked, but I'd also have to duplicate the
comments explaining why it's okish to just delete the mapping, which seemed a
bit excessive.

I've added a "// UnregisterBecausePermissionRevoked sometimes calls us with an
empty one." comment here; does that make it clear enough?

[mlamouri (slow - plz ping), 2015/03/13 17:32:13]

It's clearer but I would suggest to not do that. If you want to re-use that
logic later, move that check there later. As of now, that check has no place
here and moving it when it will be needed will have a low cost. Unless you
already have a CL ready with which this check there makes sense?

+  else
+    GetGCMDriver()->UnregisterWithSenderId(app_id_guid, sender_id,
+                                           unregister_callback);
 #else
   GetGCMDriver()->Unregister(app_id_guid, unregister_callback);
 #endif
 }
 
 void PushMessagingServiceImpl::DidUnregister(
     bool was_registered,
     const content::PushMessagingService::UnregisterCallback& callback,
     gcm::GCMClient::Result result) {
   if (was_registered)
@@ skipping 70 matching lines @@
             weak_factory_.GetWeakPtr(), id, barrier_closure));
   }
 }
 
 void PushMessagingServiceImpl::UnregisterBecausePermissionRevoked(
     const PushMessagingApplicationId& id, const base::Closure& closure,
     const std::string& sender_id, bool success, bool not_found) {
   base::Closure barrier_closure = base::BarrierClosure(2, closure);
 
   // Unregister the PushMessagingApplicationId 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 (crbug.com/464328). In that case Unregister will

[mlamouri (slow - plz ping), 2015/03/13 16:25:49]

This CL is fixing the bug you list. If someone care enough, that person will
check git blame.

[johnme, 2015/03/13 17:21:12]

Done.

+  // just delete the application ID to block future messages.
+  // TODO(johnme): Auto-unregister before SW DB is cleared (crbug.com/402458).

[mlamouri (slow - plz ping), 2015/03/13 16:25:49]

https://crbug.com/....

[johnme, 2015/03/13 17:21:12]

Done (does your editor linkify links, but only if they start with http(s) ?).

[mlamouri (slow - plz ping), 2015/03/13 17:32:13]

I have no idea what my editors would do but "crbug.com/xxxx" isn't really an
URL. The scheme is required so it will be better for everyone to have it (easier
if you use a browser that wouldn't autocomplete and easier if your editor has a
grep using http?://).

   Unregister(id.app_id_guid(), sender_id,
              base::Bind(&UnregisterCallbackToClosure, barrier_closure));
 
   // Clear the associated service worker push registration id.
   ClearPushRegistrationID(profile_, id.origin(),
                           id.service_worker_registration_id(), barrier_closure);
 }
 
 void PushMessagingServiceImpl::SetContentSettingChangedCallbackForTesting(
       const base::Closure& callback) {
@@ skipping 17 matching lines @@
       CONTENT_SETTING_ALLOW;
 }
 
 gcm::GCMDriver* PushMessagingServiceImpl::GetGCMDriver() const {
   gcm::GCMProfileService* gcm_profile_service =
       gcm::GCMProfileServiceFactory::GetForProfile(profile_);
   CHECK(gcm_profile_service);
   CHECK(gcm_profile_service->driver());
   return gcm_profile_service->driver();
 }