[Sync] Add histograms for cookie jar state on Sessions commit

On every SESSIONS entity commit, record whether the cookie jar matches the signed in user. If it doesn't, record also whether the cookie jar was empty or not. BUG=599593 Review-Url: https://codereview.chromium.org/1991973002 Cr-Commit-Position: refs/heads/master@{#394624}
FairyWorld · May 19, 2016 · 41ee49d · 41ee49d
1 parent 606a2ea
commit 41ee49d
Show file tree

Hide file tree

Showing 22 changed files with 108 additions and 48 deletions.
diff --git a/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc b/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/macros.h"
+#include "base/test/histogram_tester.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sessions_helper.h"
@@ -160,6 +161,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, CookieJarMismatch) {
   ASSERT_TRUE(CheckInitialState(0));
 
   // Add a new session to client 0 and wait for it to sync.
+  base::HistogramTester histogram_tester;
   ScopedWindowMap old_windows;
   GURL url = GURL("http://127.0.0.1/bubba");
   ASSERT_TRUE(OpenTabAndGetLocalWindows(0, url, old_windows.GetMutable()));
@@ -172,6 +174,9 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, CookieJarMismatch) {
   sync_pb::ClientToServerMessage message;
   ASSERT_TRUE(GetFakeServer()->GetLastCommitMessage(&message));
   ASSERT_TRUE(message.commit().config_params().cookie_jar_mismatch());
+  histogram_tester.ExpectUniqueSample("Sync.CookieJarMatchOnNavigation", false,
+                                      1);
+  histogram_tester.ExpectUniqueSample("Sync.CookieJarEmptyOnMismatch", true, 1);
 
   // Trigger a cookie jar change (user signing in to content area).
   gaia::ListedAccount signed_in_account;
@@ -191,4 +196,10 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, CookieJarMismatch) {
   // Verify the cookie jar mismatch bool is set to false.
   ASSERT_TRUE(GetFakeServer()->GetLastCommitMessage(&message));
   ASSERT_FALSE(message.commit().config_params().cookie_jar_mismatch());
+
+  // Verify the histograms were recorded properly.
+  histogram_tester.ExpectTotalCount("Sync.CookieJarMatchOnNavigation", 2);
+  histogram_tester.ExpectBucketCount("Sync.CookieJarMatchOnNavigation", true,
+                                     1);
+  histogram_tester.ExpectUniqueSample("Sync.CookieJarEmptyOnMismatch", true, 1);
 }
