From 3bd2059381aed28d3ba9410ca733b5faf259a939 Mon Sep 17 00:00:00 2001 From: mlamouri Date: Mon, 1 Dec 2014 13:20:48 -0800 Subject: [PATCH] [Push] Unregister if there is no available SW. When there is no registered service worker, instead of dropping the message, the PushMessagingService will unregister the web app to which the message was delivered. BUG=437817 Review URL: https://codereview.chromium.org/766223002 Cr-Commit-Position: refs/heads/master@{#306254} --- .../services/gcm/fake_gcm_profile_service.cc | 8 +++ .../services/gcm/fake_gcm_profile_service.h | 5 ++ .../services/gcm/push_messaging_browsertest.cc | 77 ++++++++++++++++++++++ .../services/gcm/push_messaging_service_impl.cc | 28 +++++++- .../services/gcm/push_messaging_service_impl.h | 6 ++ chrome/test/data/push_messaging/test.html | 12 ++++ 6 files changed, 135 insertions(+), 1 deletion(-) diff --git a/chrome/browser/services/gcm/fake_gcm_profile_service.cc b/chrome/browser/services/gcm/fake_gcm_profile_service.cc index 8169ded5ad16..b5bdb17f1432 100644 --- a/chrome/browser/services/gcm/fake_gcm_profile_service.cc +++ b/chrome/browser/services/gcm/fake_gcm_profile_service.cc @@ -143,6 +143,9 @@ void FakeGCMProfileService::UnregisterFinished(const std::string& app_id) { CustomFakeGCMDriver* custom_driver = static_cast(driver()); custom_driver->OnUnregisterFinished(app_id, result); + + if (!unregister_callback_.is_null()) + unregister_callback_.Run(app_id); } void FakeGCMProfileService::SendFinished( @@ -164,4 +167,9 @@ void FakeGCMProfileService::AddExpectedUnregisterResponse( unregister_responses_.push_back(result); } +void FakeGCMProfileService::SetUnregisterCallback( + const UnregisterCallback& callback) { + unregister_callback_ = callback; +} + } // namespace gcm diff --git a/chrome/browser/services/gcm/fake_gcm_profile_service.h b/chrome/browser/services/gcm/fake_gcm_profile_service.h index 2326b9922fb7..fe2ffd8cbf6b 100644 --- a/chrome/browser/services/gcm/fake_gcm_profile_service.h +++ b/chrome/browser/services/gcm/fake_gcm_profile_service.h @@ -20,6 +20,8 @@ namespace gcm { // Acts as a bridge between GCM API and GCMClient layer for testing purposes. class FakeGCMProfileService : public GCMProfileService { public: + typedef base::Callback UnregisterCallback; + // Helper function to be used with // KeyedService::SetTestingFactory(). static KeyedService* Build(content::BrowserContext* context); @@ -36,6 +38,8 @@ class FakeGCMProfileService : public GCMProfileService { void AddExpectedUnregisterResponse(GCMClient::Result result); + void SetUnregisterCallback(const UnregisterCallback& callback); + const GCMClient::OutgoingMessage& last_sent_message() const { return last_sent_message_; } @@ -65,6 +69,7 @@ class FakeGCMProfileService : public GCMProfileService { std::list unregister_responses_; GCMClient::OutgoingMessage last_sent_message_; std::string last_receiver_id_; + UnregisterCallback unregister_callback_; DISALLOW_COPY_AND_ASSIGN(FakeGCMProfileService); }; diff --git a/chrome/browser/services/gcm/push_messaging_browsertest.cc b/chrome/browser/services/gcm/push_messaging_browsertest.cc index 9b83d5a44cf7..82fbd2a2ab63 100644 --- a/chrome/browser/services/gcm/push_messaging_browsertest.cc +++ b/chrome/browser/services/gcm/push_messaging_browsertest.cc @@ -72,6 +72,34 @@ class InfoBarResponder : public infobars::InfoBarManager::Observer { bool accept_; bool has_observed_; }; + +// Class to instantiate on the stack that is meant to be used with +// FakeGCMProfileService. The ::Run() method follows the signature of +// FakeGCMProfileService::UnregisterCallback. +class UnregistrationCallback { + public: + UnregistrationCallback() : done_(false) {} + + void Run(const std::string& app_id) { + app_id_ = app_id; + done_ = true; + base::MessageLoop::current()->Quit(); + } + + void WaitUntilSatisfied() { + while (!done_) + content::RunMessageLoop(); + } + + const std::string& app_id() { + return app_id_; + } + + private: + bool done_; + std::string app_id_; +}; + } // namespace class PushMessagingBrowserTest : public InProcessBrowserTest { @@ -237,4 +265,53 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, PushEventSuccess) { EXPECT_EQ("testdata", script_result); } +IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, PushEventNoServiceWorker) { + std::string script_result; + + ASSERT_TRUE(RunScript("registerServiceWorker()", &script_result)); + ASSERT_EQ("ok - service worker registered", script_result); + + InfoBarResponder accepting_responder(browser(), true); + ASSERT_TRUE(RunScript("requestNotificationPermission();", &script_result)); + ASSERT_EQ("permission status - granted", script_result); + + ASSERT_TRUE(RunScript("registerPush()", &script_result)); + EXPECT_EQ(std::string(kPushMessagingEndpoint) + " - 1", script_result); + + PushMessagingApplicationId app_id(https_server()->GetURL(""), 0L); + EXPECT_EQ(app_id.ToString(), gcm_service()->last_registered_app_id()); + EXPECT_EQ("1234567890", gcm_service()->last_registered_sender_ids()[0]); + + ASSERT_TRUE(RunScript("isControlled()", &script_result)); + ASSERT_EQ("false - is not controlled", script_result); + + loadTestPage(); // Reload to become controlled. + + ASSERT_TRUE(RunScript("isControlled()", &script_result)); + ASSERT_EQ("true - is controlled", script_result); + + // Unregister service worker. Sending a message should now fail. + ASSERT_TRUE(RunScript("unregisterServiceWorker()", &script_result)); + ASSERT_EQ("service worker unregistration status: true", script_result); + + // When the push service will receive it next message, given that there is no + // SW available, it should unregister |app_id|. + UnregistrationCallback callback; + gcm_service()->SetUnregisterCallback(base::Bind(&UnregistrationCallback::Run, + base::Unretained(&callback))); + + GCMClient::IncomingMessage message; + GCMClient::MessageData messageData; + messageData.insert(std::pair("data", "testdata")); + message.data = messageData; + push_service()->OnMessage(app_id.ToString(), message); + + callback.WaitUntilSatisfied(); + EXPECT_EQ(app_id.ToString(), callback.app_id()); + + // No push data should have been received. + ASSERT_TRUE(RunScript("pushData.getImmediately()", &script_result)); + EXPECT_EQ("null", script_result); +} + } // namespace gcm diff --git a/chrome/browser/services/gcm/push_messaging_service_impl.cc b/chrome/browser/services/gcm/push_messaging_service_impl.cc index 19981a207036..164f6639d6e9 100644 --- a/chrome/browser/services/gcm/push_messaging_service_impl.cc +++ b/chrome/browser/services/gcm/push_messaging_service_impl.cc @@ -184,6 +184,15 @@ void PushMessagingServiceImpl::DeliverMessageCallback( content::PushDeliveryStatus status) { // TODO(mvanouwerkerk): UMA logging. // TODO(mvanouwerkerk): Is there a way to recover from failure? + switch (status) { + case content::PUSH_DELIVERY_STATUS_SUCCESS: + case content::PUSH_DELIVERY_STATUS_SERVICE_WORKER_ERROR: + case content::PUSH_DELIVERY_STATUS_EVENT_WAITUNTIL_REJECTED: + break; + case content::PUSH_DELIVERY_STATUS_NO_SERVICE_WORKER: + Unregister(application_id); + break; + } } void PushMessagingServiceImpl::OnMessagesDeleted(const std::string& app_id) { @@ -356,6 +365,23 @@ void PushMessagingServiceImpl::DidRequestPermission( register_callback)); } -// TODO(johnme): Unregister should call DecreasePushRegistrationCount. +void PushMessagingServiceImpl::Unregister( + const PushMessagingApplicationId& application_id) { + DCHECK(gcm_profile_service_->driver()); + + gcm_profile_service_->driver()->Unregister( + application_id.ToString(), + base::Bind(&PushMessagingServiceImpl::DidUnregister, + weak_factory_.GetWeakPtr())); +} + +void PushMessagingServiceImpl::DidUnregister(GCMClient::Result result) { + if (result != GCMClient::SUCCESS) { + DVLOG(1) << "GCM unregistration failed."; + return; + } + + DecreasePushRegistrationCount(1); +} } // namespace gcm diff --git a/chrome/browser/services/gcm/push_messaging_service_impl.h b/chrome/browser/services/gcm/push_messaging_service_impl.h index 0b50a9530e1e..d523ad4d2387 100644 --- a/chrome/browser/services/gcm/push_messaging_service_impl.h +++ b/chrome/browser/services/gcm/push_messaging_service_impl.h @@ -90,6 +90,12 @@ class PushMessagingServiceImpl : public content::PushMessagingService, const content::PushMessagingService::RegisterCallback& callback, bool allow); + // TODO(mvanouwerkerk): this will need to be extended and move to the + // PushMessagingService interface later. + void Unregister(const PushMessagingApplicationId& application_id); + + void DidUnregister(GCMClient::Result result); + GCMProfileService* gcm_profile_service_; // It owns us. Profile* profile_; // It owns our owner. diff --git a/chrome/test/data/push_messaging/test.html b/chrome/test/data/push_messaging/test.html index 6ee954e112e7..ce1b2496239b 100644 --- a/chrome/test/data/push_messaging/test.html +++ b/chrome/test/data/push_messaging/test.html @@ -48,6 +48,10 @@ } }; + FutureData.prototype.getImmediately = function() { + sendResultToTest(this.data); + }; + function requestNotificationPermission() { Notification.requestPermission(function(permission) { sendResultToTest('permission status - ' + permission); @@ -61,6 +65,14 @@ }, sendErrorToTest); } + function unregisterServiceWorker() { + navigator.serviceWorker.getRegistration().then(function(swRegistration) { + swRegistration.unregister().then(function(result) { + sendResultToTest('service worker unregistration status: ' + result); + }) + }).catch(sendErrorToTest); + } + function removeManifest() { var element = document.querySelector('link[rel="manifest"]'); if (element) { -- 2.11.4.GIT