Handle push resubscribes with no sender info (avoids DCHECK)

chrome/browser/push_messaging/push_messaging_browsertest.cc

Index: chrome/browser/push_messaging/push_messaging_browsertest.cc
diff --git a/chrome/browser/push_messaging/push_messaging_browsertest.cc b/chrome/browser/push_messaging/push_messaging_browsertest.cc
index be24d16c81710dae50c09d718e458cec36df6df2..ecd8ad663d7182acee91e3a5161c3bc28fa3552f 100644
--- a/chrome/browser/push_messaging/push_messaging_browsertest.cc
+++ b/chrome/browser/push_messaging/push_messaging_browsertest.cc
@@ -187,6 +187,8 @@ class PushMessagingBrowserTest : public InProcessBrowserTest {
 
   void LoadTestPage() { LoadTestPage(GetTestURL()); }
 
+  void LoadTestPageWithoutManifest() { LoadTestPage(GetNoManifestTestURL()); }
+
   bool RunScript(const std::string& script, std::string* result) {
     return RunScript(script, result, nullptr);
   }
@@ -271,6 +273,10 @@ class PushMessagingBrowserTest : public InProcessBrowserTest {
  protected:
   virtual std::string GetTestURL() { return "/push_messaging/test.html"; }
 
+  virtual std::string GetNoManifestTestURL() {
+    return "/push_messaging/test_no_manifest.html";
+  }
+
   virtual Browser* GetBrowser() const { return browser(); }
 
   gcm::FakeGCMProfileService* gcm_service_;
@@ -524,24 +530,12 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, SubscribeWorker) {
       "AbortError - Registration failed - missing applicationServerKey, and "
       "gcm_sender_id not found in manifest",
       script_result);
-  // Now run the subscribe from the service worker with a key. This
-  // should succeed, and write the key to the datastore.
-  ASSERT_TRUE(RunScript("workerSubscribePush()", &script_result));
-  std::string token1;
-  ASSERT_NO_FATAL_FAILURE(
-      EndpointToToken(script_result, true /* standard_protocol */, &token1));
-
-  ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
-  EXPECT_EQ("unsubscribe result: true", script_result);
-  EXPECT_NE(push_service(), GetAppHandler());
 
-  // Now run the subscribe from the service worker without a key.
-  // In this case, the key will be read from the datastore.
-  ASSERT_TRUE(RunScript("workerSubscribePushNoKey()", &script_result));
-  std::string token2;
+  // Now run the subscribe from the service worker with a key.

[johnme, 2016/11/04 17:43:36]

Nit: I know you didn't write this sentence, but it sounds like it's referring to
some different service worker that has a key associated. Mind fixing that whilst
you're editing this line?

[awdf, 2016/11/07 16:54:30]

Done.

+  // This should succeed.
+  ASSERT_TRUE(RunScript("workerSubscribePush()", &script_result));
   ASSERT_NO_FATAL_FAILURE(
-      EndpointToToken(script_result, true /* standard_protocol */, &token2));
-  EXPECT_NE(token1, token2);
+      EndpointToToken(script_result, true /* standard_protocol */));
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
@@ -595,6 +589,200 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
   EXPECT_NE(push_service(), GetAppHandler());
 }
 
+IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
+                       ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest) {

[johnme, 2016/11/04 17:43:37]

Nit: These test names could probably be shortened a little (and
WithoutKeyFailure below confusingly sounds like it doesn't fail). How about
ResubscribeAfterDocumentManifest, ResubscribeAfterWorkerNumericASK, etc?

[awdf, 2016/11/07 16:54:30]

Very good point that WithoutKeyFailure can be parsed as "without key-failure"
rather than "..without key. Failure!" I didn't think of that..

Aside: I actually wanted to call them something like
"ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest_shouldFail" but was
trying to fit in with the naming style already in place which doesn't really
give directives or verbs (blah_shouldFail / blahFailsWhen..), instead it seems
to mostly use nouns (blahFailure).

I prefer to err on the side of clarity rather than brevity in test names (since
they never need to be typed out in code), so have stuck with the long names but
swapped the order of 'WithoutKey' and 'Failure'. I still think it *would* be
clearer if it could be 'ResubscribeWithoutKeyAfter...ShouldFail' - wdyt?

[johnme, 2016/11/07 18:11:59]

Since each of these tests is testing three things (that subscribing without key
from document always fails, that subscribing from sw either fails or succeeds,
and that subscribing from sw after unsubscribing does the same thing), I think
it'd be clearer to omit Failure/ShouldFail from the test names, and let people
read the ASSERT/EXPECT statements (and associated comments). But I don't mind
the current names if you still prefer them.

[awdf, 2016/11/07 19:13:22]

Done.

+  std::string script_result;
+
+  ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
+  ASSERT_EQ("ok - service worker registered", script_result);
+
+  ASSERT_NO_FATAL_FAILURE(RequestAndAcceptPermission());
+
+  LoadTestPage();  // Reload to become controlled.
+
+  ASSERT_TRUE(RunScript("isControlled()", &script_result));
+  ASSERT_EQ("true - is controlled", script_result);
+
+  // Run the subscription from the document without a key, this will trigger
+  // the code to read sender id from the manifest and will write it to the
+  // datastore.
+  ASSERT_TRUE(RunScript("documentSubscribePushWithoutKey()", &script_result));
+  std::string token1;
+  ASSERT_NO_FATAL_FAILURE(
+      EndpointToToken(script_result, false /* standard_protocol */, &token1));
+
+  ASSERT_TRUE(RunScript("removeManifest()", &script_result));
+  ASSERT_EQ("manifest removed", script_result);
+
+  // Try to resubscribe from the document without a key or manifest.
+  // This should fail.
+  ASSERT_TRUE(RunScript("documentSubscribePushWithoutKey()", &script_result));
+  EXPECT_EQ(
+      "AbortError - Registration failed - missing applicationServerKey, "
+      "and manifest empty or missing",
+      script_result);
+
+  // Now run the subscribe from the service worker without a key.
+  // In this case, the sender id should be read from the datastore.
+  ASSERT_TRUE(RunScript("workerSubscribePushNoKey()", &script_result));
+  std::string token2;
+  ASSERT_NO_FATAL_FAILURE(
+      EndpointToToken(script_result, false /* standard_protocol */, &token2));
+  EXPECT_EQ(token1, token2);

[johnme, 2016/11/04 17:43:37]

Now unsubscribe then subscribe successfully from a worker without key?

[awdf, 2016/11/07 16:54:30]

Done.

+
+  ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
+  EXPECT_EQ("unsubscribe result: true", script_result);
+  EXPECT_NE(push_service(), GetAppHandler());
+}
+
+IN_PROC_BROWSER_TEST_F(
+    PushMessagingBrowserTest,
+    ResubscribeWithoutKeyFailureAfterSubscribingFromDocumentWithP256Key) {
+  std::string script_result;
+
+  ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
+  ASSERT_EQ("ok - service worker registered", script_result);
+
+  ASSERT_NO_FATAL_FAILURE(RequestAndAcceptPermission());
+
+  LoadTestPageWithoutManifest();  // Reload to become controlled.
+
+  ASSERT_TRUE(RunScript("isControlled()", &script_result));
+  ASSERT_EQ("true - is controlled", script_result);
+
+  // Run the subscription from the document with a key.
+  ASSERT_TRUE(RunScript("documentSubscribePush()", &script_result));
+  ASSERT_NO_FATAL_FAILURE(EndpointToToken(script_result));
+
+  // Try to resubscribe from the document without a key - should fail.
+  ASSERT_TRUE(RunScript("documentSubscribePushWithoutKey()", &script_result));
+  EXPECT_EQ(
+      "AbortError - Registration failed - missing applicationServerKey, "
+      "and manifest empty or missing",
+      script_result);
+
+  // Now try to resubscribe from the service worker without a key.
+  // This should also fail as the original key was not numeric.
+  ASSERT_TRUE(RunScript("workerSubscribePushNoKey()", &script_result));
+  EXPECT_EQ(
+      "AbortError - Registration failed - missing applicationServerKey, "
+      "and gcm_sender_id not found in manifest",
+      script_result);

[johnme, 2016/11/04 17:43:37]

Now unsubscribe then subscribe successfully from a worker without key?

[awdf, 2016/11/07 16:54:30]

I don't think that would be successful actually, didn't we say we'd also reject
this case in the ticket?

https://bugs.chromium.org/p/chromium/issues/detail?id=659230&desc=4#c15

(unless you're implying 'reload with manifest & resubscribe from document then
subscribe from worker' too ?)

Maybe I should explicitly check it fails too though..

[johnme, 2016/11/07 18:11:59]

Sorry yes, I mean unsubscribe then check that it still fails.

[awdf, 2016/11/07 19:13:22]

Done.

+
+  ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
+  EXPECT_EQ("unsubscribe result: true", script_result);
+  EXPECT_NE(push_service(), GetAppHandler());
+}
+
+IN_PROC_BROWSER_TEST_F(
+    PushMessagingBrowserTest,
+    ResubscribeWithoutKeyFailureAfterSubscribingFromWorkerWithP256Key) {
+  std::string script_result;
+
+  ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
+  ASSERT_EQ("ok - service worker registered", script_result);
+
+  ASSERT_NO_FATAL_FAILURE(RequestAndAcceptPermission());
+
+  LoadTestPageWithoutManifest();  // Reload to become controlled.
+
+  ASSERT_TRUE(RunScript("isControlled()", &script_result));
+  ASSERT_EQ("true - is controlled", script_result);
+
+  // Run the subscribe from the service worker with a key.
+  // This should succeed.
+  ASSERT_TRUE(RunScript("workerSubscribePush()", &script_result));
+  ASSERT_NO_FATAL_FAILURE(
+      EndpointToToken(script_result, true /* standard_protocol */));
+
+  // Try to resubscribe from the document without a key - should fail.
+  ASSERT_TRUE(RunScript("documentSubscribePushWithoutKey()", &script_result));
+  EXPECT_EQ(
+      "AbortError - Registration failed - missing applicationServerKey, "
+      "and manifest empty or missing",
+      script_result);
+
+  // Now try to resubscribe from the service worker without a key.
+  // This should also fail as the original key was not numeric.
+  ASSERT_TRUE(RunScript("workerSubscribePushNoKey()", &script_result));
+  EXPECT_EQ(
+      "AbortError - Registration failed - missing applicationServerKey, and "
+      "gcm_sender_id not found in manifest",
+      script_result);

[johnme, 2016/11/04 17:43:36]

Now unsubscribe then subscribe successfully from a worker without key?

[awdf, 2016/11/07 16:54:30]

Same here, shouldn't it fail again?

[johnme, 2016/11/07 18:11:59]

Yup, I also meant unsubscribe then check that it still fails.

[awdf, 2016/11/07 19:13:22]

Done.

+
+  ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
+  EXPECT_EQ("unsubscribe result: true", script_result);
+  EXPECT_NE(push_service(), GetAppHandler());
+}
+
+IN_PROC_BROWSER_TEST_F(
+    PushMessagingBrowserTest,
+    ResubscribeWithoutKeyFailureAfterSubscribingFromDocumentWithNumber) {
+  std::string script_result;
+
+  ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
+  ASSERT_EQ("ok - service worker registered", script_result);
+
+  ASSERT_NO_FATAL_FAILURE(RequestAndAcceptPermission());
+
+  LoadTestPageWithoutManifest();  // Reload to become controlled.
+
+  ASSERT_TRUE(RunScript("isControlled()", &script_result));
+  ASSERT_EQ("true - is controlled", script_result);
+
+  // Run the subscribe from the service worker with a key.
+  // This should succeed.
+  ASSERT_TRUE(
+      RunScript("documentSubscribePushWithNumericKey()", &script_result));
+  ASSERT_NO_FATAL_FAILURE(
+      EndpointToToken(script_result, false /* standard_protocol */));
+
+  // Try to resubscribe from the document without a key - should fail.
+  ASSERT_TRUE(RunScript("documentSubscribePushWithoutKey()", &script_result));
+  EXPECT_EQ(
+      "AbortError - Registration failed - missing applicationServerKey, "
+      "and manifest empty or missing",
+      script_result);

[johnme, 2016/11/04 17:43:37]

Now subscribe successfully from a worker without key, unsubscribe, and subscribe
successfully from a worker without key again?

[awdf, 2016/11/07 16:54:30]

and here..

[johnme, 2016/11/07 18:11:59]

No, these do indeed succeed. And I see that you check that they succeed in patch
set 8's ResubscribeFailureWithoutKeyAfterSubscribingFromDocumentWithNumber -
thanks :)

[awdf, 2016/11/07 19:13:22]

Acknowledged.

+
+  ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
+  EXPECT_EQ("unsubscribe result: true", script_result);
+  EXPECT_NE(push_service(), GetAppHandler());
+}
+
+IN_PROC_BROWSER_TEST_F(
+    PushMessagingBrowserTest,
+    ResubscribeWithoutKeyFailureAfterSubscribingFromWorkerWithNumber) {
+  std::string script_result;
+
+  ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result));
+  ASSERT_EQ("ok - service worker registered", script_result);
+
+  ASSERT_NO_FATAL_FAILURE(RequestAndAcceptPermission());
+
+  LoadTestPageWithoutManifest();  // Reload to become controlled.
+
+  ASSERT_TRUE(RunScript("isControlled()", &script_result));
+  ASSERT_EQ("true - is controlled", script_result);
+
+  // Run the subscribe from the service worker with a key.
+  // This should succeed.
+  ASSERT_TRUE(
+      RunScript("documentSubscribePushWithNumericKey()", &script_result));

[johnme, 2016/11/04 17:43:37]

s/document/worker/, based on test name

[awdf, 2016/11/07 16:54:30]

Done.

+  ASSERT_NO_FATAL_FAILURE(
+      EndpointToToken(script_result, false /* standard_protocol */));
+
+  // Try to resubscribe from the document without a key - should fail.
+  ASSERT_TRUE(RunScript("documentSubscribePushWithoutKey()", &script_result));
+  EXPECT_EQ(
+      "AbortError - Registration failed - missing applicationServerKey, "
+      "and manifest empty or missing",
+      script_result);

[johnme, 2016/11/04 17:43:37]

Now subscribe successfully from a worker without key, unsubscribe, and subscribe
successfully from a worker without key again?

[awdf, 2016/11/07 16:54:30]

..

[johnme, 2016/11/07 18:11:59]

Ditto I see that you check they succeed :)

+
+  ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
+  EXPECT_EQ("unsubscribe result: true", script_result);
+  EXPECT_NE(push_service(), GetAppHandler());
+}
+
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, SubscribePersisted) {
   std::string script_result;