Handle push resubscribes with no sender info (avoids DCHECK)

chrome/browser/push_messaging/push_messaging_browsertest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <map>
 #include <string>
 
@@ skipping 169 matching lines @@
 
     InProcessBrowserTest::TearDown();
   }
 
   void LoadTestPage(const std::string& path) {
     ui_test_utils::NavigateToURL(GetBrowser(), https_server_->GetURL(path));
   }
 
   void LoadTestPage() { LoadTestPage(GetTestURL()); }
 
+  void LoadTestPageWithoutManifest() { LoadTestPage(GetNoManifestTestURL()); }
+
   bool RunScript(const std::string& script, std::string* result) {
     return RunScript(script, result, nullptr);
   }
 
   bool RunScript(const std::string& script, std::string* result,
                  content::WebContents* web_contents) {
     if (!web_contents)
       web_contents = GetBrowser()->tab_strip_model()->GetActiveWebContents();
     return content::ExecuteScriptAndExtractString(web_contents->GetMainFrame(),
                                                   script, result);
@@ skipping 64 matching lines @@
 
   void SetSiteEngagementScore(const GURL& url, double score) {
     SiteEngagementService* service =
         SiteEngagementService::Get(GetBrowser()->profile());
     service->ResetScoreForURL(url, score);
   }
 
  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_;
   instance_id::FakeGCMDriverForInstanceID* gcm_driver_;
   base::HistogramTester histogram_tester_;
 
  private:
   std::unique_ptr<net::EmbeddedTestServer> https_server_;
   PushMessagingServiceImpl* push_service_;
 
@@ skipping 233 matching lines @@
 
   ASSERT_TRUE(RunScript("isControlled()", &script_result));
   ASSERT_EQ("true - is controlled", script_result);
 
   // Try to subscribe from a worker without a key. This should fail.
   ASSERT_TRUE(RunScript("workerSubscribePushNoKey()", &script_result));
   EXPECT_EQ(
       "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.
+
+  // 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));
-  std::string token1;
   ASSERT_NO_FATAL_FAILURE(
-      EndpointToToken(script_result, true /* standard_protocol */, &token1));
+      EndpointToToken(script_result, true /* standard_protocol */));
 
   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;
-  ASSERT_NO_FATAL_FAILURE(
-      EndpointToToken(script_result, true /* standard_protocol */, &token2));
-  EXPECT_NE(token1, token2);
-
-  ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
-  EXPECT_EQ("unsubscribe result: true", script_result);
   EXPECT_NE(push_service(), GetAppHandler());
 }
 
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
                        SubscribeWorkerUsingManifest) {

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

I've suggested adding an extra unsubscribe and resubscribe to the tests you
added, then you can kill SubscribeWorkerUsingManifest since it'll be a strict
subset of ResubscribeWithoutKeyAfterSubscribingWithKeyInManifest.

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

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));
@@ skipping 25 matching lines @@
   std::string token2;
   ASSERT_NO_FATAL_FAILURE(
       EndpointToToken(script_result, false /* standard_protocol */, &token2));
   EXPECT_NE(token1, token2);
 
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
   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;
 
   // First, test that Service Worker registration IDs are assigned in order of
   // registering the Service Workers, and the (fake) push subscription ids are
   // assigned in order of push subscription (even when these orders are
   // different).
 
   std::string token1;
   ASSERT_NO_FATAL_FAILURE(SubscribeSuccessfully(true /* use_key */, &token1));
@@ skipping 1250 matching lines @@
   ASSERT_NO_FATAL_FAILURE(SubscribeSuccessfully());
   ASSERT_FALSE(background_mode_manager->IsBackgroundModeActive());
 
   // After dropping the last subscription background mode is still inactive.
   std::string script_result;
   ASSERT_TRUE(RunScript("unsubscribePush()", &script_result));
   EXPECT_EQ("unsubscribe result: true", script_result);
   ASSERT_FALSE(background_mode_manager->IsBackgroundModeActive());
 }
 #endif  // BUILDFLAG(ENABLE_BACKGROUND) && !defined(OS_CHROMEOS)