Push API: Don't wait for network when unsubscribing

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}
Committed: https://chromium.googlesource.com/chromium/src/+/ef71bd0bc3e889890de79958ae65052029d429f1

[johnme, 2017-02-06 18:34:38]

Description was changed from

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

to

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

[johnme, 2017-02-06 18:34:48]

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

[Peter Beverloo, 2017-02-07 18:11:08]

SGTM, no stamp yet due to unused FailAfterMaxBackoff logic

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
File chrome/browser/gcm/fake_gcm_profile_service.cc (right):

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
chrome/browser/gcm/fake_gcm_profile_service.cc:131:
base::Bind(&CustomFakeGCMDriver::DoRegister, base::Unretained(this),
Here and elsewhere - why is it safe to use |this|? Why not use |weak_factory_|?

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
File chrome/browser/gcm/fake_gcm_profile_service.h (right):

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
chrome/browser/gcm/fake_gcm_profile_service.h:13: #include
"components/gcm_driver/gcm_driver.h"
nit: IWYU means we should include the following instead:

components/gcm_driver/common/gcm_messages.h
components/gcm_driver/gcm_client.h

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
chrome/browser/gcm/fake_gcm_profile_service.h:64: friend class
CustomFakeGCMDriver;
nit: since the CustomFakeGCMDriver already stores a pointer to the
FakeGCMProfileService*, can we avoid (a) the nested class and (b) the friend
declaration by just adding a public is_offline()?

Since this entire class is meant for testing, that seems nicer.

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
chrome/browser/gcm/fake_gcm_profile_service.h:70: bool is_offline_ = false;
nit: it's a bit unfortunate that collect_ is documented by the property, but
is_offline_ is documented by the setter. Can we align?

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

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1703:
gcm_service_->set_offline(true);
We're never actually using the FailAfterMaxBackoff() logic added to the fake
profile service.

That's a fair bit of complexity - let's make sure we use it, e.g. by advancing
time to the max backoff and checking for SERVER_ERROR?

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client.h:75: 
micro nit: no newline

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client_impl.cc:972: // FakeGCMProfileService's offline
simulation to match.
Here and later, can we shorten and generalize w/ the following?

// NOTE: Update FakeGCMProfileService's offline simulation when splitting
// out additional |result| values.

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/insta...
File components/gcm_driver/instance_id/instance_id.h (right):

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/insta...
components/gcm_driver/instance_id/instance_id.h:44: // 6 is omitted, in case we
ever merge this enum with GCMClient::Result.
:)

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/insta...
components/gcm_driver/instance_id/instance_id.h:49: 
micro nit^2: no newline

[johnme, 2017-02-08 14:14:37]

Addressed review comments - thanks :)

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
File chrome/browser/gcm/fake_gcm_profile_service.cc (right):

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
chrome/browser/gcm/fake_gcm_profile_service.cc:131:
base::Bind(&CustomFakeGCMDriver::DoRegister, base::Unretained(this),
On 2017/02/07 18:11:08, Peter Beverloo wrote:
> Here and elsewhere - why is it safe to use |this|? Why not use
|weak_factory_|?

Done (the existing code used |this| for immediate PostTask thread hops,
implicitly placing some restrictions on how tests can use this class; I agree
that it'd be safer to consistently use weak pointers though).

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
File chrome/browser/gcm/fake_gcm_profile_service.h (right):

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
chrome/browser/gcm/fake_gcm_profile_service.h:13: #include
"components/gcm_driver/gcm_driver.h"
On 2017/02/07 18:11:08, Peter Beverloo wrote:
> nit: IWYU means we should include the following instead:
> 
> components/gcm_driver/common/gcm_messages.h
> components/gcm_driver/gcm_client.h

Done.

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
chrome/browser/gcm/fake_gcm_profile_service.h:64: friend class
CustomFakeGCMDriver;
On 2017/02/07 18:11:08, Peter Beverloo wrote:
> nit: since the CustomFakeGCMDriver already stores a pointer to the
> FakeGCMProfileService*, can we avoid (a) the nested class and (b) the friend
> declaration by just adding a public is_offline()?
> 
> Since this entire class is meant for testing, that seems nicer.

No, it's not just is_offline_, it's also collect_, last_registered_app_id_,
last_registered_sender_ids_, unregister_responses_, last_sent_message_ and
last_receiver_id_. So, um, every single private variable of
FakeGCMProfileService :) I think that validates this approach of treating the
CustomFakeGCMDriver as part of the internals of FakeGCMProfileService.

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/gcm/fake_gcm...
chrome/browser/gcm/fake_gcm_profile_service.h:70: bool is_offline_ = false;
On 2017/02/07 18:11:08, Peter Beverloo wrote:
> nit: it's a bit unfortunate that collect_ is documented by the property, but
> is_offline_ is documented by the setter. Can we align?

Done (documented both at public setter).

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

https://codereview.chromium.org/2675293003/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1703:
gcm_service_->set_offline(true);
On 2017/02/07 18:11:08, Peter Beverloo wrote:
> We're never actually using the FailAfterMaxBackoff() logic added to the fake
> profile service.
> 
> That's a fair bit of complexity - let's make sure we use it, e.g. by advancing
> time to the max backoff and checking for SERVER_ERROR?

I've simplified the offline sim to just `if (service_->is_offline_) return;`

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client.h:75: 
On 2017/02/07 18:11:08, Peter Beverloo wrote:
> micro nit: no newline

Done.

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_client_impl.cc:972: // FakeGCMProfileService's offline
simulation to match.
On 2017/02/07 18:11:08, Peter Beverloo wrote:
> Here and later, can we shorten and generalize w/ the following?
> 
> // NOTE: Update FakeGCMProfileService's offline simulation when splitting
> // out additional |result| values.

Done (no longer needs to reference FakeGCMProfileService now that the offline
sim is so crude).

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/insta...
File components/gcm_driver/instance_id/instance_id.h (right):

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/insta...
components/gcm_driver/instance_id/instance_id.h:44: // 6 is omitted, in case we
ever merge this enum with GCMClient::Result.
On 2017/02/07 18:11:08, Peter Beverloo wrote:
> :)

Acknowledged.

https://codereview.chromium.org/2675293003/diff/1/components/gcm_driver/insta...
components/gcm_driver/instance_id/instance_id.h:49: 
On 2017/02/07 18:11:08, Peter Beverloo wrote:
> micro nit^2: no newline

Done.

[Peter Beverloo, 2017-02-08 15:35:22]

lgtm

[johnme, 2017-02-08 15:38:06]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-02-08 15:38:10]

The CQ bit was unchecked by johnme@chromium.org

[johnme, 2017-02-08 15:40:32]

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

[johnme, 2017-02-08 15:40:33]

jwd: please approve added histograms - thanks!

[jwd, 2017-02-09 15:50:30]

lgtm

[johnme, 2017-02-09 16:31:06]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2017-02-09 16:31:36]

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, 2017-02-09 17:46:01]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486657866612100,
"parent_rev": "3701cde07f15ce076d9005bac70cb6fe65f465bb", "commit_rev":
"ef71bd0bc3e889890de79958ae65052029d429f1"}

[commit-bot: I haz the power, 2017-02-09 17:46:35]

Description was changed from

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

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/ef71bd0bc3e889890de79958ae65...
==========

[commit-bot: I haz the power, 2017-02-09 17:46:36]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ef71bd0bc3e889890de79958ae65...