Push API: Refactor and fix unsubscribe API

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)

Committed: https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720
Cr-Commit-Position: refs/heads/master@{#422330}

[johnme, 2016-09-29 21:29:59]

johnme@chromium.org changed reviewers:
+ peter@chromium.org

[johnme, 2016-09-29 21:36:59]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 21:37:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 21:57:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 21:57:23]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Peter Beverloo, 2016-09-30 14:26:07]

This is a great patch John, thank you!! LGTM

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_browsertest.cc:806: // that isn't
yet implemented.
nit: refer to 402458?

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_browsertest.cc:874:
base::HistogramTester histogram_tester2;
Any reason to not just increment the expected counts based on the above? I think
that's what we do everywhere else, even if it may mean changing more lines in
the rare cases where this changes.

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1311: //
auto-unsubscribe when a Service Worker is unregistered (but we should).
Here and everywhere else: please refer to the bug instead of saying "but we
should", that's more actionable, and makes it much easier to identify places
that have to be updated when we actually start doing that.

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:119: }
Most compilers will want you to return a default value after the switch(), in
case |result| for whatever reason contains an invalid value anyway.

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:348:
content::PUSH_UNREGISTRATION_REASON_JAVASCRIPT_API;
What about defining a PUSH_UNREGISTRATION_REASON_UNKNOWN (or NONE) enum instead?
Minor overhead in the enum/histogram, but it makes this code *a lot* nicer.

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:731: const
content::PushMessagingService::UnregisterCallback& callback) {
We probably want to have a `using` statement in this file for
PushMessagingService.

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:731: const
content::PushMessagingService::UnregisterCallback& callback) {
Specific to UnregisterCallback, since this is the PushMessagingServiceImpl we
could import the interface's type in the header w/ a

  using UnregisterCallback = PushMessagingService::UnregisterCallback;

I wish that'd happen by default anyway, since the entire
"content::PushMessagingService::" prefix adds no value.

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:733: // There's
nothing more we can do.
// We cannot unsubscribe with the GCM Driver without an |app_id|.

(The reader now has to parse the rest of the call flow to figure out what
possibly could have happened if we had one.)

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.h:200: const
content::PushMessagingService::UnregisterCallback& callback);
q: could we use std::move() for the callback as opposed to pass by ref?

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.h:214: const
std::string& app_id_if_was_instance_id,
nit: if_was -> when?

https://codereview.chromium.org/2387483002/diff/1/chrome/test/data/push_messa...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/2387483002/diff/1/chrome/test/data/push_messa...
chrome/test/data/push_messaging/push_test.js:62: function
swRegistrationReady(reg) {
nit: this definitely deserves some documentation :)

On a tangent, Jake's going to publish an article on Service Worker lifetimes
soon, so that should help people reason about this in general.

https://codereview.chromium.org/2387483002/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/layout_test_push_messaging_service.cc
(right):

https://codereview.chromium.org/2387483002/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_push_messaging_service.cc:140:
is_subscribed_ = false;
Is there any way a layout test can end up in this situation? I don't think it's
very useful to add this logic (even if it were here to mirror the actual Impl)
if it's an impossible case.

Maybe calling PushSubscription.unsubscribe() twice might work? Can we check the
result values of that?

[johnme, 2016-09-30 17:02:10]

Addressed review comments - thanks!

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_browsertest.cc:806: // that isn't
yet implemented.
On 2016/09/30 14:26:06, Peter Beverloo wrote:
> nit: refer to 402458?

Done.

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_browsertest.cc:874:
base::HistogramTester histogram_tester2;
On 2016/09/30 14:26:06, Peter Beverloo wrote:
> Any reason to not just increment the expected counts based on the above? I
think
> that's what we do everywhere else, even if it may mean changing more lines in
> the rare cases where this changes.

