New issue
Advanced search Search tips

Issue 799483 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Validate cached subscriptions once per week

Project Member Reported by peter@chromium.org, Jan 5 2018

Issue description

When an InstanceID subscription is created, we currently cache it indefinitely. This means that we wouldn't be aware of cases where the server has removed a subscription.

Checking every time is going to be too costly. Instead, we should check validity once per week, matching Android, for calls to GetToken().

https://cs.chromium.org/chromium/src/components/gcm_driver/instance_id/instance_id.h?l=88

 

Comment 1 by peter@chromium.org, Jan 8 2018

Internal tracking issue is at b/70985515.

Comment 3 by na...@chromium.org, Feb 2 2018

Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 13 2018

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

commit c3d792e75d9fdf0885a13679b69c616103dca8a1
Author: Mugdha Lakhani <nator@chromium.org>
Date: Tue Feb 13 15:10:13 2018

Configure token invalidation period using Finch

and use it to conditionally skip GCM registration
(if the token is still fresh).
Also add a unit test to verify intended behavior.
Additionally, add a UMA metric to note whether a
GCM registration request was because of a stale
token.

Bug:  799483 
Change-Id: I994ff79d13b82ed845a4aaed97f91ed8674e62e6
Reviewed-on: https://chromium-review.googlesource.com/908488
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Mugdha Lakhani <nator@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536357}
[modify] https://crrev.com/c3d792e75d9fdf0885a13679b69c616103dca8a1/components/gcm_driver/BUILD.gn
[add] https://crrev.com/c3d792e75d9fdf0885a13679b69c616103dca8a1/components/gcm_driver/features.cc
[modify] https://crrev.com/c3d792e75d9fdf0885a13679b69c616103dca8a1/components/gcm_driver/features.h
[modify] https://crrev.com/c3d792e75d9fdf0885a13679b69c616103dca8a1/components/gcm_driver/gcm_client_impl.cc
[modify] https://crrev.com/c3d792e75d9fdf0885a13679b69c616103dca8a1/components/gcm_driver/gcm_client_impl_unittest.cc
[modify] https://crrev.com/c3d792e75d9fdf0885a13679b69c616103dca8a1/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/c3d792e75d9fdf0885a13679b69c616103dca8a1/tools/metrics/histograms/histograms.xml

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 14 2018

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

commit b54dbbb476819288e0d821568c24fbb024da531f
Author: Mugdha Lakhani <nator@chromium.org>
Date: Wed Feb 14 13:11:49 2018

Remove unused constant.

Bug:  799483 
Change-Id: I9411d141492ae3db3f6baf413b31c7daa099b3a0
Reviewed-on: https://chromium-review.googlesource.com/918801
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536702}
[modify] https://crrev.com/b54dbbb476819288e0d821568c24fbb024da531f/components/gcm_driver/features.cc
[modify] https://crrev.com/b54dbbb476819288e0d821568c24fbb024da531f/components/gcm_driver/features.h

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 23 2018

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

commit 1b76cbeaa2fcdcd62857dcd20b0aed223a0a08df
Author: Peter Beverloo <peter@chromium.org>
Date: Fri Feb 23 16:23:02 2018

Correctly deserialize GCM IID registration Ids

By passing |serialized_value| as the first argument to the
std::string::find() operation, we're not using the # sign as the
separator, but rather as the offset at which we start searching for the
needle. This led to tokens failing validation because the expected
registration Id differs, which in turn made features drop recent
subscriptions thinking they were broken.

Bug:  799483 
Change-Id: I9522b5093f89419c89f0f80d70adfa7d0b2f6e57
Reviewed-on: https://chromium-review.googlesource.com/932262
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538795}
[modify] https://crrev.com/1b76cbeaa2fcdcd62857dcd20b0aed223a0a08df/components/gcm_driver/gcm_client_impl_unittest.cc
[modify] https://crrev.com/1b76cbeaa2fcdcd62857dcd20b0aed223a0a08df/components/gcm_driver/registration_info.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Feb 27 2018

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

commit c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4
Author: Peter Beverloo <peter@chromium.org>
Date: Tue Feb 27 17:17:29 2018

Calling PushManager.subscribe() should always hit the GCM Driver

The PushMessagingManager currently maintains its own subscription cache
in the Service Worker database, keeping track of the subscriptions it
thinks are valid. This means that the PushMessagingManager assumes that
the subscription is valid.

There are cases where the underlying push service, in our case Google
Cloud Messaging implemented through our GCM Driver, invalidates a
subscription. We need to find out about that when it happens, so change
the PushMessagingManager to *always* attempt to create a subscription,
upon which we rely on the push service client to return the same
information given the same input.

The GCM Driver has its own cache, so in the vast majority of cases these
calls won't hit the server. They will, however, on occasion.

Bug:  799483 
Change-Id: I715c6f2eb8296b4512b6b7e9e31734ce9f254744
Reviewed-on: https://chromium-review.googlesource.com/932401
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539473}
[modify] https://crrev.com/c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4/chrome/browser/push_messaging/push_messaging_browsertest.cc
[modify] https://crrev.com/c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4/chrome/browser/push_messaging/push_messaging_service_impl.cc
[modify] https://crrev.com/c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4/components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.cc
[modify] https://crrev.com/c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4/components/gcm_driver/registration_info.cc
[modify] https://crrev.com/c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4/content/browser/push_messaging/push_messaging_manager.cc
[modify] https://crrev.com/c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4/content/browser/push_messaging/push_messaging_manager.h
[modify] https://crrev.com/c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4/content/public/browser/push_messaging_service.h
[modify] https://crrev.com/c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4/content/public/common/push_messaging_status.mojom
[modify] https://crrev.com/c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4/content/renderer/push_messaging/push_messaging_client.cc
[modify] https://crrev.com/c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4/content/renderer/push_messaging/push_provider.cc
[modify] https://crrev.com/c894bfbe17eeeb32eadf1ff99c21ca2b93a519a4/tools/metrics/histograms/enums.xml

Comment 8 by na...@chromium.org, May 30 2018

Status: Fixed (was: Started)

Sign in to add a comment