Add Lacros histogram in unified feedback report.

Adds crosapi GetHistograms to allow ash pull the lacros histograms
from lacros and attach it in the unified feedback report as
a zipped file lacros_histograms.zip.

TEST:
1. Start Lacros chrome, send a feedback report. Verify on listnr
that the feedback report contains lacros_histograms.zip. Download
and unzip the file and verify the histogram data in unzipped file
looks good.
2. Don't launch Lacros chrome, send a feedback report from ash.
Verify on listnr that the feedback report does not contain
lacros_histograms.zip.

Bug: 1119925
Change-Id: I91f5f05270d28ec038fcb5f9ea5002191ebd7f5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2493363Reviewed-by: Miriam Zimmerman <mutexlox@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Jenny Zhang <jennyz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#821054}

chrome/browser/chromeos/crosapi/browser_manager.cc

@@ -58,7 +58,10 @@ BrowserManager* g_instance = nullptr;
 
 // The min version of LacrosChromeService mojo interface that supports
 // GetFeedbackData API.
-uint32_t kGetFeedbackDataMinVersion = 6;
+constexpr uint32_t kGetFeedbackDataMinVersion = 6;
+// The min version of LacrosChromeService mojo interface that supports
+// GetHistograms API.
+constexpr uint32_t kGetHistogramsMinVersion = 7;
 
 base::FilePath LacrosLogPath() {
   return browser_util::GetUserDataDir().Append("lacros.log");
@@ -233,6 +236,16 @@ void BrowserManager::GetFeedbackData(GetFeedbackDataCallback callback) {
   lacros_chrome_service_->GetFeedbackData(std::move(callback));
 }
 
+bool BrowserManager::GetHistogramsSupported() const {
+  return lacros_chrome_service_version_ >= kGetHistogramsMinVersion;
+}
+
+void BrowserManager::GetHistograms(GetHistogramsCallback callback) {
+  DCHECK(lacros_chrome_service_.is_connected());
+  DCHECK(GetHistogramsSupported());
+  lacros_chrome_service_->GetHistograms(std::move(callback));
+}
+
 void BrowserManager::AddObserver(BrowserManagerObserver* observer) {
   observers_.AddObserver(observer);
 }

chrome/browser/chromeos/crosapi/browser_manager.h

@@ -82,6 +82,13 @@ class BrowserManager : public session_manager::SessionManagerObserver {
   // Virtual for testing.
   virtual void GetFeedbackData(GetFeedbackDataCallback callback);
 
+  // Returns true if crosapi interface supports GetHistograms API.
+  bool GetHistogramsSupported() const;
+
+  using GetHistogramsCallback = base::OnceCallback<void(const std::string&)>;
+  // Gets Lacros histograms.
+  void GetHistograms(GetHistogramsCallback callback);
+
   void AddObserver(BrowserManagerObserver* observer);
   void RemoveObserver(BrowserManagerObserver* observer);
 

chrome/browser/chromeos/crosapi/test_mojo_connection_manager_unittest.cc

@@ -55,6 +55,7 @@ class TestLacrosChromeService : public crosapi::mojom::LacrosChromeService {
 
   void NewWindow(NewWindowCallback callback) override {}
   void GetFeedbackData(GetFeedbackDataCallback callback) override {}
+  void GetHistograms(GetHistogramsCallback callback) override {}
 
   bool init_is_called() { return init_is_called_; }
 

chrome/browser/extensions/BUILD.gn

@@ -931,15 +931,13 @@ static_library("extensions") {
 
   if (chromeos_is_browser_only || is_chromeos) {
     sources += [ "api/enterprise_platform_keys/enterprise_platform_keys_api.h" ]
+    deps += [ "//chromeos/crosapi/mojom" ]
     if (chromeos_is_browser_only) {
       sources += [
         "api/enterprise_platform_keys/enterprise_platform_keys_api_lacros.cc",
         "api/enterprise_platform_keys/enterprise_platform_keys_api_lacros.h",
       ]
-      deps += [
-        "//chromeos/crosapi/mojom",
-        "//chromeos/lacros",
-      ]
+      deps += [ "//chromeos/lacros" ]
     }
   }
 

chrome/browser/extensions/api/feedback_private/chrome_feedback_private_delegate.cc

@@ -29,6 +29,7 @@
 #include "base/strings/string_split.h"
 #include "base/system/sys_info.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
+#include "chrome/browser/chromeos/crosapi/browser_manager.h"
 #include "chrome/browser/chromeos/system_logs/iwlwifi_dump_log_source.h"
 #include "chrome/browser/chromeos/system_logs/single_debug_daemon_log_source.h"
 #include "chrome/browser/chromeos/system_logs/single_log_file_log_source.h"
@@ -236,6 +237,17 @@ ChromeFeedbackPrivateDelegate::GetLandingPageType(
   return board[0] == "eve" ? api::feedback_private::LANDING_PAGE_TYPE_TECHSTOP
                            : api::feedback_private::LANDING_PAGE_TYPE_NORMAL;
 }
+
+void ChromeFeedbackPrivateDelegate::GetLacrosHistograms(
+    GetHistogramsCallback callback) {
+  crosapi::BrowserManager* browser_manager = crosapi::BrowserManager::Get();
+  if (browser_manager->GetHistogramsSupported() &&
+      browser_manager->IsRunning()) {
+    browser_manager->GetHistograms(std::move(callback));
+  } else {
+    std::move(callback).Run(std::string());
+  }
+}
 #endif  // defined(OS_CHROMEOS)
 
 std::string ChromeFeedbackPrivateDelegate::GetSignedInUserEmail(

chrome/browser/extensions/api/feedback_private/chrome_feedback_private_delegate.h

@@ -31,6 +31,7 @@ class ChromeFeedbackPrivateDelegate : public FeedbackPrivateDelegate {
   void UnloadFeedbackExtension(content::BrowserContext* context) const override;
   api::feedback_private::LandingPageType GetLandingPageType(
       const feedback::FeedbackData& feedback_data) const override;
+  void GetLacrosHistograms(GetHistogramsCallback callback) override;
 #endif  // defined(OS_CHROMEOS)
   std::string GetSignedInUserEmail(
       content::BrowserContext* context) const override;

chrome/browser/lacros/lacros_chrome_service_delegate_impl.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/lacros/lacros_chrome_service_delegate_impl.h"
 
 #include "base/logging.h"
+#include "base/metrics/statistics_recorder.h"
 #include "chrome/browser/lacros/feedback_util.h"
 #include "chrome/browser/lacros/system_logs/lacros_system_log_fetcher.h"
 #include "chrome/browser/profiles/profile.h"
@@ -12,9 +13,16 @@
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/common/channel_info.h"
 #include "components/feedback/feedback_report.h"
+#include "components/feedback/feedback_util.h"
 #include "components/feedback/system_logs/system_logs_fetcher.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 
+namespace {
+
+constexpr char kHistogramsFilename[] = "lacros_histograms.txt";
+
+}  // namespace
+
 LacrosChromeServiceDelegateImpl::LacrosChromeServiceDelegateImpl() = default;
 
 LacrosChromeServiceDelegateImpl::~LacrosChromeServiceDelegateImpl() = default;
@@ -44,6 +52,20 @@ void LacrosChromeServiceDelegateImpl::GetFeedbackData(
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
+void LacrosChromeServiceDelegateImpl::GetHistograms(
+    GetHistogramsCallback callback) {
+  std::string histograms =
+      base::StatisticsRecorder::ToJSON(base::JSON_VERBOSITY_LEVEL_FULL);
+  std::string compressed_histograms;
+  if (feedback_util::ZipString(base::FilePath(kHistogramsFilename),
+                               std::move(histograms), &compressed_histograms)) {
+    std::move(callback).Run(std::move(compressed_histograms));
+  } else {
+    LOG(ERROR) << "Failed to compress lacros histograms.";
+    std::move(callback).Run(std::string());
+  }
+}
+
 void LacrosChromeServiceDelegateImpl::OnSystemInformationReady(
     std::unique_ptr<system_logs::SystemLogsResponse> sys_info) {
   base::Value system_log_entries(base::Value::Type::DICTIONARY);

chrome/browser/lacros/lacros_chrome_service_delegate_impl.h

@@ -24,6 +24,7 @@ class LacrosChromeServiceDelegateImpl
   void NewWindow() override;
   std::string GetChromeVersion() override;
   void GetFeedbackData(GetFeedbackDataCallback callback) override;
+  void GetHistograms(GetHistogramsCallback callback) override;
 
  private:
   void OnSystemInformationReady(

chromeos/crosapi/mojom/crosapi.mojom

@@ -11,6 +11,7 @@ import "chromeos/crosapi/mojom/keystore_service.mojom";
 import "chromeos/crosapi/mojom/message_center.mojom";
 import "chromeos/crosapi/mojom/screen_manager.mojom";
 import "chromeos/crosapi/mojom/select_file.mojom";
+import "mojo/public/mojom/base/big_string.mojom";
 import "mojo/public/mojom/base/token.mojom";
 import "mojo/public/mojom/base/values.mojom";
 import "services/device/public/mojom/hid.mojom";
@@ -188,4 +189,9 @@ interface LacrosChromeService {
   [MinVersion=6]
   GetFeedbackData@3() => (
       mojo_base.mojom.DictionaryValue feedback_info);
+
+  // Returns the lacros histograms used for generating feedback report.
+  // The histograms returned is zip compressed and is typically around 100KB.
+  [MinVersion=7]
+  GetHistograms@4() => (mojo_base.mojom.BigString compressed_histograms);
 };

chromeos/lacros/BUILD.gn

@@ -18,7 +18,6 @@ component("lacros") {
     "//mojo/public/cpp/bindings",
   ]
   sources = [
-    "get_feedback_data_callback.h",
     "lacros_chrome_service_delegate.h",
     "lacros_chrome_service_impl.cc",
     "lacros_chrome_service_impl.h",

chromeos/lacros/get_feedback_data_callback.h

-// Copyright 2020 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.
-
-#ifndef CHROMEOS_LACROS_GET_FEEDBACK_DATA_CALLBACK_H_
-#define CHROMEOS_LACROS_GET_FEEDBACK_DATA_CALLBACK_H_
-
-#include "base/callback.h"
-#include "base/values.h"
-
-// Callback used by GetFeedbackData crosapi and all the downstream classes
-// that propagate GetFeedbackData call for getting lacros feedback data.
-using GetFeedbackDataCallback = base::OnceCallback<void(base::Value)>;
-
-#endif  //  CHROMEOS_LACROS_GET_FEEDBACK_DATA_CALLBACK_H_

chromeos/lacros/lacros_chrome_service_delegate.h

@@ -7,7 +7,8 @@
 
 #include <string>
 
-#include "chromeos/lacros/get_feedback_data_callback.h"
+#include "base/callback.h"
+#include "base/values.h"
 
 namespace chromeos {
 
@@ -26,8 +27,13 @@ class LacrosChromeServiceDelegate {
   // For example, "87.0.0.1 dev", "86.0.4240.38 beta".
   virtual std::string GetChromeVersion() = 0;
 
+  using GetFeedbackDataCallback = base::OnceCallback<void(base::Value)>;
   // Gets lacros feedback data.
   virtual void GetFeedbackData(GetFeedbackDataCallback callback) = 0;
+
+  using GetHistogramsCallback = base::OnceCallback<void(const std::string&)>;
+  // Gets lacros histograms.
+  virtual void GetHistograms(GetHistogramsCallback callback) = 0;
 };
 
 }  // namespace chromeos

chromeos/lacros/lacros_chrome_service_impl.cc

@@ -82,6 +82,13 @@ class LacrosChromeServiceNeverBlockingState
                        owner_, std::move(callback)));
   }
 
+  void GetHistograms(GetHistogramsCallback callback) override {
+    owner_sequence_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&LacrosChromeServiceImpl::GetHistogramsAffineSequence,
+                       owner_, std::move(callback)));
+  }
+
   // Unlike most of other methods of this class, this is called on the
   // affined thread. Specifically, it is intended to be called before starting
   // the message pumping of the affined thread to pass the initialization
@@ -421,6 +428,12 @@ void LacrosChromeServiceImpl::GetFeedbackDataAffineSequence(
   delegate_->GetFeedbackData(std::move(callback));
 }
 
+void LacrosChromeServiceImpl::GetHistogramsAffineSequence(
+    GetHistogramsCallback callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
+  delegate_->GetHistograms(std::move(callback));
+}
+
 int LacrosChromeServiceImpl::AshChromeServiceVersion() {
   if (g_disable_all_crosapi_for_tests)
     return -1;

chromeos/lacros/lacros_chrome_service_impl.h

@@ -19,7 +19,6 @@
 #include "chromeos/crosapi/mojom/message_center.mojom.h"
 #include "chromeos/crosapi/mojom/screen_manager.mojom.h"
 #include "chromeos/crosapi/mojom/select_file.mojom.h"
-#include "chromeos/lacros/get_feedback_data_callback.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -191,9 +190,14 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   // Creates a new window on the affine sequence.
   void NewWindowAffineSequence();
 
+  using GetFeedbackDataCallback = base::OnceCallback<void(base::Value)>;
   // Gets feedback data on the affine sequence.
   void GetFeedbackDataAffineSequence(GetFeedbackDataCallback callback);
 
+  using GetHistogramsCallback = base::OnceCallback<void(const std::string&)>;
+  // Gets histograms on the affine sequence.
+  void GetHistogramsAffineSequence(GetHistogramsCallback callback);
+
   // Returns ash's version of the AshChromeService mojo interface version. This
   // determines which interface methods are available. This is safe to call from
   // any sequence. This can only be called after BindReceiver().

extensions/browser/api/feedback_private/feedback_private_api.cc

@@ -67,6 +67,10 @@ constexpr char kBluetoothLogsAttachmentNameOld[] = "bluetooth_logs.old.bz2";
 
 constexpr int kKaleidoscopeProductId = 5192933;
 
+#if defined(OS_CHROMEOS)
+constexpr char kLacrosHistogramsFilename[] = "lacros_histograms.zip";
+#endif
+
 // Getting the filename of a blob prepends a "C:\fakepath" to the filename.
 // This is undesirable, strip it if it exists.
 std::string StripFakepath(const std::string& path) {
@@ -356,7 +360,7 @@ ExtensionFunction::ResponseAction FeedbackPrivateSendFeedbackFunction::Run() {
 #if defined(OS_CHROMEOS)
     delegate->FetchExtraLogs(
         feedback_data,
-        base::BindOnce(&FeedbackPrivateSendFeedbackFunction::OnAllLogsFetched,
+        base::BindOnce(&FeedbackPrivateSendFeedbackFunction::OnAshLogsFetched,
                        this, send_histograms, send_bluetooth_logs,
                        send_tab_titles));
     return RespondLater();
@@ -411,6 +415,36 @@ void FeedbackPrivateSendFeedbackFunction::OnAllLogsFetched(
                  GetLandingPageType(*feedback_data)));
 }
 
+#if defined(OS_CHROMEOS)
+void FeedbackPrivateSendFeedbackFunction::OnAshLogsFetched(
+    bool send_histograms,
+    bool send_bluetooth_logs,
+    bool send_tab_titles,
+    scoped_refptr<feedback::FeedbackData> feedback_data) {
+  FeedbackPrivateDelegate* feedback_private_delegate =
+      ExtensionsAPIClient::Get()->GetFeedbackPrivateDelegate();
+  feedback_private_delegate->GetLacrosHistograms(base::BindOnce(
+      &FeedbackPrivateSendFeedbackFunction::OnLacrosHistogramsFetched, this,
+      send_histograms, send_bluetooth_logs, send_tab_titles, feedback_data));
+}
+
+void FeedbackPrivateSendFeedbackFunction::OnLacrosHistogramsFetched(
+    bool send_histograms,
+    bool send_bluetooth_logs,
+    bool send_tab_titles,
+    scoped_refptr<feedback::FeedbackData> feedback_data,
+    const std::string& compressed_histograms) {
+  // Attach lacros histogram to feedback data.
+  if (!compressed_histograms.empty()) {
+    feedback_data->AddFile(kLacrosHistogramsFilename,
+                           std::move(compressed_histograms));
+  }
+
+  OnAllLogsFetched(send_histograms, send_bluetooth_logs, send_tab_titles,
+                   feedback_data);
+}
+#endif  // defined(OS_CHROMEOS)
+
 void FeedbackPrivateSendFeedbackFunction::OnCompleted(
     api::feedback_private::LandingPageType type,
     bool success) {

extensions/browser/api/feedback_private/feedback_private_api.h

@@ -150,6 +150,19 @@ class FeedbackPrivateSendFeedbackFunction : public ExtensionFunction {
                         bool send_tab_titles,
                         scoped_refptr<feedback::FeedbackData> feedback_data);
   void OnCompleted(api::feedback_private::LandingPageType type, bool success);
+
+#if defined(OS_CHROMEOS)
+  void OnAshLogsFetched(bool send_histograms,
+                        bool send_bluetooth_logs,
+                        bool send_tab_titles,
+                        scoped_refptr<feedback::FeedbackData> feedback_data);
+  void OnLacrosHistogramsFetched(
+      bool send_histograms,
+      bool send_bluetooth_logs,
+      bool send_tab_titles,
+      scoped_refptr<feedback::FeedbackData> feedback_data,
+      const std::string& compressed_histograms);
+#endif  // OS_CHROMEOS
 };
 
 class FeedbackPrivateLoginFeedbackCompleteFunction : public ExtensionFunction {

extensions/browser/api/feedback_private/feedback_private_delegate.h

@@ -74,6 +74,11 @@ class FeedbackPrivateDelegate {
   // report is successfully sent.
   virtual api::feedback_private::LandingPageType GetLandingPageType(
       const feedback::FeedbackData& feedback_data) const = 0;
+
+  using GetHistogramsCallback = base::OnceCallback<void(const std::string&)>;
+  // Gets Lacros histograms in zip compressed format which will be attached
+  // as a file in unified feedback report.
+  virtual void GetLacrosHistograms(GetHistogramsCallback callback) = 0;
 #endif
 
   // Returns the normalized email address of the signed-in user associated with

extensions/shell/browser/api/feedback_private/shell_feedback_private_delegate.cc

@@ -58,6 +58,12 @@ ShellFeedbackPrivateDelegate::GetLandingPageType(
     const feedback::FeedbackData& feedback_data) const {
   return api::feedback_private::LANDING_PAGE_TYPE_NOLANDINGPAGE;
 }
+
+void ShellFeedbackPrivateDelegate::GetLacrosHistograms(
+    GetHistogramsCallback callback) {
+  NOTIMPLEMENTED();
+  std::move(callback).Run(std::string());
+}
 #endif
 
 std::string ShellFeedbackPrivateDelegate::GetSignedInUserEmail(

extensions/shell/browser/api/feedback_private/shell_feedback_private_delegate.h

@@ -31,6 +31,7 @@ class ShellFeedbackPrivateDelegate : public FeedbackPrivateDelegate {
   void UnloadFeedbackExtension(content::BrowserContext* context) const override;
   api::feedback_private::LandingPageType GetLandingPageType(
       const feedback::FeedbackData& feedback_data) const override;
+  void GetLacrosHistograms(GetHistogramsCallback callback) override;
 #endif  // defined(OS_CHROMEOS)
   std::string GetSignedInUserEmail(
       content::BrowserContext* context) const override;

mojo/public/mojom/base/big_string.mojom

@@ -14,6 +14,7 @@ import "mojo/public/mojom/base/big_buffer.mojom";
 // an arbitrarily large amount of data (available memory permitting) without
 // negatively impacting IPC performance or hitting hard message size
 // boundaries.
+[Stable]
 struct BigString {
   mojo_base.mojom.BigBuffer data;
 };