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

chrome/browser/push_messaging/push_messaging_service_impl.cc

Index: chrome/browser/push_messaging/push_messaging_service_impl.cc
diff --git a/chrome/browser/push_messaging/push_messaging_service_impl.cc b/chrome/browser/push_messaging/push_messaging_service_impl.cc
index 6f155ffc20975ae91840b3667ffd9cda2b9edbc8..72bdbdbdd7dbfc5119330506909564669770721d 100644
--- a/chrome/browser/push_messaging/push_messaging_service_impl.cc
+++ b/chrome/browser/push_messaging/push_messaging_service_impl.cc
@@ -649,8 +649,11 @@ void PushMessagingServiceImpl::Unregister(
                  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
@@ -741,6 +744,11 @@ void PushMessagingServiceImpl::UnregisterBecausePermissionRevoked(
   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));