Allow resubscribing for push without a sender id without unsubscribing first |
|||||||||||||||
Issue descriptionWhat steps will reproduce the problem? (1) Call PushManager.subscribe() with a sender id (provided in the manifest, for example) (2) Call PushManager.subscribe() again, *without* a sender id What is the expected output? The second subscribe() should succeed if the first one did, just as it does if you unsubscribe between steps 1 & 2 (see PushMessagingBrowserTest.SubscribeWorker - https://cs.chromium.org/chromium/src/chrome/browser/push_messaging/push_messaging_browsertest.cc?dr&sq=package:chromium&rcl=1477394469&l=520 ) What do you see instead? the second subscribe() fails [edit: And we fail the following DCHECK which could lead to undefined behaviour] - [90634:90634:1025/185436:FATAL:gcm_key_store.cc(130)] Check failed: !key_data_.count(app_id) || (!authorized_entity.empty() && !key_data_[app_id].count(authorized_entity) && !key_data_[app_id].count(std::string())). Instance ID tokens cannot share an app_id with a non-InstanceID GCM registration Logging shows authorized_entity (deriving from sender_id) is empty. Because if a registration already exists on the second subscribe, the new sender id is passed without checks within PushMessagingMessasgeFilter.DidCheckForExistingRegistration
,
Oct 25 2016
,
Oct 26 2016
We used to require this because there's no way to fetch the right manifest when subscribing in a Service Worker. Now that we support `applicationServerKey`, developers can provide their authentication information in the subscribe() call. That makes me wonder -- could we restrict this to repeated subscriptions originating from a Service Worker where the cached authentication is a sender ID? (I.e. disallow it from document contexts entirely, and from Service Workers when using a P-256 public key?)
,
Oct 26 2016
Is this an accurate summary of what you're proposing we allow for resubscribes? https://docs.google.com/document/d/11SRHmEenRVob8pXt6gPVEaixCSFQR8vT4MNpqI2G5pY/edit?hl=en (johnme@ & peter@ , you should have edit access)
,
Oct 26 2016
Sorry - wrong link, here's the right one- https://docs.google.com/document/d/1_ghioYLVi4cnEC-RswpkLHJTyTKaLv3IODulCEZ3CFE/edit?usp=sharing
,
Oct 26 2016
I updated the doc to distinguish between the case we allow deliberately (and changing it may break sites) and the cases we would rather disallow, but propose to allow to reduce code complexity.
,
Oct 26 2016
SGTM - thank you for the clear overview!
,
Oct 26 2016
,
Oct 26 2016
This is a new regression which causes a crash on ToT (when following the steps above), so upping priority so hopefully we can fix this before it goes out in a release.
,
Oct 26 2016
,
Oct 26 2016
To clarify, this doesn't actually crash on release builds but does fail a DCHECK (thus crashing in debug) so could cause undefined behaviour. We believe the regression was introduced in M55 with this commit: https://chromium.googlesource.com/chromium/src/+/a504573264e3021119337fdfbebc76c7f72a3510
,
Oct 28 2016
,
Oct 31 2016
,
Nov 2 2016
The bug was originally about the subscribe(), subscribe() case, but do we also want to use the new rules for when you do: 1. subscribe() with sender info 2. unsubscribe() 3. subscribe() with no sender info ? (because that's what my initial patch does but it makes PushMessagingBrowserTest.SubscribeWorker fail, which tests you can subscribe from a SW, unsubscribe, then subscribe with no key).
,
Nov 2 2016
Should be the same rules (except that if you unsubscribe in between we shouldn't enforce the check being added in issue 638924 that requires the provided sender ID, if any, to match the stored one, if any). Why does that make PushMessagingBrowserTest.SubscribeWorker fail?
,
Nov 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816 commit d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816 Author: awdf <awdf@chromium.org> Date: Tue Nov 08 14:01:07 2016 Handle push resubscribes with no sender info (avoids DCHECK) - Prior to this patch you would fail a DCHECK if you called PushManager.subscribe() with a sender id or key and then PushManager.subscribe() without a sender id or key. - This fixes that and only allows resubscribing with no sender info when the original subscribe was made with a gcm_sender_id. Otherwise we throw an AbortError - no key provided. BUG= 659230 Review-Url: https://codereview.chromium.org/2469293002 Cr-Commit-Position: refs/heads/master@{#430601} [modify] https://crrev.com/d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816/chrome/browser/push_messaging/push_messaging_browsertest.cc [modify] https://crrev.com/d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816/chrome/test/data/push_messaging/push_test.js [modify] https://crrev.com/d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816/chrome/test/data/push_messaging/service_worker.js [add] https://crrev.com/d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816/chrome/test/data/push_messaging/test_no_manifest.html [modify] https://crrev.com/d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816/content/browser/push_messaging/push_messaging_message_filter.cc
,
Nov 8 2016
,
Nov 8 2016
requesting merge to M55 - this is a reasonably small patch which fixes a regression that hasn't yet hit stable.
,
Nov 9 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Nov 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bd071b0aaa4d50bb0cd48becbd108fa87802c728 commit bd071b0aaa4d50bb0cd48becbd108fa87802c728 Author: Jen Harkness <harkness@chromium.org> Date: Wed Nov 09 15:36:38 2016 Handle push resubscribes with no sender info (avoids DCHECK) - Prior to this patch you would fail a DCHECK if you called PushManager.subscribe() with a sender id or key and then PushManager.subscribe() without a sender id or key. - This fixes that and only allows resubscribing with no sender info when the original subscribe was made with a gcm_sender_id. Otherwise we throw an AbortError - no key provided. BUG= 659230 Review-Url: https://codereview.chromium.org/2469293002 Cr-Commit-Position: refs/heads/master@{#430601} (cherry picked from commit d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816) Review URL: https://codereview.chromium.org/2487233002 . Cr-Commit-Position: refs/branch-heads/2883@{#505} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/bd071b0aaa4d50bb0cd48becbd108fa87802c728/chrome/browser/push_messaging/push_messaging_browsertest.cc [modify] https://crrev.com/bd071b0aaa4d50bb0cd48becbd108fa87802c728/chrome/test/data/push_messaging/push_test.js [modify] https://crrev.com/bd071b0aaa4d50bb0cd48becbd108fa87802c728/chrome/test/data/push_messaging/service_worker.js [add] https://crrev.com/bd071b0aaa4d50bb0cd48becbd108fa87802c728/chrome/test/data/push_messaging/test_no_manifest.html [modify] https://crrev.com/bd071b0aaa4d50bb0cd48becbd108fa87802c728/content/browser/push_messaging/push_messaging_message_filter.cc
,
Nov 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb7ed316a34b428c478eba9999084f94f6d74263 commit eb7ed316a34b428c478eba9999084f94f6d74263 Author: jennyz <jennyz@chromium.org> Date: Fri Nov 18 07:23:00 2016 Revert of Handle push resubscribes with no sender info (avoids DCHECK) (patchset #2 id:20001 of https://codereview.chromium.org/2487233002/ ) Reason for revert: https://uberchromegw.corp.google.com/i/chromeos.chrome/builders/Linux%20ChromeOS%20Buildspec%20Tests/builds/1007 Original issue's description: > Handle push resubscribes with no sender info (avoids DCHECK) > > - Prior to this patch you would fail a DCHECK if you called > PushManager.subscribe() with a sender id or key and then > PushManager.subscribe() without a sender id or key. > > - This fixes that and only allows resubscribing with no sender info > when the original subscribe was made with a gcm_sender_id. > Otherwise we throw an AbortError - no key provided. > > BUG= 659230 > > Review-Url: https://codereview.chromium.org/2469293002 > Cr-Commit-Position: refs/heads/master@{#430601} > (cherry picked from commit d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816) > > Committed: https://chromium.googlesource.com/chromium/src/+/bd071b0aaa4d50bb0cd48becbd108fa87802c728 TBR=harkness@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 659230 NOTRY=true NOPRESUBMIT=true TBR=harkness@chromium.org Review-Url: https://codereview.chromium.org/2511133003 Cr-Commit-Position: refs/branch-heads/2883@{#608} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/eb7ed316a34b428c478eba9999084f94f6d74263/chrome/browser/push_messaging/push_messaging_browsertest.cc [modify] https://crrev.com/eb7ed316a34b428c478eba9999084f94f6d74263/chrome/test/data/push_messaging/push_test.js [modify] https://crrev.com/eb7ed316a34b428c478eba9999084f94f6d74263/chrome/test/data/push_messaging/service_worker.js [delete] https://crrev.com/a0590b1231fc4f559fea8eefdc96b6d5dcd72e22/chrome/test/data/push_messaging/test_no_manifest.html [modify] https://crrev.com/eb7ed316a34b428c478eba9999084f94f6d74263/content/browser/push_messaging/push_messaging_message_filter.cc
,
Nov 18 2016
The reason those tests failed on the release-branch was because the cherry-picked commit also depends on commits 5069448246d980f994a700bc937aba59dc6e9ab6 and db15f2e4e890234356aaa82befafb0efb917cd20 which fix bugs in the test code, so should have been cherry-picked across too.
,
Nov 18 2016
Requesting approval to re-merge d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816 to m55 since it got reverted for a test failure (see comment 22). As before, it's a reasonably small patch which fixes a regression that hasn't yet hit stable. This will also require merging 5069448246d980f994a700bc937aba59dc6e9ab6 and db15f2e4e890234356aaa82befafb0efb917cd20 in order to fix the test failure, but both of those are tiny test-only patches so are safe.
,
Nov 19 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Nov 21 2016
I've cherry-picked the two dependencies: 1. Cherry-picked db15f2e4e890234356aaa82befafb0efb917cd20 in https://codereview.chromium.org/2513393004 as d49d16d7d503c10d4bb12de178bc54d6af1efdce 2883@{#627} 2. Cherry-picked 5069448246d980f994a700bc937aba59dc6e9ab6 in https://codereview.chromium.org/2517003003 as 59705097c9ca9ee36d441860aaf06e00a70b8bff 2883@{#628}
,
Nov 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d83ec675b64c4adc029fe4b753b617651dc9f16 commit 4d83ec675b64c4adc029fe4b753b617651dc9f16 Author: John Mellor <johnme@chromium.org> Date: Mon Nov 21 15:48:02 2016 Handle push resubscribes with no sender info (avoids DCHECK) - Prior to this patch you would fail a DCHECK if you called PushManager.subscribe() with a sender id or key and then PushManager.subscribe() without a sender id or key. - This fixes that and only allows resubscribing with no sender info when the original subscribe was made with a gcm_sender_id. Otherwise we throw an AbortError - no key provided. BUG= 659230 Review-Url: https://codereview.chromium.org/2469293002 Cr-Commit-Position: refs/heads/master@{#430601} (cherry picked from commit d8e7aa32e6e15b1ce0cde3a6ccf878fb16ffc816) Skipped cherry-picking tests ResubscribeWithoutKeyAfterSubscribingFromDocumentWithNumber and ResubscribeWithoutKeyAfterSubscribingFromWorkerWithNumber and their helper functions documentSubscribePushWithNumericKey and workerSubscribePushWithNumericKey since m55 predates 3f9e6105d28fbb930d90e38c23d07a9af9b53dbc which allowed passing numeric values for applicationServerKey, so these are not applicable in m55. Review URL: https://codereview.chromium.org/2518043002 . Cr-Commit-Position: refs/branch-heads/2883@{#629} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/4d83ec675b64c4adc029fe4b753b617651dc9f16/chrome/browser/push_messaging/push_messaging_browsertest.cc [modify] https://crrev.com/4d83ec675b64c4adc029fe4b753b617651dc9f16/chrome/test/data/push_messaging/push_test.js [add] https://crrev.com/4d83ec675b64c4adc029fe4b753b617651dc9f16/chrome/test/data/push_messaging/test_no_manifest.html [modify] https://crrev.com/4d83ec675b64c4adc029fe4b753b617651dc9f16/content/browser/push_messaging/push_messaging_message_filter.cc
,
Dec 12 2016
|
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by awdf@chromium.org
, Oct 25 2016