Announcement notification: Handle case where Finch is not ready.

Finch parameter is not guaranteed to be ready on first run. This may
introduce unexpected behaviors, like the user see the notification on
second time running Chrome.

Now we always persist the first run timestamp and uses a Finch
specified timestamp from the server to check if first run happens
recently. If so then just skip the notification.

Also address an issue that origin checks for
NotificationPlatformBridgeMac is removed, now we let the empty GURL
pass.

Bug: 1047286,1046457
Change-Id: I59723c7d35230345ca9d77ccc810a07646d34b0b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032346
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737193}

chrome/browser/notifications/notification_platform_bridge_mac.mm

@@ -460,6 +460,16 @@ bool NotificationPlatformBridgeMac::VerifyNotificationData(
     return false;
   }
 
+  // Origin is not actually required but if it's there it should be a valid one.
+  NSString* origin =
+      [response objectForKey:notification_constants::kNotificationOrigin];
+  if (origin && origin.length) {
+    std::string notificationOrigin = base::SysNSStringToUTF8(origin);
+    GURL url(notificationOrigin);
+    if (!url.is_valid())
+      return false;
+  }
+
   return true;
 }
 

chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm

@@ -206,6 +206,21 @@ TEST_F(NotificationPlatformBridgeMacTest,
   EXPECT_FALSE(NotificationPlatformBridgeMac::VerifyNotificationData(response));
 }
 
+TEST_F(NotificationPlatformBridgeMacTest, TestNotificationVerifyOrigin) {
+  NSMutableDictionary* response = BuildDefaultNotificationResponse();
+  [response setValue:@"invalidorigin"
+              forKey:notification_constants::kNotificationOrigin];
+  EXPECT_FALSE(NotificationPlatformBridgeMac::VerifyNotificationData(response));
+
+  // If however the origin is not present the response should be fine.
+  [response removeObjectForKey:notification_constants::kNotificationOrigin];
+  EXPECT_TRUE(NotificationPlatformBridgeMac::VerifyNotificationData(response));
+
+  // Empty origin should be fine.
+  [response setValue:@"" forKey:notification_constants::kNotificationOrigin];
+  EXPECT_TRUE(NotificationPlatformBridgeMac::VerifyNotificationData(response));
+}
+
 TEST_F(NotificationPlatformBridgeMacTest, TestDisplayNoButtons) {
   std::unique_ptr<Notification> notification =
       CreateBanner("Title", "Context", "https://gmail.com", nullptr, nullptr);

chrome/browser/updates/announcement_notification/announcement_notification_service.cc

@@ -9,6 +9,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/clock.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile_attributes_entry.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
@@ -35,22 +36,24 @@ class AnnouncementNotificationServiceImpl
   AnnouncementNotificationServiceImpl(const base::FilePath& profile_path,
                                       bool new_profile,
                                       PrefService* pref_service,
-                                      std::unique_ptr<Delegate> delegate)
+                                      std::unique_ptr<Delegate> delegate,
+                                      base::Clock* clock)
       : profile_path_(profile_path),
         new_profile_(new_profile),
         pref_service_(pref_service),
         delegate_(std::move(delegate)),
+        clock_(clock),
         skip_first_run_(true),
         skip_new_profile_(false),
         skip_display_(false),
         remote_version_(kInvalidVersion),
         require_signout_(false),
         show_one_(false) {
+    DCHECK(delegate_);
+
     if (!IsFeatureEnabled())
       return;
 
-    DCHECK(delegate_);
-
     // By default do nothing in first run.
     skip_first_run_ = base::GetFieldTrialParamByFeatureAsBool(
         kAnnouncementNotification, kSkipFirstRun, true);
@@ -66,6 +69,14 @@ class AnnouncementNotificationServiceImpl
         kAnnouncementNotification, kShowOneAllProfiles, false);
     remote_url_ = base::GetFieldTrialParamValueByFeature(
         kAnnouncementNotification, kAnnouncementUrl);
+
+    bool success = base::Time::FromString(
+        base::GetFieldTrialParamValueByFeature(kAnnouncementNotification,
+                                               kSkipFirstRunAfterTime)
+            .c_str(),
+        &skip_first_run_after_);
+    if (!success)
+      skip_first_run_after_ = base::Time();
   }
 
   ~AnnouncementNotificationServiceImpl() override = default;
@@ -73,6 +84,10 @@ class AnnouncementNotificationServiceImpl
  private:
   // AnnouncementNotificationService implementation.
   void MaybeShowNotification() override {
+    // Finch config may not be delivered on first run. Records the first run
+    // timestamp to check whether we want to skip the notification.
+    RecordFirstRunIfNeeded();
+
     if (!IsFeatureEnabled())
       return;
 
@@ -92,6 +107,13 @@ class AnnouncementNotificationServiceImpl
     }
   }
 