diff --git a/components/browser_sync/browser/profile_sync_service.cc b/components/browser_sync/browser/profile_sync_service.cc
@@ -2125,19 +2125,21 @@ void ProfileSyncService::OnGaiaAccountsInCookieUpdated(
   if (!IsBackendInitialized())
     return;
 
-  bool cookie_mismatch = true;
+  bool cookie_jar_mismatch = true;
+  bool cookie_jar_empty = accounts.size() == 0;
   std::string account_id = signin_->GetAccountIdToUse();
 
   // Iterate through list of accounts, looking for current sync account.
   for (const auto& account : accounts) {
     if (account.gaia_id == account_id) {
-      cookie_mismatch = false;
+      cookie_jar_mismatch = false;
       break;
     }
   }
 
-  DVLOG(1) << "Cookie jar mismatch: " << cookie_mismatch;
-  backend_->OnCookieJarChanged(cookie_mismatch);
+  DVLOG(1) << "Cookie jar mismatch: " << cookie_jar_mismatch;
+  DVLOG(1) << "Cookie jar empty: " << cookie_jar_empty;
+  backend_->OnCookieJarChanged(cookie_jar_mismatch, cookie_jar_empty);
 }
 
 void ProfileSyncService::AddObserver(

diff --git a/components/sync_driver/glue/sync_backend_host.h b/components/sync_driver/glue/sync_backend_host.h
@@ -231,7 +231,7 @@ class SyncBackendHost : public sync_driver::BackendDataTypeConfigurer {
 
   // Notify the syncer that the cookie jar has changed.
   // See SyncManager::OnCookieJarChanged.
-  virtual void OnCookieJarChanged(bool account_mismatch) = 0;
+  virtual void OnCookieJarChanged(bool account_mismatch, bool empty_jar) = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SyncBackendHost);

diff --git a/components/sync_driver/glue/sync_backend_host_core.cc b/components/sync_driver/glue/sync_backend_host_core.cc
@@ -783,9 +783,10 @@ void SyncBackendHostCore::DoClearServerData(
   sync_manager_->ClearServerData(callback);
 }
 
-void SyncBackendHostCore::DoOnCookieJarChanged(bool account_mismatch) {
+void SyncBackendHostCore::DoOnCookieJarChanged(bool account_mismatch,
+                                               bool empty_jar) {
   DCHECK_EQ(base::MessageLoop::current(), sync_loop_);
-  sync_manager_->OnCookieJarChanged(account_mismatch);
+  sync_manager_->OnCookieJarChanged(account_mismatch, empty_jar);
 }
 
 void SyncBackendHostCore::ClearServerDataDone(

diff --git a/components/sync_driver/glue/sync_backend_host_core.h b/components/sync_driver/glue/sync_backend_host_core.h
@@ -262,7 +262,7 @@ class SyncBackendHostCore
       const syncer::SyncManager::ClearServerDataCallback& frontend_callback);
 
   // Notify the syncer that the cookie jar has changed.
-  void DoOnCookieJarChanged(bool account_mismatch);
+  void DoOnCookieJarChanged(bool account_mismatch, bool empty_jar);
 
  private:
   friend class base::RefCountedThreadSafe<SyncBackendHostCore>;

diff --git a/components/sync_driver/glue/sync_backend_host_impl.cc b/components/sync_driver/glue/sync_backend_host_impl.cc
@@ -856,11 +856,12 @@ void SyncBackendHostImpl::ClearServerData(
                             core_.get(), callback));
 }
 
-void SyncBackendHostImpl::OnCookieJarChanged(bool account_mismatch) {
+void SyncBackendHostImpl::OnCookieJarChanged(bool account_mismatch,
+                                             bool empty_jar) {
   DCHECK(ui_thread_->BelongsToCurrentThread());
   registrar_->sync_thread()->task_runner()->PostTask(
       FROM_HERE, base::Bind(&SyncBackendHostCore::DoOnCookieJarChanged,
-                            core_.get(), account_mismatch));
+                            core_.get(), account_mismatch, empty_jar));
 }
 
 void SyncBackendHostImpl::ClearServerDataDoneOnFrontendLoop(

diff --git a/components/sync_driver/glue/sync_backend_host_impl.h b/components/sync_driver/glue/sync_backend_host_impl.h
@@ -147,7 +147,7 @@ class SyncBackendHostImpl
   void RefreshTypesForTest(syncer::ModelTypeSet types) override;
   void ClearServerData(
       const syncer::SyncManager::ClearServerDataCallback& callback) override;
-  void OnCookieJarChanged(bool account_mismatch) override;
+  void OnCookieJarChanged(bool account_mismatch, bool empty_jar) override;
 
   // InvalidationHandler implementation.
   void OnInvalidatorStateChange(syncer::InvalidatorState state) override;

diff --git a/components/sync_driver/glue/sync_backend_host_mock.cc b/components/sync_driver/glue/sync_backend_host_mock.cc
@@ -154,6 +154,7 @@ void SyncBackendHostMock::ClearServerData(
   callback.Run();
 }
 
-void SyncBackendHostMock::OnCookieJarChanged(bool account_mismatch) {}
+void SyncBackendHostMock::OnCookieJarChanged(bool account_mismatch,
+                                             bool empty_jar) {}
 
 }  // namespace browser_sync
