John, would you mind taking a look at this? It'd be fantastic if adding the call to ClearPushSubscriptionID() is all it takes!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92ef94f69eb941d144e31f6cac45c6e3ce8a8720

commit 92ef94f69eb941d144e31f6cac45c6e3ce8a8720
Author: johnme <johnme@chromium.org>
Date: Sat Oct 01 21:24:48 2016

Push API: Refactor and fix unsubscribe API

Before this patch, 4 high-level codepaths for unsubscribe had evolved:
a) JS-initiated unsubscribe where PushMessagingMessageFilter's
   UnsubscribeHavingGottenIds would skip talking to the
   PushMessagingServiceImpl because it could not find the subscription
   in the Service Worker database.
b) JS-initiated unsubscribe where PushMessagingMessageFilter did talk to
   the PushMessagingServiceImpl, then directly removed the subscription
   from the Service Worker database in PushMessagingMessageFilter's
   Core::DidUnregisterFromService.
c) Automatic unsubscribe after revoked permission where
   PushMessagingServiceImpl::UnsubscribeBecausePermissionRevoked would
   call PushMessagingService::ClearPushSubscriptionID to ask the content
   layer to remove the subscription from the Service Worker database.
d) Automatic unsubscribe after bad incoming messages where
   PushMessagingServiceImpl would never remove the subscription from the
   Service Worker database.

This patch unifies them, such that all unsubscription requests go via
PushMessagingServiceImpl::UnsubscribeInternal, and this method is now
responsible for calling PushMessagingService::ClearPushSubscriptionID
in all cases to remove the subscription from the Service Worker
database.

- Eliminating (d) fixes the PUSH_DELIVERY_STATUS_PERMISSION_DENIED case,
  where previously we would automatically unsubscribe but never remove
  the corresponding subscription from the Service Worker database (this
  situation occurs for users that had been hit by crbug.com/633310).

- Eliminating (a) makes us more robust against any cases where a
  subscription had been removed from the Service Worker database but not
  from the PushMessagingAppIdentifier map (e.g. due to race conditions
  where processes are killed partway through writing state to disk).

- This adds UMA logging for the reason that caused unsubscription. This
  will be useful in tracking down  https://crbug.com/642139 

- This adds tests for each of the reasons that can trigger automatic
  unsubscription. Previously many of these had no coverage.

- This fixes PushMessagingBrowserTest.UnsubscribeSuccess and
  LegacyUnsubscribeSuccess which were failing to test what they intended
  to test (instead of calling unsubscribe on old references to
  unsubscribed PushSubscriptions and PushSubscriptions from unregistered
  Service Workers, they were trying and failing to get a fresh
  reference, and considering that failure to mean that unsubscribe had
  succeeded); and I added a test where the Service Worker is replaced,
  since unregistering a Service Worker isn't actually enough to stop it
  controlling the current page.

- This merges the DidUnsubscribeInstanceID and DidUnsubscribe methods in
  PushMessagingServiceImpl to avoid duplication of logic.

BUG= 646426 , 642139 ,633310
NOTRY=true
(remaining trybot failures are flake)

Review-Url: https://codereview.chromium.org/2387483002
Cr-Commit-Position: refs/heads/master@{#422330}

[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/chrome/browser/push_messaging/push_messaging_browsertest.cc
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/chrome/browser/push_messaging/push_messaging_service_impl.cc
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/chrome/browser/push_messaging/push_messaging_service_impl.h
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/chrome/test/data/push_messaging/push_test.js
[add] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/chrome/test/data/push_messaging/service_worker_with_skipWaiting_claim.js
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.cc
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.h
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/content/browser/push_messaging/push_messaging_message_filter.cc
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/content/browser/push_messaging/push_messaging_message_filter.h
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/content/public/browser/push_messaging_service.cc
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/content/public/browser/push_messaging_service.h
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/content/public/common/push_messaging_status.h
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/content/shell/browser/layout_test/layout_test_push_messaging_service.cc
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/content/shell/browser/layout_test/layout_test_push_messaging_service.h
[modify] https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720/tools/metrics/histograms/histograms.xml

peter@ Do we have any manual repro steps available for this issue to verify it from Chrome-TE end.

Thanks! 

Steps to reproduce the PUSH_DELIVERY_STATUS_PERMISSION_DENIED failure fixed by 92ef94f69eb941d144e31f6cac45c6e3ce8a8720 (other edge cases have no known way to reproduce):

1. Use a build that reproduces issue 633310, e.g. 52.0.2743.82
2. Start from a clean profile (in particular, site permissions for rawgit.com must be in their default state).
3. Launch chrome.
4. Go to https://rawgit.com/johnmellor/d792819c4faa24a190bb7ea0138fba3e/raw/b113b79b554c847ce438852861e2b81587248858/
5. Without asking for permission, the page should appear to subscribe successfully and show a `curl` command. This is issue 633310.
6. Copy the curl command somewhere, but do not run it yet.
7. Close chrome, and make sure it fully exited.
8. Now use a build that contains the fix for issue 633310 but does not yet contain 92ef94f69eb941d144e31f6cac45c6e3ce8a8720. Since the fix for issue 633310 landed in 3 places as 3 separate patches, you have 3 options here:
a) You could either use an m52 branch build that precedes 37145ff4b449471ced08b452a5e110a5375f9a2e
b) or use an m53 branch build that precedes a2dce45b6967880a048b9f099ebd942d3a8fe05e
c) or use an m54 branch build that precedes 4f82b403f397fbad2e44fe287c05956fcb7cf282
9. Make sure you use the same profile directory as you were using with the previous build.
10. Launch chrome.
11. Now run the curl command you copied down.
13. Go to https://rawgit.com/johnmellor/d792819c4faa24a190bb7ea0138fba3e/raw/b113b79b554c847ce438852861e2b81587248858/ again
14. If it immediately shows the same curl command as the one you had copied down, then you have reproduced the failure.

To instead verify the fix, at step 8 use a build that includes 92ef94f69eb941d144e31f6cac45c6e3ce8a8720 above. Then at step 14 it should instead prompt for notifications permission. Now carry on with a few more tests:

15. Accept the permission prompt.
16. Now it should show another curl command. The long ID that follows "registration_ids\":[\"" should be different from the ID in the curl command you copied down earlier.
17. Run this new curl command (on any computer that has curl, e.g. a Linux desktop, doesn't have to be the same computer).
18. You should see a notification (on the computer with the chrome build being tested); (note that it might only last ~10 seconds - that's fine).
18. Open chrome://histograms (on the computer with the chrome build being tested).
19. There should be an entry for PushMessaging.UnregistrationReason with a single sample in bucket number 4 (which corresponds to "Incoming message origin no longer has permission").

Disclaimer: I haven't tested these instructions - good luck!