Done (but I had to switch to a less strict ExpectBucketCount)

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1311: //
auto-unsubscribe when a Service Worker is unregistered (but we should).
On 2016/09/30 14:26:06, Peter Beverloo wrote:
> Here and everywhere else: please refer to the bug instead of saying "but we
> should", that's more actionable, and makes it much easier to identify places
> that have to be updated when we actually start doing that.

Done.

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:119: }
On 2016/09/30 14:26:06, Peter Beverloo wrote:
> Most compilers will want you to return a default value after the switch(), in
> case |result| for whatever reason contains an invalid value anyway.

No, default is evil! It prevents compilers from warning when you omit enum
values. But yeah, I've added NOTREACHED and a fallback return (CHECK(false)
would be neater, but probably bloats code size more? We need a better abort
macro for such cases...).

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:348:
content::PUSH_UNREGISTRATION_REASON_JAVASCRIPT_API;
On 2016/09/30 14:26:06, Peter Beverloo wrote:
> What about defining a PUSH_UNREGISTRATION_REASON_UNKNOWN (or NONE) enum
instead?
> Minor overhead in the enum/histogram, but it makes this code *a lot* nicer.

Done.

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:731: const
content::PushMessagingService::UnregisterCallback& callback) {
On 2016/09/30 14:26:06, Peter Beverloo wrote:
> We probably want to have a `using` statement in this file for
> PushMessagingService.

I just removed all occurrences of (content::)?PushMessagingService:: except for
one in an anonymous function =D

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:731: const
content::PushMessagingService::UnregisterCallback& callback) {
On 2016/09/30 14:26:06, Peter Beverloo wrote:
> Specific to UnregisterCallback, since this is the PushMessagingServiceImpl we
> could import the interface's type in the header w/ a
> 
>   using UnregisterCallback = PushMessagingService::UnregisterCallback;
> 
> I wish that'd happen by default anyway, since the entire
> "content::PushMessagingService::" prefix adds no value.

I just removed all occurrences of (content::)?PushMessagingService:: except for
one in an anonymous function =D

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.cc:733: // There's
nothing more we can do.
On 2016/09/30 14:26:06, Peter Beverloo wrote:
> // We cannot unsubscribe with the GCM Driver without an |app_id|.
> 
> (The reader now has to parse the rest of the call flow to figure out what
> possibly could have happened if we had one.)

Done (expanded to cover PushMessagingAppIdentifier map).

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.h:200: const
content::PushMessagingService::UnregisterCallback& callback);
On 2016/09/30 14:26:07, Peter Beverloo wrote:
> q: could we use std::move() for the callback as opposed to pass by ref?

Maybe. But I think I'd rather be consistent with the rest of the class for now,
and we can bulk update these later. Otherwise it'll be hard to remember when to
wrap parameters with std::move.

https://codereview.chromium.org/2387483002/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_service_impl.h:214: const
std::string& app_id_if_was_instance_id,
On 2016/09/30 14:26:07, Peter Beverloo wrote:
> nit: if_was -> when?

Done.

https://codereview.chromium.org/2387483002/diff/1/chrome/test/data/push_messa...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/2387483002/diff/1/chrome/test/data/push_messa...
chrome/test/data/push_messaging/push_test.js:62: function
swRegistrationReady(reg) {
On 2016/09/30 14:26:07, Peter Beverloo wrote:
> nit: this definitely deserves some documentation :)
> 
> On a tangent, Jake's going to publish an article on Service Worker lifetimes
> soon, so that should help people reason about this in general.

Done.

https://codereview.chromium.org/2387483002/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/layout_test_push_messaging_service.cc
(right):

https://codereview.chromium.org/2387483002/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_push_messaging_service.cc:140:
is_subscribed_ = false;
On 2016/09/30 14:26:07, Peter Beverloo wrote:
> Is there any way a layout test can end up in this situation? I don't think
it's
> very useful to add this logic (even if it were here to mirror the actual Impl)
> if it's an impossible case.
> 
> Maybe calling PushSubscription.unsubscribe() twice might work? Can we check
the
> result values of that?

