From 911bfc30e2a8756ceabf0034dfa187d20aa55f71 Mon Sep 17 00:00:00 2001 From: Azeem Arshad Date: Tue, 12 Mar 2019 23:16:51 +0000 Subject: [PATCH] [AndroidSms] Fix issue with background notifications after migration. This CL fixes issue with background notifications not working after PWA migration. This was caused due to StreamingConnectionEstablisher not clearing state in TearDown. Also fixed related issue with stopping service worker in TearDown. Bug: 939581 Change-Id: Icfda555117fac726b39f34b8a601dc447c9d6ef5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1510655 Reviewed-by: Jeremy Klein Commit-Queue: Azeem Arshad Cr-Original-Commit-Position: refs/heads/master@{#639129}(cherry picked from commit a9aab3a9d68f69e0482bff47a00dfaf07aaeeaee) Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1518849 Reviewed-by: Azeem Arshad Cr-Commit-Position: refs/branch-heads/3729@{#68} Cr-Branched-From: d4a8972e30b604f090aeda5dfff68386ae656267-refs/heads/master@{#638880} --- .../chromeos/android_sms/connection_manager.cc | 2 ++ .../android_sms/connection_manager_unittest.cc | 5 +++++ .../streaming_connection_establisher.cc | 3 ++- .../streaming_connection_establisher_unittest.cc | 14 +++++++++++++- 4 files changed, 22 insertions(+), 2 deletions(-) diff --git a/chrome/browser/chromeos/android_sms/connection_manager.cc b/chrome/browser/chromeos/android_sms/connection_manager.cc index 34b8039cf2f0..2243adbd47c6 100644 --- a/chrome/browser/chromeos/android_sms/connection_manager.cc +++ b/chrome/browser/chromeos/android_sms/connection_manager.cc @@ -138,6 +138,8 @@ void ConnectionManager::UpdateConnectionStatus() { connection_establisher_->TearDownConnection( *enabled_pwa_url_, GetCurrentServiceWorkerContext()); GetCurrentServiceWorkerContext()->RemoveObserver(this); + active_version_id_.reset(); + prev_active_version_id_.reset(); } enabled_pwa_url_ = updated_pwa_url; diff --git a/chrome/browser/chromeos/android_sms/connection_manager_unittest.cc b/chrome/browser/chromeos/android_sms/connection_manager_unittest.cc index 0380ae7dc483..e227c0569576 100644 --- a/chrome/browser/chromeos/android_sms/connection_manager_unittest.cc +++ b/chrome/browser/chromeos/android_sms/connection_manager_unittest.cc @@ -306,6 +306,11 @@ TEST_F(ConnectionManagerTest, FeatureStateChange) { // Verify that enabling feature establishes connection again. SetPwaState(PwaState::kEnabledWithNewUrl); VerifyEstablishConnectionCalls(3u /* expected_count */); + + // Verify that connection is established if the version id changes. + fake_new_service_worker_context()->NotifyObserversOnNoControllees( + kDummyVersionId2, GetAndroidMessagesURL()); + VerifyEstablishConnectionCalls(4u /* expected_count */); } TEST_F(ConnectionManagerTest, AppUrlMigration) { diff --git a/chrome/browser/chromeos/android_sms/streaming_connection_establisher.cc b/chrome/browser/chromeos/android_sms/streaming_connection_establisher.cc index 20f08235f18d..2bffdfe58b77 100644 --- a/chrome/browser/chromeos/android_sms/streaming_connection_establisher.cc +++ b/chrome/browser/chromeos/android_sms/streaming_connection_establisher.cc @@ -45,7 +45,8 @@ void StreamingConnectionEstablisher::EstablishConnection( void StreamingConnectionEstablisher::TearDownConnection( const GURL& url, content::ServiceWorkerContext* service_worker_context) { - service_worker_context->StopAllServiceWorkersForOrigin(url); + service_worker_context->StopAllServiceWorkersForOrigin(url.GetOrigin()); + is_connected_ = false; } void StreamingConnectionEstablisher::SendStartStreamingMessageIfNotConnected( diff --git a/chrome/browser/chromeos/android_sms/streaming_connection_establisher_unittest.cc b/chrome/browser/chromeos/android_sms/streaming_connection_establisher_unittest.cc index 13fbd8e71b4a..d19a7f400a6b 100644 --- a/chrome/browser/chromeos/android_sms/streaming_connection_establisher_unittest.cc +++ b/chrome/browser/chromeos/android_sms/streaming_connection_establisher_unittest.cc @@ -117,7 +117,19 @@ TEST_F(StreamingConnectionEstablisherTest, TearDownConnection) { const auto& stop_calls = fake_service_worker_context.stop_all_service_workers_for_origin_calls(); ASSERT_EQ(1u, stop_calls.size()); - EXPECT_EQ(GetAndroidMessagesURL(), stop_calls[0]); + EXPECT_EQ(GetAndroidMessagesURL().GetOrigin(), stop_calls[0]); + + // Verify that subsequent message message dispatch calls succeed. + auto& message_dispatch_calls = + fake_service_worker_context + .start_service_worker_and_dispatch_long_running_message_calls(); + + connection_establisher.EstablishConnection( + GetAndroidMessagesURL(), + ConnectionEstablisher::ConnectionMode::kStartConnection, + &fake_service_worker_context); + base::RunLoop().RunUntilIdle(); + ASSERT_EQ(1u, message_dispatch_calls.size()); } } // namespace android_sms