push_test js caches document subscription options |
|||
Issue descriptionBecause the pushSubscriptionOptions are global (here: https://cs.chromium.org/chromium/src/chrome/test/data/push_messaging/push_test.js?q=documentsubscribepush&sq=package:chromium&dr=C&l=11) and then mutated in documentSubscribePush(), this means that e.g. if you call documentSubscribePush() removeManifest() documentSubscribePushWithoutKey() then documentSubscribePushWithoutKey() actually subscribes *with* the cached key from where it's set in the first documentSubscribePush() call. I already fixed a similar bug in the service worker code in https://codereview.chromium.org/2449963002 but just noticed the same bug in the push_test.js code. It doesn't affect existing tests (maybe because they always unsubscribe before resubscribing) but needs fixing before I can write tests for https://bugs.chromium.org/p/chromium/issues/detail?id=659230
,
Nov 1 2016
,
Nov 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/59705097c9ca9ee36d441860aaf06e00a70b8bff commit 59705097c9ca9ee36d441860aaf06e00a70b8bff Author: John Mellor <johnme@chromium.org> Date: Mon Nov 21 14:39:40 2016 Fix potential bug in push_test.js - do not cache subscription options - Prior to this patch, the application server key could be inadvertently cached, e.g. if a test in PushMessagingBrowserTest did the following: documentSubscribePush() removeManifest() documentSubscribePushWithoutKey() then the second push would actually use the cached key from the first. - No existing tests happen to hit the bug but that's just luck. BUG= 660874 Review-Url: https://codereview.chromium.org/2460223004 Cr-Commit-Position: refs/heads/master@{#428983} (cherry picked from commit 5069448246d980f994a700bc937aba59dc6e9ab6) Rebased to adjust documentSubscribePushBadKey (which had been removed by https://codereview.chromium.org/2411733002 on master). Merge approval: https://crbug.com/659230#c25 Review URL: https://codereview.chromium.org/2517003003 . Cr-Commit-Position: refs/branch-heads/2883@{#628} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/59705097c9ca9ee36d441860aaf06e00a70b8bff/chrome/browser/push_messaging/push_messaging_browsertest.cc [modify] https://crrev.com/59705097c9ca9ee36d441860aaf06e00a70b8bff/chrome/test/data/push_messaging/push_test.js [delete] https://crrev.com/d49d16d7d503c10d4bb12de178bc54d6af1efdce/chrome/test/data/push_messaging/test_no_subscription_options.html |
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Nov 1 2016