Yes, it can. There are layout tests that explicitly test that if you call
unsubscribe twice the 2nd one returns false (due to
PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED). They started failing
when I made PMMF call through to PMSI even if the SW database does not contain a
subscription ID. This is the minimal implementation that makes those tests pass
again.

[johnme, 2016-09-30 17:02:20]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 17:03:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2016-09-30 17:06:23]

johnme@chromium.org changed reviewers:
+ avi@chromium.org, mpearson@chromium.org

[johnme, 2016-09-30 17:06:23]

avi: please review content/public/
mpearson: please review histograms.xml
Thanks!

[Avi (use Gerrit), 2016-09-30 17:19:26]

LGTM!

Nice.

[commit-bot: I haz the power, 2016-09-30 21:57:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 21:57:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mark P, 2016-09-30 22:39:04]

histograms.xml lgtm once you revise per comment #1 and assuming comment #2's
answer is "just a clarification"

--mark

https://codereview.chromium.org/2387483002/diff/20001/content/public/common/p...
File content/public/common/push_messaging_status.h (right):

https://codereview.chromium.org/2387483002/diff/20001/content/public/common/p...
content/public/common/push_messaging_status.h:68: // Push unregistration reason
for reporting in UMA.
Please add standard warning label here (yes, I see you have something similar
below too), something like
These values are written to logs.  New enum values can be added, but existing
enums must never be renumbered or deleted and reused.

https://codereview.chromium.org/2387483002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2387483002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:93827: +  <int value="3" label="Origin
no longer has permission"/>
Is this just a clarification of meaning or is this a change in meaning (i.e.,
change in when this value is recorded)?

[johnme, 2016-09-30 23:10:41]

Added warning comments about renumbering/reusing deleted enum values - thanks :)

https://codereview.chromium.org/2387483002/diff/20001/content/public/common/p...
File content/public/common/push_messaging_status.h (right):

https://codereview.chromium.org/2387483002/diff/20001/content/public/common/p...
content/public/common/push_messaging_status.h:68: // Push unregistration reason
for reporting in UMA.
On 2016/09/30 22:39:03, Mark P wrote:
> Please add standard warning label here (yes, I see you have something similar
> below too), something like
> These values are written to logs.  New enum values can be added, but existing
> enums must never be renumbered or deleted and reused.

Done (added "Enum values can be added, but must never be renumbered or deleted
and reused." to each of the enums in this class, as they're all used for UMA).

https://codereview.chromium.org/2387483002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2387483002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:93827: +  <int value="3" label="Origin
no longer has permission"/>
On 2016/09/30 22:39:04, Mark P wrote:
> Is this just a clarification of meaning or is this a change in meaning (i.e.,
> change in when this value is recorded)?

Just a clarification.

[johnme, 2016-09-30 23:10:50]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-09-30 23:10:51]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
avi@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2387483002/#ps40001
(title: "Added enum reuse comments")

[commit-bot: I haz the power, 2016-09-30 23:11:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 01:07:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-01 01:07:28]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[johnme, 2016-10-01 21:12:24]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2016-10-01 21:12:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2016-10-01 21:16:51]

The CQ bit was unchecked by johnme@chromium.org

[johnme, 2016-10-01 21:19:08]

Description was changed from

==========
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
==========

to

==========
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)
==========

[johnme, 2016-10-01 21:19:19]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2016-10-01 21:19:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 21:25:08]

Description was changed from

==========
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)
==========

to

==========
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)
==========

[commit-bot: I haz the power, 2016-10-01 21:25:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-01 21:26:54]

Description was changed from

==========
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)
==========

to

==========
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)

Committed: https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720
Cr-Commit-Position: refs/heads/master@{#422330}
==========

[commit-bot: I haz the power, 2016-10-01 21:26:56]

Patchset 3 (id:??) landed as
https://crrev.com/92ef94f69eb941d144e31f6cac45c6e3ce8a8720
Cr-Commit-Position: refs/heads/master@{#422330}