New issue
Advanced search Search tips

Issue 465399 link

Starred by 4 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 464787



Sign in to add a comment

Auto-retry push messaging unsubscriptions that failed due to network errors etc

Project Member Reported by joh...@chromium.org, Mar 9 2015

Issue description

Once  issue 464787  is fixed, all codepaths calling PushMessagingServiceImpl::Unregister will expect the unsubscription to always succeed (even if offline).

Specifically, the subscription will be deactivated locally, making getSubscription() resolve null, and preventing future delivery of push messages; but only one attempt will be made to send the unsubscription to GCM (which may of course fail, e.g. if offline).

Instead, Chrome should automatically retry unsubscription - with exponential backoff - until it succeeds, or exceeds some large time limit.

This automatic retry could either be implemented by the PushMessagingServiceImpl, or by the underlying GCMDriver.

Jian: I remember you were working on automatic retry in the GCM API. What's the status on that?
 

Comment 1 by jianli@chromium.org, Mar 13 2015

I think the retry logic should probably live in that platform layer. That is, GCMDriverDesktop should handle it for non-Android and PushMessagingServiceImpl should take care of all Android cases. The reason for this is that GCMDriver does not know how to backoff and retry reasonably for different platforms. For GCMDriverDesktop we will probably want to extend the existing backoff policy instead of re-issuing the unregistration request. For push messaging, you may want to find out how Android GCM handles it and go from there.

Comment 2 by joh...@chromium.org, Mar 16 2015

> GCMDriverDesktop should handle it for non-Android and PushMessagingServiceImpl should take care of all Android cases

That seems odd - PushMessagingServiceImpl and GCMDriverDesktop are very different layers. If I implement retry in PushMessagingServiceImpl, it would seem simplest to have that apply both for desktop and Android; conversely if push relies on GCMDriverDesktop's retry logic, it would make sense to implement retry in GCMDriverAndroid.

To clarify, when I talk about retrying, I also mean that it should keep retrying after the browser/OS restarts; so this isn't just a matter of adjusting the timeout in gcm_backoff_policy.cc.
John, addressing it in PushMessagingServiceImpl only handles the case of web push. How about making unregistration better for all consumers of GCM in Chrome?

GCMDriverDesktop and GCMDriverAndroid would be the two places I would see that happening. It is interesting however, that GMSCore does not handle that for us already. Are you sure about that?

Comment 4 by joh...@chromium.org, Mar 16 2015

> interesting however, that GMSCore does not handle that for us already. Are you sure about that?

Android GMSCore may quickly retry a few times if the network is flaky, but they don't persist unsubscriptions to disk or retry them once the user is no longer offline.

> GCMDriverDesktop and GCMDriverAndroid

Seems reasonable; but why not do this once for both of them?

> For GCMDriverDesktop we will probably want to extend the existing backoff policy instead of re-issuing the unregistration request.

What's wrong with re-issuing the unregistration request?
>> GCMDriverDesktop and GCMDriverAndroid
>Seems reasonable; but why not do this once for both of them?

It's OK to do it once, provided that it covers cases outside of web push.

Is GCMDriverAndroid tracking registrations today the way desktop version is?

Comment 6 by jianli@chromium.org, Mar 16 2015

Sorry, I mean that it might be better to have different logic built into GCMDriverAndroid and GCMDriverDesktop layer since GCM is implemented differently. 

For Android, we rely on GMSCore. You just said that GMSCore may quickly retry a few times (BTW, do you have any doc on this retry behavior?). Do you know whether the app uninstall service will have more retries when the GCM unregistration fails?

We can try to build a common retry logic for both Android and non-Android. But first we have to find out the differences between these two.

Comment 7 by dxie@chromium.org, Mar 25 2015

are we doing anything for m43?  Branch coming up soon.

Comment 8 by dxie@chromium.org, Apr 1 2015

Labels: -M-43 -ReleaseBlock-Beta M-44
punting it to m44.

Comment 9 by joh...@chromium.org, Apr 14 2015

Labels: -Fizz-Push Cr-Blink-PushAPI
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 20 2015

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

commit dce40c39e0d44436283fbc43fa9b708153dd04e6
Author: johnme <johnme@chromium.org>
Date: Mon Apr 20 17:06:20 2015

Refactor net::BackoffEntry to not require subclassing

Before this patch, net::BackoffEntry had a virtual ImplGetTimeNow method
that tests etc would override to change what time is considered "now".

As suggested by rsleevi in https://codereview.chromium.org/1023473003,
this patch removes that method, and instead makes net::BackoffEntry
accept a base::TickClock in the constructor, to allow overriding the
time without subclassing.

(this will allow future changes to net::BackoffEntry without the
fragile base class problem)

Accordingly, I've removed all subclasses of BackoffEntry, and made them
pass TickClocks instead; in most cases this has been a nice
simplification.

BUG=465399
TBR=stevenjb@chromium.org

Review URL: https://codereview.chromium.org/1076853003

Cr-Commit-Position: refs/heads/master@{#325865}

[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/chrome/browser/captive_portal/captive_portal_service.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/chrome/browser/captive_portal/captive_portal_service.h
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/chrome/browser/captive_portal/captive_portal_service_unittest.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/chrome/browser/chromeos/net/network_portal_detector_impl.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/chrome/browser/chromeos/net/network_portal_detector_impl.h
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/chromeos/network/portal_detector/network_portal_detector_strategy.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/chromeos/network/portal_detector/network_portal_detector_strategy.h
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/components/domain_reliability/scheduler.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/components/domain_reliability/util.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/components/domain_reliability/util.h
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/components/password_manager/core/browser/affiliation_fetch_throttler.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/google_apis/gcm/engine/connection_factory_impl_unittest.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/net/base/backoff_entry.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/net/base/backoff_entry.h
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/net/base/backoff_entry_unittest.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/net/url_request/url_request_throttler_simulation_unittest.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/net/url_request/url_request_throttler_test_support.cc
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/net/url_request/url_request_throttler_test_support.h
[modify] http://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6/net/url_request/url_request_throttler_unittest.cc

Labels: -M-44 MovedFrom-44 M-45
[AUTO] Moving all non essential bugs to the next Milestone.  (This decision is based on the labels attached to your ticket.)


Ref: https://sites.google.com/a/chromium.org/dev/developers/ticket-milestone-punting-1

Comment 13 by peter@chromium.org, Jun 25 2015

Labels: -Pri-1 -M-45 Pri-2 M-46
There's a nasty side-effect of this bug which means that if I send a push notification to the endpoint chrome shows "Site has been updated in the background" even though I'm no longer running a service worker for the site.

Should I open a separate bug to fix that, or is this part of this work?

Comment 15 by peter@chromium.org, Oct 20 2015

#14 - you could follow  Issue 545293  for that
Project Member

Comment 16 by sheriffbot@chromium.org, Jul 13 2016

Labels: Hotlist-OpenBugWithCL
A change has landed for this issue, but it's been open for over 6 months. Please review and close it if applicable. If this issue should remain open, remove the "Hotlist-OpenBugWithCL" label. If no action is taken, it will be archived in 30 days.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-OpenBugWithCL
Removing Hotlist-OpenBugWithCL label as this bug has blocked on issue.

Comment 18 by peter@chromium.org, Dec 22 2017

Owner: peter@chromium.org

Sign in to add a comment