+  void RecordFirstRunIfNeeded() {
+    if (!delegate_->IsFirstRun())
+      return;
+
+    pref_service_->SetTime(kAnnouncementFirstRunTimePrefName, clock_->Now());
+  }
+
   bool IsFeatureEnabled() const {
     return base::FeatureList::IsEnabled(kAnnouncementNotification);
   }
@@ -99,14 +121,9 @@ class AnnouncementNotificationServiceImpl
   bool IsVersionValid(int version) const { return version >= 0; }
 
   void OnNewVersion() {
-    // Skip first run if needed.
-    if (delegate_->IsFirstRun() && skip_first_run_)
+    if (ShouldSkipDueToFirstRun())
       return;
 
-    ShowNotification();
-  }
-
-  void ShowNotification() {
     if (skip_display_)
       return;
 
@@ -122,10 +139,38 @@ class AnnouncementNotificationServiceImpl
     if (show_one_ && notification_shown)
       return;
 
+    ShowNotification();
+  }
+
+  void ShowNotification() {
     notification_shown = true;
     delegate_->ShowNotification();
   }
 
+  // Returns whether the notification should be skipped based on first run
+  // controls defined in Finch.
+  bool ShouldSkipDueToFirstRun() const {
+    // Don't show notification for first run if Finch parameter specified
+    // "skip_first_run" to true.
+    if (delegate_->IsFirstRun() && skip_first_run_)
+      return true;
+
+    // Finch parameters is not guaranteed to receive by Chrome on first run.
+    // Don't show notification if first run happens after the timestamp
+    // specified in Finch parameter "skip_first_run_after_time".
+    base::Time first_run_time =
+        pref_service_->GetTime(kAnnouncementFirstRunTimePrefName);
+    if (!first_run_time.is_null() && !skip_first_run_after_.is_null() &&
+        first_run_time >= skip_first_run_after_) {
+      return true;
+    }
+
+    // Notice that the first run can happen before
+    // kAnnouncementFirstRunTimePrefName was added to the code base . In this
+    // case, we do want to show the announcement.
+    return false;
+  }
+
   bool IsUserSignIn() {
     DCHECK(g_browser_process);
     // No browser process, assume the user is not signed in.
@@ -147,6 +192,7 @@ class AnnouncementNotificationServiceImpl
   bool new_profile_;
   PrefService* pref_service_;
   std::unique_ptr<Delegate> delegate_;
+  base::Clock* clock_;
 
   // Whether to skip first Chrome launch. Parsed from Finch.
   bool skip_first_run_;
@@ -171,6 +217,9 @@ class AnnouncementNotificationServiceImpl
   // URL.
   std::string remote_url_;
 
+  // If first run happens after this time, then notification should not show.
+  base::Time skip_first_run_after_;
+
   base::WeakPtrFactory<AnnouncementNotificationServiceImpl> weak_ptr_factory_{
       this};
 
@@ -185,6 +234,7 @@ void AnnouncementNotificationService::RegisterProfilePrefs(
     PrefRegistrySimple* registry) {
   DCHECK(registry);
   registry->RegisterIntegerPref(kCurrentVersionPrefName, kInvalidVersion);
+  registry->RegisterTimePref(kAnnouncementFirstRunTimePrefName, base::Time());
 }
 
 // static
@@ -192,9 +242,10 @@ AnnouncementNotificationService* AnnouncementNotificationService::Create(
     const base::FilePath& profile_path,
     bool new_profile,
     PrefService* pref_service,
-    std::unique_ptr<Delegate> delegate) {
+    std::unique_ptr<Delegate> delegate,
+    base::Clock* clock) {
   return new AnnouncementNotificationServiceImpl(
-      profile_path, new_profile, pref_service, std::move(delegate));
+      profile_path, new_profile, pref_service, std::move(delegate), clock);
 }
 
 // static

chrome/browser/updates/announcement_notification/announcement_notification_service.h

@@ -15,9 +15,10 @@
 #include "components/keyed_service/core/keyed_service.h"
 #include "url/gurl.h"
 
-namespace {
+namespace base {
+class Clock;
 class FilePath;
-}  // namespace
+}  // namespace base
 
 class PrefRegistrySimple;
 class PrefService;
@@ -29,6 +30,12 @@ extern const base::Feature kAnnouncementNotification;
 // notification on first run.
 constexpr char kSkipFirstRun[] = "skip_first_run";
 