diff --git a/components/sync_driver/glue/sync_backend_host_mock.h b/components/sync_driver/glue/sync_backend_host_mock.h
@@ -121,7 +121,7 @@ class SyncBackendHostMock : public SyncBackendHost {
   void ClearServerData(
       const syncer::SyncManager::ClearServerDataCallback& callback) override;
 
-  void OnCookieJarChanged(bool account_mismatch) override;
+  void OnCookieJarChanged(bool account_mismatch, bool empty_jar) override;
 
   void set_fail_initial_download(bool should_fail);
 

diff --git a/sync/engine/commit.cc b/sync/engine/commit.cc
@@ -32,21 +32,20 @@ Commit::~Commit() {
   DCHECK(cleaned_up_);
 }
 
-Commit* Commit::Init(
-    ModelTypeSet requested_types,
-    ModelTypeSet enabled_types,
-    size_t max_entries,
-    const std::string& account_name,
-    const std::string& cache_guid,
-    bool cookie_jar_mismatch,
-    CommitProcessor* commit_processor,
-    ExtensionsActivity* extensions_activity) {
+Commit* Commit::Init(ModelTypeSet requested_types,
+                     ModelTypeSet enabled_types,
+                     size_t max_entries,
+                     const std::string& account_name,
+                     const std::string& cache_guid,
+                     bool cookie_jar_mismatch,
+                     bool cookie_jar_empty,
+                     CommitProcessor* commit_processor,
+                     ExtensionsActivity* extensions_activity) {
   // Gather per-type contributions.
   ContributionMap contributions;
-  commit_processor->GatherCommitContributions(
-      requested_types,
-      max_entries,
-      &contributions);
+  commit_processor->GatherCommitContributions(requested_types, max_entries,
+                                              cookie_jar_mismatch,
+                                              cookie_jar_empty, &contributions);
 
   // Give up if no one had anything to commit.
   if (contributions.empty())

diff --git a/sync/engine/commit.h b/sync/engine/commit.h
@@ -51,15 +51,15 @@ class SYNC_EXPORT Commit {
   // This destructor will DCHECK if CleanUp() has not been called.
   ~Commit();
 
-  static Commit* Init(
-      ModelTypeSet requested_types,
-      ModelTypeSet enabled_types,
-      size_t max_entries,
-      const std::string& account_name,
-      const std::string& cache_guid,
-      bool cookie_jar_mismatch,
-      CommitProcessor* commit_processor,
-      ExtensionsActivity* extensions_activity);
+  static Commit* Init(ModelTypeSet requested_types,
+                      ModelTypeSet enabled_types,
+                      size_t max_entries,
+                      const std::string& account_name,
+                      const std::string& cache_guid,
+                      bool cookie_jar_mismatch,
+                      bool cookie_jar_empty,
+                      CommitProcessor* commit_processor,
+                      ExtensionsActivity* extensions_activity);
 
   SyncerError PostAndProcessResponse(sessions::NudgeTracker* nudge_tracker,
                                      sessions::SyncSession* session,

diff --git a/sync/engine/commit_processor.cc b/sync/engine/commit_processor.cc
@@ -8,6 +8,7 @@
 
 #include <utility>
 
+#include "base/metrics/histogram_macros.h"
 #include "sync/engine/commit_contribution.h"
 #include "sync/engine/commit_contributor.h"
 #include "sync/protocol/sync.pb.h"
@@ -24,6 +25,8 @@ CommitProcessor::~CommitProcessor() {}
 void CommitProcessor::GatherCommitContributions(
     ModelTypeSet commit_types,
     size_t max_entries,
+    bool cookie_jar_mismatch,
+    bool cookie_jar_empty,
     Commit::ContributionMap* contributions) {
   size_t num_entries = 0;
   for (ModelTypeSet::Iterator it = commit_types.First();
@@ -42,6 +45,15 @@ void CommitProcessor::GatherCommitContributions(
     if (contribution) {
       num_entries += contribution->GetNumEntries();
       contributions->insert(std::make_pair(it.Get(), std::move(contribution)));
+
+      if (it.Get() == SESSIONS) {
+        UMA_HISTOGRAM_BOOLEAN("Sync.CookieJarMatchOnNavigation",
+                              !cookie_jar_mismatch);
+        if (cookie_jar_mismatch) {
+          UMA_HISTOGRAM_BOOLEAN("Sync.CookieJarEmptyOnMismatch",
+                                cookie_jar_empty);
+        }
+      }
     }
     if (num_entries >= max_entries) {
       DCHECK_EQ(num_entries, max_entries)

diff --git a/sync/engine/commit_processor.h b/sync/engine/commit_processor.h
@@ -45,8 +45,12 @@ class SYNC_EXPORT CommitProcessor {
   // map, gather any entries queued for commit into CommitContributions.  The
   // total number of entries in all the returned CommitContributions shall not
   // exceed |max_entries|.
+  // Note: |cookie_jar_mismatch| and |cookie_jar_empty| are used only for
+  // metrics recording purposes specific to the SESSIONS type.
   void GatherCommitContributions(ModelTypeSet commit_types,
                                  size_t max_entries,
+                                 bool cookie_jar_mismatch,
+                                 bool cookie_jar_empty,
                                  Commit::ContributionMap* contributions);
 
  private:

diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
@@ -179,13 +179,14 @@ SyncerError Syncer::BuildAndPostCommits(ModelTypeSet requested_types,
   // errors from the ServerConnectionManager if an exist has been requested.
   // However, it doesn't hurt to check it anyway.
   while (!ExitRequested()) {
-    std::unique_ptr<Commit> commit(Commit::Init(
-        requested_types, session->context()->GetEnabledTypes(),
-        session->context()->max_commit_batch_size(),
-        session->context()->account_name(),
-        session->context()->directory()->cache_guid(),
-        session->context()->cookie_jar_mismatch(), commit_processor,
-        session->context()->extensions_activity()));
+    std::unique_ptr<Commit> commit(
+        Commit::Init(requested_types, session->context()->GetEnabledTypes(),
+                     session->context()->max_commit_batch_size(),
+                     session->context()->account_name(),
+                     session->context()->directory()->cache_guid(),
+                     session->context()->cookie_jar_mismatch(),
+                     session->context()->cookie_jar_empty(), commit_processor,
+                     session->context()->extensions_activity()));
     if (!commit) {
       break;
     }

diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h
@@ -409,7 +409,7 @@ class SYNC_EXPORT SyncManager {
   // Updates Sync's tracking of whether the cookie jar has a mismatch with the
   // chrome account. See ClientConfigParams proto message for more info.
   // Note: this does not trigger a sync cycle. It just updates the sync context.
-  virtual void OnCookieJarChanged(bool account_mismatch) = 0;
+  virtual void OnCookieJarChanged(bool account_mismatch, bool empty_jar) = 0;
 };
 
 }  // namespace syncer

diff --git a/sync/internal_api/public/test/fake_sync_manager.h b/sync/internal_api/public/test/fake_sync_manager.h
@@ -116,7 +116,7 @@ class FakeSyncManager : public SyncManager {
       syncer::TypeDebugInfoObserver* observer) override;
   void RequestEmitDebugInfo() override;
   void ClearServerData(const ClearServerDataCallback& callback) override;
-  void OnCookieJarChanged(bool account_mismatch) override;
+  void OnCookieJarChanged(bool account_mismatch, bool empty_jar) override;
 
  private:
   scoped_refptr<base::SequencedTaskRunner> sync_task_runner_;

diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
@@ -1026,9 +1026,11 @@ void SyncManagerImpl::ClearServerData(const ClearServerDataCallback& callback) {
   scheduler_->ScheduleClearServerData(params);
 }
 
-void SyncManagerImpl::OnCookieJarChanged(bool account_mismatch) {
+void SyncManagerImpl::OnCookieJarChanged(bool account_mismatch,
+                                         bool empty_jar) {
   DCHECK(thread_checker_.CalledOnValidThread());
   session_context_->set_cookie_jar_mismatch(account_mismatch);
+  session_context_->set_cookie_jar_empty(empty_jar);
 }
 
 }  // namespace syncer
diff --git a/sync/internal_api/sync_manager_impl.h b/sync/internal_api/sync_manager_impl.h
@@ -114,7 +114,7 @@ class SYNC_EXPORT SyncManagerImpl
       syncer::TypeDebugInfoObserver* observer) override;
   void RequestEmitDebugInfo() override;
   void ClearServerData(const ClearServerDataCallback& callback) override;
-  void OnCookieJarChanged(bool account_mismatch) override;
+  void OnCookieJarChanged(bool account_mismatch, bool empty_jar) override;
 
   // SyncEncryptionHandler::Observer implementation.
   void OnPassphraseRequired(

diff --git a/sync/internal_api/test/fake_sync_manager.cc b/sync/internal_api/test/fake_sync_manager.cc
@@ -269,6 +269,7 @@ void FakeSyncManager::ClearServerData(const ClearServerDataCallback& callback) {
   callback.Run();
 }
 
-void FakeSyncManager::OnCookieJarChanged(bool account_mismatch) {}
+void FakeSyncManager::OnCookieJarChanged(bool account_mismatch,
+                                         bool empty_jar) {}
 
 }  // namespace syncer
diff --git a/sync/sessions/sync_session_context.cc b/sync/sessions/sync_session_context.cc
@@ -32,7 +32,8 @@ SyncSessionContext::SyncSessionContext(
       server_enabled_pre_commit_update_avoidance_(false),
       client_enabled_pre_commit_update_avoidance_(
           client_enabled_pre_commit_update_avoidance),
-      cookie_jar_mismatch_(false) {
+      cookie_jar_mismatch_(false),
+      cookie_jar_empty_(false) {
   std::vector<SyncEngineEventListener*>::const_iterator it;
   for (it = listeners.begin(); it != listeners.end(); ++it)
     listeners_.AddObserver(*it);

diff --git a/sync/sessions/sync_session_context.h b/sync/sessions/sync_session_context.h
@@ -134,6 +134,10 @@ class SYNC_EXPORT SyncSessionContext {
     cookie_jar_mismatch_ = cookie_jar_mismatch;
   }
 
+  bool cookie_jar_empty() const { return cookie_jar_empty_; }
+
+  void set_cookie_jar_empty(bool empty_jar) { cookie_jar_empty_ = empty_jar; }
+
  private:
   // Rather than force clients to set and null-out various context members, we
   // extend our encapsulation boundary to scoped helpers that take care of this
@@ -193,6 +197,9 @@ class SYNC_EXPORT SyncSessionContext {
    // mismatch implies all of them are different from the chrome account.
   bool cookie_jar_mismatch_;
 
+  // If there's a cookie jar mismatch, whether the cookie jar was empty or not.
+  bool cookie_jar_empty_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncSessionContext);
 };
 

diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
@@ -53857,6 +53857,23 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Sync.CookieJarEmptyOnMismatch" enum="BooleanEmpty">
+  <owner>zea@chromium.org</owner>
+  <summary>
+    Whether the gaia cookie jar was empty. Recorded on every SESSIONS commit
+    where the gaia cookie jar does not include the signed in user (
+    CookieJarMatchOnNavigation == false).
+  </summary>
+</histogram>
+
+<histogram name="Sync.CookieJarMatchOnNavigation" enum="BooleanMatched">
+  <owner>zea@chromium.org</owner>
+  <summary>
+    Whether the gaia cookie jar included the signed in user (matched) or not.
+    Recorded on every SESSIONS commit.
+  </summary>
+</histogram>
+
 <histogram name="Sync.CredentialsLost" enum="BooleanCredentialsLost">
   <owner>zea@chromium.org</owner>
   <summary>