+// The Finch parameter name for a string value that represents a time.
+// If first run happens after this time, notification will not show.
+// The string defined in Finch config should specify UTC time, and will be
+// parsed by base::Time::FromString().
+constexpr char kSkipFirstRunAfterTime[] = "skip_first_run_after_time";
+
 // The Finch parameter name for a boolean value that whether to show
 // notification for new profile.
 constexpr char kSkipNewProfile[] = "skip_new_profile";
@@ -54,6 +61,10 @@ constexpr char kAnnouncementUrl[] = "announcement_url";
 constexpr char kCurrentVersionPrefName[] =
     "announcement_notification_service_current_version";
 
+// Preference name to persist the time of Chrome first run.
+constexpr char kAnnouncementFirstRunTimePrefName[] =
+    "announcement_notification_service_first_run_time";
+
 // Used to show a notification when the version defined in Finch parameter is
 // higher than the last version saved in the preference service.
 class AnnouncementNotificationService : public KeyedService {
@@ -78,7 +89,8 @@ class AnnouncementNotificationService : public KeyedService {
       const base::FilePath& profile_path,
       bool new_profile,
       PrefService* pref_service,
-      std::unique_ptr<Delegate> delegate);
+      std::unique_ptr<Delegate> delegate,
+      base::Clock* clock);
   static GURL GetAnnouncementURL();
 
   AnnouncementNotificationService();

chrome/browser/updates/announcement_notification/announcement_notification_service_factory.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 
+#include "base/time/default_clock.h"
 #include "build/build_config.h"
 #include "chrome/browser/notifications/notification_display_service_factory.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
@@ -54,7 +55,8 @@ KeyedService* AnnouncementNotificationServiceFactory::BuildServiceInstanceFor(
       std::make_unique<AnnouncementNotificationDelegate>(display_service);
 #endif  // OS_ANDROID
   return AnnouncementNotificationService::Create(
-      profile->GetPath(), profile->IsNewProfile(), pref, std::move(delegate));
+      profile->GetPath(), profile->IsNewProfile(), pref, std::move(delegate),
+      base::DefaultClock::GetInstance());
 }
 
 content::BrowserContext*

chrome/browser/updates/announcement_notification/announcement_notification_service_unittest.cc

@@ -11,6 +11,8 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/simple_test_clock.h"
+#include "base/time/time.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
@@ -54,38 +56,34 @@ class AnnouncementNotificationServiceTest : public testing::Test {
 
   MockDelegate* delegate() { return delegate_; }
 
-  int CurrentVersionPref() {
+  base::SimpleTestClock* clock() { return &clock_; }
+
+  int CurrentVersionPref() const {
     return pref_service_->GetInteger(kCurrentVersionPrefName);
   }
 
-  void InitProfile(bool sign_in) {
-    test_profile_manager_.reset(
-        new TestingProfileManager(TestingBrowserProcess::GetGlobal()));
-    ASSERT_TRUE(test_profile_manager_->SetUp());
-    test_profile_ =
-        test_profile_manager_->CreateTestingProfile("dummy@gmail.com");
-    std::string gaia_id = sign_in ? "dummy_gaia_id" : std::string();
-    test_profile_manager_->profile_attributes_storage()->AddProfile(
-        test_profile_manager_->profiles_dir().AppendASCII(kProfileId),
-        base::ASCIIToUTF16("dummy_name"), gaia_id, base::string16(),
-        sign_in /*is_consented_primary_account*/, 0, std::string(),
-        EmptyAccountId());
+  base::Time FirstRunTimePref() const {
+    return pref_service_->GetTime(kAnnouncementFirstRunTimePrefName);
   }
 
-  void Init(bool enable_feature,
-            bool skip_first_run,
-            int version,
-            int current_version) {
-    // Setup Finch config.
-    std::map<std::string, std::string> parameters = {
-        {kSkipFirstRun, skip_first_run ? "true" : "false"},
-        {kVersion, base::NumberToString(version)}};
-    InitProfile(false);
-    Init(parameters, enable_feature, current_version, false);
+  base::Time SetFirstRunTimePref(const char* time_str) {
+    base::Time time;
+    EXPECT_TRUE(base::Time::FromString(time_str, &time));
+    pref_service_->SetTime(kAnnouncementFirstRunTimePrefName, time);
+    return time;
+  }
+
+  base::Time SetNow(const char* now_str) {
+    base::Time now;
+    EXPECT_TRUE(base::Time::FromString(now_str, &now));
+    clock()->SetNow(now);
+    EXPECT_FALSE(now.is_null());
+    return now;
   }
 
   void Init(const std::map<std::string, std::string>& parameters,
             bool enable_feature,
+            bool sign_in,
             int current_version,
             bool new_profile) {
     if (enable_feature) {
@@ -95,6 +93,19 @@ class AnnouncementNotificationServiceTest : public testing::Test {
       scoped_feature_list_.InitAndDisableFeature(kAnnouncementNotification);
     }
 
+    // Setup sign in status.
+    test_profile_manager_.reset(
+        new TestingProfileManager(TestingBrowserProcess::GetGlobal()));
+    ASSERT_TRUE(test_profile_manager_->SetUp());
+    test_profile_ =
+        test_profile_manager_->CreateTestingProfile("dummy@gmail.com");
+    std::string gaia_id = sign_in ? "dummy_gaia_id" : std::string();
+    test_profile_manager_->profile_attributes_storage()->AddProfile(
+        test_profile_manager_->profiles_dir().AppendASCII(kProfileId),
+        base::ASCIIToUTF16("dummy_name"), gaia_id, base::string16(),
+        sign_in /*is_consented_primary_account*/, 0, std::string(),
+        EmptyAccountId());
+
     // Register pref.
     pref_service_ = std::make_unique<TestingPrefServiceSimple>();
     AnnouncementNotificationService::RegisterProfilePrefs(
@@ -107,13 +118,14 @@ class AnnouncementNotificationServiceTest : public testing::Test {
     service_ = base::WrapUnique<AnnouncementNotificationService>(
         AnnouncementNotificationService::Create(
             test_profile_manager_->profiles_dir().AppendASCII(kProfileId),
-            new_profile, pref_service_.get(), std::move(delegate)));
+            new_profile, pref_service_.get(), std::move(delegate), &clock_));
   }
 
  private:
   content::BrowserTaskEnvironment task_environment_;
   TestingProfile* test_profile_;
   std::unique_ptr<TestingProfileManager> test_profile_manager_;
+  base::SimpleTestClock clock_;
   base::test::ScopedFeatureList scoped_feature_list_;
   std::unique_ptr<AnnouncementNotificationService> service_;
   std::unique_ptr<TestingPrefServiceSimple> pref_service_;
@@ -125,8 +137,7 @@ TEST_F(AnnouncementNotificationServiceTest, RequireSignOut) {
   std::map<std::string, std::string> parameters = {
       {kSkipFirstRun, "false"}, {kVersion, "2"}, {kRequireSignout, "true"}};
   // Profile now is signed in.
-  InitProfile(true /*sign_in*/);
-  Init(parameters, true, 1, false);
+  Init(parameters, true, true /*sign_in*/, 1, false);
 
   ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(false));
   EXPECT_CALL(*delegate(), ShowNotification()).Times(0);
@@ -137,9 +148,7 @@ TEST_F(AnnouncementNotificationServiceTest, RequireSignOut) {
 TEST_F(AnnouncementNotificationServiceTest, SkipNewProfile) {
   std::map<std::string, std::string> parameters = {
       {kSkipFirstRun, "false"}, {kVersion, "2"}, {kSkipNewProfile, "true"}};
-  // Profile now is signed in.
-  InitProfile(false);
-  Init(parameters, true, 1, true /*new_profile*/);
+  Init(parameters, true, false, 1, true /*new_profile*/);
 
   ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(false));
   EXPECT_CALL(*delegate(), ShowNotification()).Times(0);
@@ -153,15 +162,90 @@ TEST_F(AnnouncementNotificationServiceTest, RemoteUrl) {
       {kVersion, "4"},
       {kSkipNewProfile, "true"},
       {kAnnouncementUrl, kRemoteUrl}};
-  // Profile now is signed in.
-  InitProfile(false);
-  Init(parameters, true, 1, false);
+  Init(parameters, true, false, 1, false);
   ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(false));
   EXPECT_CALL(*delegate(), ShowNotification());
   service()->MaybeShowNotification();
   EXPECT_EQ(CurrentVersionPref(), 4);
 }
 
+// First run timestamp should be persisted even if the Finch parameter is not
+// received or the feature is disabled.
+TEST_F(AnnouncementNotificationServiceTest, SaveFirstRunTimeOnFirstRun) {
+  base::Time now = SetNow("30 May 2018 12:00:00");
+  std::map<std::string, std::string> parameters;
+  Init(parameters, false /*enable_feature*/, false, -1, true);
+  ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(true));
+  EXPECT_CALL(*delegate(), ShowNotification()).Times(0);
+  service()->MaybeShowNotification();
+  EXPECT_EQ(CurrentVersionPref(), -1);
+  EXPECT_EQ(FirstRunTimePref(), now);
+}
+
+// First run timestamp should not be persisted when not on first run.
+TEST_F(AnnouncementNotificationServiceTest, SaveFirstRunTimeNotFirstRun) {
+  SetNow("30 May 2018 12:00:00");
+  std::map<std::string, std::string> parameters;
+  Init(parameters, false /*enable_feature*/, false, -1, true);
+  ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(false));
+  service()->MaybeShowNotification();
+  EXPECT_EQ(CurrentVersionPref(), -1);
+  EXPECT_EQ(FirstRunTimePref(), base::Time());
+}
+
+// Not to show notification if first run timestamp happens after Finch parameter
+// timestamp.
+TEST_F(AnnouncementNotificationServiceTest, SkipFirstRunAfterTimeNotShow) {
+  SetNow("10 Feb 2020 13:00:00");
+  std::map<std::string, std::string> parameters = {
+      {kSkipFirstRun, "false"},
+      {kVersion, "2"},
+      {kSkipNewProfile, "false"},
+      {kSkipFirstRunAfterTime, "10 Feb 2020 12:15:00"}};
+  Init(parameters, true, false, 1, false);
+  auto first_run_time = SetFirstRunTimePref("10 Feb 2020 12:30:00");
+  ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(false));
+  EXPECT_CALL(*delegate(), ShowNotification()).Times(0);
+  service()->MaybeShowNotification();
+  EXPECT_EQ(CurrentVersionPref(), 2);
+  EXPECT_EQ(FirstRunTimePref(), first_run_time);
+}
+
+// Show notification if first run timestamp happens before Finch parameter
+// timestamp.
+TEST_F(AnnouncementNotificationServiceTest, SkipFirstRunAfterTimeShow) {
+  SetNow("10 Feb 2020 13:00:00");
+  std::map<std::string, std::string> parameters = {
+      {kSkipFirstRun, "false"},
+      {kVersion, "2"},
+      {kSkipNewProfile, "false"},
+      {kSkipFirstRunAfterTime, "10 Feb 2020 12:15:00"}};
+  Init(parameters, true, false, 1, false);
+  SetFirstRunTimePref("10 Feb 2020 12:10:00");
+  ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(false));
+  EXPECT_CALL(*delegate(), ShowNotification());
+  service()->MaybeShowNotification();
+  EXPECT_EQ(CurrentVersionPref(), 2);
+}
+
+// Show notification if there is no first run timestamp but Finch has
+// "skip_first_run_after_time" parameter.
+TEST_F(AnnouncementNotificationServiceTest, SkipFirstRunAfterNoFirstRunPref) {
+  SetNow("10 Feb 2020 13:00:00");
+  std::map<std::string, std::string> parameters = {
+      {kSkipFirstRun, "false"},
+      {kVersion, "2"},
+      {kSkipNewProfile, "false"},
+      {kSkipFirstRunAfterTime, "10 Feb 2020 12:15:00"}};
+  Init(parameters, true, false, 1, false);
+  EXPECT_EQ(FirstRunTimePref(), base::Time());
+  ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(false));
+  EXPECT_CALL(*delegate(), ShowNotification());
+  service()->MaybeShowNotification();
+  EXPECT_EQ(CurrentVersionPref(), 2);
+  EXPECT_EQ(FirstRunTimePref(), base::Time());
+}
+
 struct VersionTestParam {
   bool enable_feature;
   bool skip_first_run;
@@ -205,13 +289,19 @@ const VersionTestParam kVersionTestParams[] = {
 
 TEST_P(AnnouncementNotificationServiceVersionTest, VersionTest) {
   const auto& param = GetParam();
-  Init(param.enable_feature, param.skip_first_run, param.version,
-       param.current_version);
+  auto now = SetNow("10 Feb 2020 13:00:00");
+  std::map<std::string, std::string> parameters = {
+      {kSkipFirstRun, param.skip_first_run ? "true" : "false"},
+      {kVersion, base::NumberToString(param.version)}};
+  Init(parameters, param.enable_feature, false /*sign_in*/,
+       param.current_version, false);
+
   ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(param.is_first_run));
   EXPECT_CALL(*delegate(), ShowNotification())
       .Times(param.show_notification_called ? 1 : 0);
   service()->MaybeShowNotification();
   EXPECT_EQ(CurrentVersionPref(), param.expected_version_pref);
+  EXPECT_EQ(FirstRunTimePref(), param.is_first_run ? now : base::Time());
 }
 
 INSTANTIATE_TEST_SUITE_P(All,