Get Lacros system information to ash via mojo for feedback reports.

This cl adds the following:
1. A new API GetSystemInformation in crosapi::LacrosChromeService
to gather Lacros system information which is called from ash.
2. A new SystemLogsSource LacrosSystemLogRemoteSource to fetch
Lacros system log data.

The unified feedback report will include the additional lacros
system information data, and each log entry key with be prefixed
with "Lacros ", so that it can be differentiate from the ash
system information log entries. The unified feedback will include
additional lacros log data such as crash report ids, extensions,
memory usage, etc, in system_logs.zip of the feedback report.

TEST:
1. Open Lacros chrome, file a feedback from "Help/Report an issue"
menu, send a feedback report. Find the report on listnr, download
system_logs.zip and check the content. In addition to ash log data,
it should include:
Lacros CHROME VERSION
Lacros OS VERSION
Lacros about_sync_data
Lacros crash_report_ids
Lacros data_reduction_proxy
Lacros extensions
lacros mem_usgae
Lacros mem_usage_with_title

Also, clicking on "system and app information" link on feedback
dialog, it will show all system log data in a preview UI, which
should also display the above Larcros log entries along with ash
log entries.

2. File a feedback report from ash chrome, make sure there is a
Lacros chrome running. It should send a unified feedback report
which includes the additional Lacros system information as shown
in Test #1.


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

chrome/browser/BUILD.gn

@@ -4399,8 +4399,12 @@ static_library("browser") {
       "lacros/account_manager_facade_lacros.h",
       "lacros/account_manager_util.cc",
       "lacros/account_manager_util.h",
+      "lacros/feedback_util.cc",
+      "lacros/feedback_util.h",
       "lacros/lacros_chrome_service_delegate_impl.cc",
       "lacros/lacros_chrome_service_delegate_impl.h",
+      "lacros/system_logs/lacros_system_log_fetcher.cc",
+      "lacros/system_logs/lacros_system_log_fetcher.h",
       "metrics/enrollment_status.h",
       "metrics/lacros_metrics_provider.cc",
       "metrics/lacros_metrics_provider.h",

chrome/browser/chromeos/BUILD.gn

@@ -995,10 +995,13 @@ source_set("chromeos") {
     "crosapi/browser_loader.h",
     "crosapi/browser_manager.cc",
     "crosapi/browser_manager.h",
+    "crosapi/browser_manager_observer.h",
     "crosapi/browser_util.cc",
     "crosapi/browser_util.h",
     "crosapi/environment_provider.cc",
     "crosapi/environment_provider.h",
+    "crosapi/fake_browser_manager.cc",
+    "crosapi/fake_browser_manager.h",
     "crosapi/feedback_ash.cc",
     "crosapi/feedback_ash.h",
     "crosapi/keystore_service_ash.cc",
@@ -2684,6 +2687,8 @@ source_set("chromeos") {
     "system/user_removal_manager.h",
     "system_logs/command_line_log_source.cc",
     "system_logs/command_line_log_source.h",
+    "system_logs/crosapi_system_log_source.cc",
+    "system_logs/crosapi_system_log_source.h",
     "system_logs/dbus_log_source.cc",
     "system_logs/dbus_log_source.h",
     "system_logs/debug_daemon_log_source.cc",
@@ -3720,6 +3725,7 @@ source_set("unit_tests") {
     "system/device_disabling_manager_unittest.cc",
     "system/procfs_util_unittest.cc",
     "system/user_removal_manager_unittest.cc",
+    "system_logs/crosapi_system_log_source_unittest.cc",
     "system_logs/shill_log_source_unittest.cc",
     "system_logs/single_debug_daemon_log_source_unittest.cc",
     "system_logs/single_log_file_log_source_unittest.cc",

chrome/browser/chromeos/crosapi/browser_manager.cc

@@ -56,6 +56,10 @@ namespace {
 // Pointer to the global instance of BrowserManager.
 BrowserManager* g_instance = nullptr;
 
+// The min version of LacrosChromeService mojo interface that supports
+// GetFeedbackData API.
+uint32_t kGetFeedbackDataMinVersion = 6;
+
 base::FilePath LacrosLogPath() {
   return browser_util::GetUserDataDir().Append("lacros.log");
 }
@@ -149,7 +153,8 @@ BrowserManager::BrowserManager(
   // Wait to query the flag until the user has entered the session. Enterprise
   // devices restart Chrome during login to apply flags. We don't want to run
   // the flag-off cleanup logic until we know we have the final flag state.
-  session_manager::SessionManager::Get()->AddObserver(this);
+  if (session_manager::SessionManager::Get())
+    session_manager::SessionManager::Get()->AddObserver(this);
 
   std::string socket_path =
       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
@@ -164,7 +169,8 @@ BrowserManager::BrowserManager(
 BrowserManager::~BrowserManager() {
   // Unregister, just in case the manager is destroyed before
   // OnUserSessionStarted() is called.
-  session_manager::SessionManager::Get()->RemoveObserver(this);
+  if (session_manager::SessionManager::Get())
+    session_manager::SessionManager::Get()->RemoveObserver(this);
 
   // Try to kill the lacros-chrome binary.
   if (lacros_process_.IsValid())
@@ -179,6 +185,10 @@ bool BrowserManager::IsReady() const {
          state_ != State::UNAVAILABLE;
 }
 
+bool BrowserManager::IsRunning() const {
+  return state_ == State::RUNNING;
+}
+
 void BrowserManager::SetLoadCompleteCallback(LoadCompleteCallback callback) {
   load_complete_callback_ = std::move(callback);
 }
@@ -213,6 +223,24 @@ void BrowserManager::NewWindow() {
   lacros_chrome_service_->NewWindow(base::DoNothing());
 }
 
+bool BrowserManager::GetFeedbackDataSupported() const {
+  return lacros_chrome_service_version_ >= kGetFeedbackDataMinVersion;
+}
+
+void BrowserManager::GetFeedbackData(GetFeedbackDataCallback callback) {
+  DCHECK(lacros_chrome_service_.is_connected());
+  DCHECK(GetFeedbackDataSupported());
+  lacros_chrome_service_->GetFeedbackData(std::move(callback));
+}
+
+void BrowserManager::AddObserver(BrowserManagerObserver* observer) {
+  observers_.AddObserver(observer);
+}
+
+void BrowserManager::RemoveObserver(BrowserManagerObserver* observer) {
+  observers_.RemoveObserver(observer);
+}
+
 void BrowserManager::Start() {
   DCHECK_EQ(state_, State::STOPPED);
   DCHECK(!lacros_path_.empty());
@@ -305,6 +333,10 @@ void BrowserManager::StartWithLogFile(base::ScopedFD logfd) {
       base::BindOnce(&BrowserManager::OnAshChromeServiceReceiverReceived,
                      weak_factory_.GetWeakPtr()));
 
+  lacros_chrome_service_.QueryVersion(
+      base::BindOnce(&BrowserManager::OnLacrosChromeServiceVersionReady,
+                     weak_factory_.GetWeakPtr()));
+
   // Create the lacros-chrome subprocess.
   base::RecordAction(base::UserMetricsAction("Lacros.Launch"));
   // If lacros_process_ already exists, because it does not call waitpid(2),
@@ -346,6 +378,8 @@ void BrowserManager::OnMojoDisconnected() {
       base::BindOnce(&TerminateLacrosChrome, std::move(lacros_process_)),
       base::BindOnce(&BrowserManager::OnLacrosChromeTerminated,
                      weak_factory_.GetWeakPtr()));
+
+  NotifyMojoDisconnected();
 }
 
 void BrowserManager::OnLacrosChromeTerminated() {
@@ -403,4 +437,13 @@ void BrowserManager::OnLoadComplete(const base::FilePath& path) {
   }
 }
 
+void BrowserManager::NotifyMojoDisconnected() {
+  for (auto& observer : observers_)
+    observer.OnMojoDisconnected();
+}
+
+void BrowserManager::OnLacrosChromeServiceVersionReady(uint32_t version) {
+  lacros_chrome_service_version_ = version;
+}
+
 }  // namespace crosapi

chrome/browser/chromeos/crosapi/browser_manager.h

@@ -11,7 +11,9 @@
 #include "base/files/file_path.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
 #include "base/process/process.h"
+#include "chrome/browser/chromeos/crosapi/browser_manager_observer.h"
 #include "chrome/browser/chromeos/crosapi/environment_provider.h"
 #include "chromeos/crosapi/mojom/crosapi.mojom.h"
 #include "components/session_manager/core/session_manager_observer.h"
@@ -48,6 +50,10 @@ class BrowserManager : public session_manager::SessionManagerObserver {
   // call SetLoadCompleteCallback() to be notified when the download completes.
   bool IsReady() const;
 
+  // Returns true if Lacros is in running state.
+  // Virtual for testing.
+  virtual bool IsRunning() const;
+
   // Sets a callback to be called when the binary download completes. The
   // download may not be successful.
   using LoadCompleteCallback = base::OnceCallback<void(bool success)>;
@@ -68,11 +74,26 @@ class BrowserManager : public session_manager::SessionManagerObserver {
   // so should be avoided.
   void NewWindow();
 
+  // Returns true if crosapi interface supports GetFeedbackData API.
+  bool GetFeedbackDataSupported() const;
+
+  using GetFeedbackDataCallback = base::OnceCallback<void(base::Value)>;
+  // Gathers Lacros feedback data.
+  // Virtual for testing.
+  virtual void GetFeedbackData(GetFeedbackDataCallback callback);
+
+  void AddObserver(BrowserManagerObserver* observer);
+  void RemoveObserver(BrowserManagerObserver* observer);
+
   const std::string& lacros_version() const { return lacros_version_; }
   void set_lacros_version(const std::string& version) {
     lacros_version_ = version;
   }
 
+ protected:
+  // Notifies Mojo connection to lacros-chrome has been disconnected.
+  void NotifyMojoDisconnected();
+
  private:
   enum class State {
     // Lacros is not initialized yet.
@@ -132,6 +153,9 @@ class BrowserManager : public session_manager::SessionManagerObserver {
   // Called on load completion.
   void OnLoadComplete(const base::FilePath& path);
 
+  // Callback of QueryVersion for LacrosChromeService.
+  void OnLacrosChromeServiceVersionReady(uint32_t version);
+
   State state_ = State::NOT_INITIALIZED;
 
   // May be null in tests.
@@ -148,6 +172,9 @@ class BrowserManager : public session_manager::SessionManagerObserver {
   // For example, "87.0.0.1 dev", "86.0.4240.38 beta".
   std::string lacros_version_;
 
+  // Version of LacrosChromeService mojo interface.
+  uint32_t lacros_chrome_service_version_ = 0;
+
   // Called when the binary download completes.
   LoadCompleteCallback load_complete_callback_;
 
@@ -170,6 +197,8 @@ class BrowserManager : public session_manager::SessionManagerObserver {
   // Used to pass ash-chrome specific flags/configurations to lacros-chrome.
   std::unique_ptr<EnvironmentProvider> environment_provider_;
 
+  base::ObserverList<BrowserManagerObserver> observers_;
+
   base::WeakPtrFactory<BrowserManager> weak_factory_{this};
 };
 

chrome/browser/chromeos/crosapi/browser_manager_observer.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 CHROME_BROWSER_CHROMEOS_CROSAPI_BROWSER_MANAGER_OBSERVER_H_
+#define CHROME_BROWSER_CHROMEOS_CROSAPI_BROWSER_MANAGER_OBSERVER_H_
+
+#include "base/observer_list_types.h"
+
+namespace crosapi {
+
+class BrowserManagerObserver : public base::CheckedObserver {
+ public:
+  // Invoked when the Mojo connection to lacros-chrome is disconnected.
+  virtual void OnMojoDisconnected() {}
+};
+
+}  // namespace crosapi
+
+#endif  // CHROME_BROWSER_CHROMEOS_CROSAPI_BROWSER_MANAGER_OBSERVER_H_

chrome/browser/chromeos/crosapi/fake_browser_manager.cc

+// 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.
+
+#include "chrome/browser/chromeos/crosapi/fake_browser_manager.h"
+
+#include "chrome/browser/component_updater/cros_component_manager.h"
+
+namespace crosapi {
+
+FakeBrowserManager::FakeBrowserManager()
+    : BrowserManager(
+          scoped_refptr<component_updater::CrOSComponentManager>(nullptr)) {}
+
+FakeBrowserManager::~FakeBrowserManager() = default;
+
+void FakeBrowserManager::SetGetFeedbackDataResponse(base::Value response) {
+  feedback_response_ = std::move(response);
+}
+
+void FakeBrowserManager::SignalMojoDisconnected() {
+  NotifyMojoDisconnected();
+}
+
+bool FakeBrowserManager::IsRunning() const {
+  return is_running_;
+}
+
+void FakeBrowserManager::GetFeedbackData(GetFeedbackDataCallback callback) {
+  const base::DictionaryValue* sysinfo_entries;
+  feedback_response_.GetAsDictionary(&sysinfo_entries);
+
+  // Run |callback| with the pre-set |feedback_responses_|, unless testing
+  // client requests waiting for crosapi mojo disconnected event being observed.
+  if (!wait_for_mojo_disconnect_)
+    std::move(callback).Run(std::move(feedback_response_));
+}
+
+}  // namespace crosapi

chrome/browser/chromeos/crosapi/fake_browser_manager.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 CHROME_BROWSER_CHROMEOS_CROSAPI_FAKE_BROWSER_MANAGER_H_
+#define CHROME_BROWSER_CHROMEOS_CROSAPI_FAKE_BROWSER_MANAGER_H_
+
+#include "base/values.h"
+#include "chrome/browser/chromeos/crosapi/browser_manager.h"
+
+namespace crosapi {
+
+// A fake implementation of BrowserManager, used for testing.
+class FakeBrowserManager : public BrowserManager {
+ public:
+  FakeBrowserManager();
+  FakeBrowserManager(const FakeBrowserManager&) = delete;
+  FakeBrowserManager& operator=(const FakeBrowserManager&) = delete;
+
+  ~FakeBrowserManager() override;
+
+  void set_is_running(bool value) { is_running_ = value; }
+  void set_wait_for_mojo_disconnect(bool value) {
+    wait_for_mojo_disconnect_ = value;
+  }
+
+  // Set up response data to be sent for the callback of Fetch.
+  void SetGetFeedbackDataResponse(base::Value response);
+
+  // Simulates crosapi mojo disconnection event observed.
+  void SignalMojoDisconnected();
+
+  // BrowserManager:
+  bool IsRunning() const override;
+  void GetFeedbackData(GetFeedbackDataCallback callback) override;
+
+ private:
+  // State indicating Lacros is running or not.
+  bool is_running_ = false;
+
+  // If this flag is set to true, simulate the case that mojo disconnect
+  // signal is received before the log data is fetched.
+  bool wait_for_mojo_disconnect_ = false;
+
+  // Stores the response to be sent back for GetFeedbackData callback.
+  base::Value feedback_response_;
+};
+
+}  // namespace crosapi
+
+#endif  // CHROME_BROWSER_CHROMEOS_CROSAPI_FAKE_BROWSER_MANAGER_H_

chrome/browser/chromeos/crosapi/test_mojo_connection_manager_unittest.cc

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

chrome/browser/chromeos/system_logs/crosapi_system_log_source.cc

+// 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.
+
+#include "chrome/browser/chromeos/system_logs/crosapi_system_log_source.h"
+
+#include "base/bind.h"
+#include "chrome/browser/chromeos/crosapi/browser_manager.h"
+
+namespace system_logs {
+
+namespace {
+constexpr char kLacrosLogEntryPrefix[] = "Lacros ";
+}  // namespace
+
+CrosapiSystemLogSource::CrosapiSystemLogSource()
+    : SystemLogsSource("LacrosSystemLog") {
+  crosapi::BrowserManager::Get()->AddObserver(this);
+}
+
+CrosapiSystemLogSource::~CrosapiSystemLogSource() {
+  crosapi::BrowserManager::Get()->RemoveObserver(this);
+}
+
+void CrosapiSystemLogSource::Fetch(SysLogsSourceCallback callback) {
+  DCHECK(callback_.is_null());
+  DCHECK(!callback.is_null());
+
+  if (crosapi::BrowserManager::Get()->IsRunning()) {
+    callback_ = std::move(callback);
+    crosapi::BrowserManager::Get()->GetFeedbackData(
+        base::BindOnce(&CrosapiSystemLogSource::OnGetFeedbackData,
+                       weak_ptr_factory_.GetWeakPtr()));
+  } else {
+    // Fetch is called right after the data source is added to the
+    // SystemLogsFetcher when Lacros is running, it is unlikely Lacros will
+    // be terminated before Fetch is called. But it does not hurt to check
+    // again and handle the case for playing safely.
+    std::move(callback).Run(std::make_unique<SystemLogsResponse>());
+  }
+}
+
+void CrosapiSystemLogSource::OnGetFeedbackData(base::Value system_infos) {
+  auto response = std::make_unique<SystemLogsResponse>();
+  DCHECK(system_infos.is_dict());
+  const base::DictionaryValue* sysinfo_dict;
+  if (system_infos.GetAsDictionary(&sysinfo_dict)) {
+    for (const auto& item : sysinfo_dict->DictItems()) {
+      std::string log_entry_key = kLacrosLogEntryPrefix + item.first;
+      std::string log_entry_value;
+      if (item.second.GetAsString(&log_entry_value)) {
+        response->emplace(log_entry_key, log_entry_value);
+      } else {
+        LOG(ERROR) << "Failed to retrieve the content for log entry: "
+                   << log_entry_key;
+      }
+    }
+  }
+  std::move(callback_).Run(std::move(response));
+}
+
+void CrosapiSystemLogSource::OnMojoDisconnected() {
+  if (callback_.is_null())
+    return;
+
+  // Run callback_ with empty response.
+  std::move(callback_).Run(std::make_unique<SystemLogsResponse>());
+}
+
+}  //  namespace system_logs

chrome/browser/chromeos/system_logs/crosapi_system_log_source.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 CHROME_BROWSER_CHROMEOS_SYSTEM_LOGS_CROSAPI_SYSTEM_LOG_SOURCE_H_
+#define CHROME_BROWSER_CHROMEOS_SYSTEM_LOGS_CROSAPI_SYSTEM_LOG_SOURCE_H_
+
+#include "base/memory/weak_ptr.h"
+#include "base/values.h"
+#include "chrome/browser/chromeos/crosapi/browser_manager_observer.h"
+#include "components/feedback/system_logs/system_logs_source.h"
+
+namespace system_logs {
+
+// Gathers Lacros system information log data via crosapi calls.
+class CrosapiSystemLogSource : public SystemLogsSource,
+                               public crosapi::BrowserManagerObserver {
+ public:
+  CrosapiSystemLogSource();
+  ~CrosapiSystemLogSource() override;
+  CrosapiSystemLogSource(const CrosapiSystemLogSource&) = delete;
+  CrosapiSystemLogSource& operator=(const CrosapiSystemLogSource&) = delete;
+
+  // SystemLogsSource
+  void Fetch(SysLogsSourceCallback request) override;
+
+ private:
+  // Callback for getting lacros feedback data.
+  void OnGetFeedbackData(base::Value system_infos);
+
+  // crosapi::BrowserManagerObserver
+  void OnMojoDisconnected() override;
+
+  SysLogsSourceCallback callback_;
+  base::WeakPtrFactory<CrosapiSystemLogSource> weak_ptr_factory_{this};
+};
+
+}  // namespace system_logs
+
+#endif  //  CHROME_BROWSER_CHROMEOS_SYSTEM_LOGS_CROSAPI_SYSTEM_LOG_SOURCE_H_

chrome/browser/chromeos/system_logs/crosapi_system_log_source_unittest.cc

+// 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.
+
+#include "chrome/browser/chromeos/system_logs/crosapi_system_log_source.h"
+
+#include "base/bind.h"
+#include "base/macros.h"
+#include "base/run_loop.h"
+#include "chrome/browser/chromeos/crosapi/fake_browser_manager.h"
+#include "content/public/test/browser_task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace system_logs {
+
+class CrosapiSystemLogSourceTest : public ::testing::Test {
+ public:
+  CrosapiSystemLogSourceTest()
+      : browser_manager_(std::make_unique<crosapi::FakeBrowserManager>()) {}
+  CrosapiSystemLogSourceTest(const CrosapiSystemLogSourceTest&) = delete;
+  CrosapiSystemLogSourceTest& operator=(const CrosapiSystemLogSourceTest&) =
+      delete;
+  ~CrosapiSystemLogSourceTest() override = default;
+
+  void SetUp() override {
+    // Set Lacros in running state by default.
+    browser_manager_->set_is_running(true);
+  }
+
+ protected:
+  SysLogsSourceCallback fetch_callback() {
+    return base::BindOnce(&CrosapiSystemLogSourceTest::OnFetchComplete,
+                          base::Unretained(this));
+  }
+
+  void SetupBrowserManagerResponse(base::Value response) {
+    browser_manager_->SetGetFeedbackDataResponse(std::move(response));
+  }
+
+  void SignalMojoDisconnected() { browser_manager_->SignalMojoDisconnected(); }
+
+  void SetLacrosNotRunning() { browser_manager_->set_is_running(false); }
+
+  void SetWaitForMojoDisconnect() {
+    browser_manager_->set_wait_for_mojo_disconnect(true);
+  }
+
+  void OnFetchComplete(std::unique_ptr<SystemLogsResponse> response) {
+    ++num_callback_calls_;
+    response_ = *response;
+  }
+
+  int num_callback_calls() const { return num_callback_calls_; }
+  const SystemLogsResponse& response() const { return response_; }
+
+ private:
+  // Creates the necessary browser threads.
+  content::BrowserTaskEnvironment task_environment_;
+
+  std::unique_ptr<crosapi::FakeBrowserManager> browser_manager_;
+
+  // Used to verify that OnGetFeedbackData was called the correct number of
+  // times.
+  int num_callback_calls_ = 0;
+
+  // Stores results from the log source passed into fetch_callback().
+  SystemLogsResponse response_;
+};
+
+TEST_F(CrosapiSystemLogSourceTest, OnFeedbackData) {
+  // Set up FakeBrowserManager to send log data with 1 log entry for
+  // the log source.
+  base::Value system_log_entries(base::Value::Type::DICTIONARY);
+  system_log_entries.SetStringKey("testing log key", "testing log content");
+  SetupBrowserManagerResponse(std::move(system_log_entries));
+
+  // Fetch log data and wait until fetch_callback() is called.
+  CrosapiSystemLogSource source;
+  source.Fetch(fetch_callback());
+  base::RunLoop().RunUntilIdle();
+
+  // Verify fetch_callback() has been called.
+  EXPECT_EQ(1, num_callback_calls());
+  ASSERT_EQ(1U, response().size());
+
+  // Verify CrosapiSystemLogSource::OnGetFeedbackData processed log data
+  // correctly. The Lacros log entry in the response() should have "Lacros "
+  // prefix in log entry key.
+  EXPECT_EQ("Lacros testing log key", response().begin()->first);
+  EXPECT_EQ("testing log content", response().begin()->second);
+
+  // Simulate the mojo disconnection event received after fetch_callback() runs.
+  SignalMojoDisconnected();
+
+  // Verify fetch_callback() has not been called again.
+  EXPECT_EQ(1, num_callback_calls());
+}
+
+TEST_F(CrosapiSystemLogSourceTest, FetchWhenLacrosNotRunning) {
+  // Set up FakeBrowserManager with Lacros not running.
+  SetLacrosNotRunning();
+
+  // Fetch log data and wait until fetch_callback() is called.
+  CrosapiSystemLogSource source;
+  source.Fetch(fetch_callback());
+  base::RunLoop().RunUntilIdle();
+
+  // Verify fetch_callback() has been called with empty response.
+  EXPECT_EQ(1, num_callback_calls());
+  ASSERT_EQ(0U, response().size());
+
+  SignalMojoDisconnected();
+
+  // Verify fetch_callback() has not been called again.
+  EXPECT_EQ(1, num_callback_calls());
+}
+
+TEST_F(CrosapiSystemLogSourceTest, OnMojoDisconnectedBeforeLogFetched) {
+  // Set up FakeBrowserManager to wait for crosapi mojo disconnected event
+  // before sending log data back, so that we can simulate the case
+  // that crosapi connection will be disconnected before the log data
+  // is fetched, i.e. before OnGetFeedbackData is called.
+  SetWaitForMojoDisconnect();
+
+  // Fetch log data.
+  CrosapiSystemLogSource source;
+  source.Fetch(fetch_callback());
+  base::RunLoop().RunUntilIdle();
+
+  // Verify fetch_callback() is not called.
+  EXPECT_EQ(0, num_callback_calls());
+  ASSERT_EQ(0U, response().size());
+
+  // Simulate the mojo disconnection event received by FakeBrowserManager.
+  SignalMojoDisconnected();
+
+  // Verify fetch_callback() is called with empty response.
+  EXPECT_EQ(1, num_callback_calls());
+  ASSERT_EQ(0U, response().size());
+}
+
+}  // namespace system_logs

chrome/browser/feedback/system_logs/chrome_system_logs_fetcher.cc

@@ -15,8 +15,10 @@
 #include "components/feedback/system_logs/system_logs_fetcher.h"
 
 #if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/crosapi/browser_manager.h"
 #include "chrome/browser/chromeos/crosapi/browser_util.h"
 #include "chrome/browser/chromeos/system_logs/command_line_log_source.h"
+#include "chrome/browser/chromeos/system_logs/crosapi_system_log_source.h"
 #include "chrome/browser/chromeos/system_logs/dbus_log_source.h"
 #include "chrome/browser/chromeos/system_logs/debug_daemon_log_source.h"
 #include "chrome/browser/chromeos/system_logs/device_event_log_source.h"
@@ -64,6 +66,14 @@ SystemLogsFetcher* BuildChromeSystemLogsFetcher(bool scrub_data) {
   fetcher->AddSource(std::make_unique<NetworkHealthSource>(scrub_data));
   fetcher->AddSource(std::make_unique<ShillLogSource>(scrub_data));
   fetcher->AddSource(std::make_unique<UiHierarchyLogSource>(scrub_data));
+
+  // Add CrosapiSystemLogSource to get lacros system information log data
+  // if Lacros is running and the crosapi version supports the Lacros remote
+  // data source.
+  if (crosapi::BrowserManager::Get()->IsRunning() &&
+      crosapi::BrowserManager::Get()->GetFeedbackDataSupported()) {
+    fetcher->AddSource(std::make_unique<CrosapiSystemLogSource>());
+  }
 #endif
 
 #if BUILDFLAG(IS_LACROS)

chrome/browser/lacros/feedback_util.cc

+// 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.
+
+#include "chrome/browser/lacros/feedback_util.h"
+
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/signin/identity_manager_factory.h"
+#include "components/signin/public/identity_manager/identity_manager.h"
+
+namespace feedback_util {
+
+std::string GetSignedInUserEmail() {
+  Profile* profile = ProfileManager::GetLastUsedProfileAllowedByPolicy();
+  DCHECK(profile) << "No last used profile is found.";
+
+  auto* identity_manager = IdentityManagerFactory::GetForProfile(profile);
+  if (!identity_manager)
+    return std::string();
+  // Browser sync consent is not required to use feedback.
+  return identity_manager
+      ->GetPrimaryAccountInfo(signin::ConsentLevel::kNotRequired)
+      .email;
+}
+
+}  // namespace feedback_util

chrome/browser/lacros/feedback_util.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 CHROME_BROWSER_LACROS_FEEDBACK_UTIL_H_
+#define CHROME_BROWSER_LACROS_FEEDBACK_UTIL_H_
+
+#include <string>
+
+namespace feedback_util {
+
+// Returns the email of the signed-in user.
+std::string GetSignedInUserEmail();
+
+}  // namespace feedback_util
+
+#endif  // CHROME_BROWSER_LACROS_FEEDBACK_UTIL_H_

chrome/browser/lacros/lacros_chrome_service_delegate_impl.cc

@@ -5,10 +5,15 @@
 #include "chrome/browser/lacros/lacros_chrome_service_delegate_impl.h"
 
 #include "base/logging.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"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/common/channel_info.h"
+#include "components/feedback/feedback_report.h"
+#include "components/feedback/system_logs/system_logs_fetcher.h"
+#include "google_apis/gaia/gaia_auth_util.h"
 
 LacrosChromeServiceDelegateImpl::LacrosChromeServiceDelegateImpl() = default;
 
@@ -24,3 +29,46 @@ void LacrosChromeServiceDelegateImpl::NewWindow() {
 std::string LacrosChromeServiceDelegateImpl::GetChromeVersion() {
   return chrome::GetVersionString();
 }
+
+void LacrosChromeServiceDelegateImpl::GetFeedbackData(
+    GetFeedbackDataCallback callback) {
+  DCHECK(!callback.is_null());
+  DCHECK(get_feedback_data_callback_.is_null());
+  get_feedback_data_callback_ = std::move(callback);
+
+  // Self-deleting object.
+  system_logs::SystemLogsFetcher* fetcher =
+      system_logs::BuildLacrosSystemLogsFetcher(/*scrub_data=*/true);
+  fetcher->Fetch(
+      base::BindOnce(&LacrosChromeServiceDelegateImpl::OnSystemInformationReady,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void LacrosChromeServiceDelegateImpl::OnSystemInformationReady(
+    std::unique_ptr<system_logs::SystemLogsResponse> sys_info) {
+  base::Value system_log_entries(base::Value::Type::DICTIONARY);
+  if (sys_info) {
+    std::string user_email = feedback_util::GetSignedInUserEmail();
+    const bool google_email = gaia::IsGoogleInternalAccountEmail(user_email);
+
+    for (auto& it : *sys_info) {
+      // TODO(crbug.com/1138703): This code is duplicated with the logic in
+      // feedback_private_api.cc, refactor to remove the duplicated code.
+      // We only send the list of all the crash report IDs if the user has a
+      // @google.com email. We strip this here so that the system information
+      // view properly reflects what we will be uploading to the server. It is
+      // also stripped later on in the feedback processing for other code paths
+      // that don't go through this.
+      if (it.first == feedback::FeedbackReport::kAllCrashReportIdsKey &&
+          !google_email) {
+        continue;
+      }
+
+      system_log_entries.SetStringKey(std::move(it.first),
+                                      std::move(it.second));
+    }
+
+    DCHECK(!get_feedback_data_callback_.is_null());
+    std::move(get_feedback_data_callback_).Run(std::move(system_log_entries));
+  }
+}

chrome/browser/lacros/lacros_chrome_service_delegate_impl.h

@@ -5,7 +5,9 @@
 #ifndef CHROME_BROWSER_LACROS_LACROS_CHROME_SERVICE_DELEGATE_IMPL_H_
 #define CHROME_BROWSER_LACROS_LACROS_CHROME_SERVICE_DELEGATE_IMPL_H_
 
+#include "base/memory/weak_ptr.h"
 #include "chromeos/lacros/lacros_chrome_service_delegate.h"
+#include "components/feedback/system_logs/system_logs_source.h"
 
 // Chrome implementation of LacrosChromeServiceDelegate.
 class LacrosChromeServiceDelegateImpl
@@ -21,6 +23,14 @@ class LacrosChromeServiceDelegateImpl
   // chromeos::LacrosChromeServiceDelegate:
   void NewWindow() override;
   std::string GetChromeVersion() override;
+  void GetFeedbackData(GetFeedbackDataCallback callback) override;
+
+ private:
+  void OnSystemInformationReady(
+      std::unique_ptr<system_logs::SystemLogsResponse> sys_info);
+
+  GetFeedbackDataCallback get_feedback_data_callback_;
+  base::WeakPtrFactory<LacrosChromeServiceDelegateImpl> weak_ptr_factory_{this};
 };
 
 #endif  // CHROME_BROWSER_LACROS_LACROS_CHROME_SERVICE_DELEGATE_IMPL_H_

chrome/browser/lacros/system_logs/lacros_system_log_fetcher.cc

+// 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.
+
+#include "chrome/browser/lacros/system_logs/lacros_system_log_fetcher.h"
+
+#include "chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.h"
+#include "chrome/browser/feedback/system_logs/log_sources/crash_ids_source.h"
+#include "chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.h"
+#include "chrome/common/extensions/extension_constants.h"
+#include "components/feedback/system_logs/system_logs_fetcher.h"
+
+namespace system_logs {
+
+SystemLogsFetcher* BuildLacrosSystemLogsFetcher(bool scrub_data) {
+  SystemLogsFetcher* fetcher = new SystemLogsFetcher(
+      scrub_data, extension_misc::kBuiltInFirstPartyExtensionIds);
+
+  fetcher->AddSource(std::make_unique<ChromeInternalLogSource>());
+  fetcher->AddSource(std::make_unique<CrashIdsSource>());
+  fetcher->AddSource(std::make_unique<MemoryDetailsLogSource>());
+
+  return fetcher;
+}
+
+}  // namespace system_logs

chrome/browser/lacros/system_logs/lacros_system_log_fetcher.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 CHROME_BROWSER_LACROS_SYSTEM_LOGS_LACROS_SYSTEM_LOG_FETCHER_H_
+#define CHROME_BROWSER_LACROS_SYSTEM_LOGS_LACROS_SYSTEM_LOG_FETCHER_H_
+
+namespace system_logs {
+
+class SystemLogsFetcher;
+
+// Creates a SystemLogsFetcher to aggregate the scrubbed lacros logs for sending
+// with unified feedback reports. If |scrub_data| is true then the logs are
+// scrubbed of PII.
+// The fetcher deletes itself once it finishes fetching data.
+SystemLogsFetcher* BuildLacrosSystemLogsFetcher(bool scrub_data);
+
+}  // namespace system_logs
+
+#endif  // CHROME_BROWSER_LACROS_SYSTEM_LOGS_LACROS_SYSTEM_LOG_FETCHER_H_

chromeos/crosapi/mojom/crosapi.mojom

@@ -11,6 +11,7 @@ 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/token.mojom";
+import "mojo/public/mojom/base/values.mojom";
 import "services/device/public/mojom/hid.mojom";
 
 // LacrosInfo is a set of parameters passed to ash from lacros-chrome
@@ -173,4 +174,12 @@ interface LacrosChromeService {
   // Opens a new window in lacros-chrome with, currently, the last used profile.
   // The callback is called on the command execution.
   NewWindow@1() => ();
+
+  // Returns lacros feedback data used for generating feedback report in
+  // a dictionary object, each entry representing a log entry in the feedback
+  // report, for example, "Lacros crash_report_ids: xxx",
+  // "Lacros extensions: xxx", "Lacros mem_usage: xxx", etc.
+  [MinVersion=6]
+  GetFeedbackData@3() => (
+      mojo_base.mojom.DictionaryValue feedback_info);
 };

chromeos/lacros/BUILD.gn

@@ -18,6 +18,7 @@ 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,6 +7,8 @@
 
 #include <string>
 
+#include "chromeos/lacros/get_feedback_data_callback.h"
+
 namespace chromeos {
 
 // Interface to inject Chrome dependent behavior into LacrosChromeServiceImpl
@@ -23,6 +25,9 @@ class LacrosChromeServiceDelegate {
   // {browser version} {channel}
   // For example, "87.0.0.1 dev", "86.0.4240.38 beta".
   virtual std::string GetChromeVersion() = 0;
+
+  // Gets lacros feedback data.
+  virtual void GetFeedbackData(GetFeedbackDataCallback callback) = 0;
 };
 
 }  // namespace chromeos

chromeos/lacros/lacros_chrome_service_impl.cc

@@ -75,6 +75,13 @@ class LacrosChromeServiceNeverBlockingState
         std::move(callback));
   }
 
+  void GetFeedbackData(GetFeedbackDataCallback callback) override {
+    owner_sequence_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&LacrosChromeServiceImpl::GetFeedbackDataAffineSequence,
+                       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
@@ -388,6 +395,12 @@ void LacrosChromeServiceImpl::NewWindowAffineSequence() {
   delegate_->NewWindow();
 }
 
+void LacrosChromeServiceImpl::GetFeedbackDataAffineSequence(
+    GetFeedbackDataCallback callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
+  delegate_->GetFeedbackData(std::move(callback));
+}
+
 int LacrosChromeServiceImpl::AshChromeServiceVersion() {
   if (g_disable_all_crosapi_for_tests)
     return -1;

chromeos/lacros/lacros_chrome_service_impl.h

@@ -19,6 +19,7 @@
 #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"
@@ -180,6 +181,9 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   // Creates a new window on the affine sequence.
   void NewWindowAffineSequence();
 
+  // Gets feedback data on the affine sequence.
+  void GetFeedbackDataAffineSequence(GetFeedbackDataCallback 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().

mojo/public/mojom/base/values.mojom

@@ -8,6 +8,7 @@ module mojo_base.mojom;
 //
 // One notable caveat is that Value supports arbitrary binary data, which JSON
 // does not support natively.
+[Stable]
 union Value {
   // Null type placeholder. This field is never used.
   uint8 null_value;
@@ -29,10 +30,12 @@ union Value {
 // declaration. Though both of these types are mapped to base::Value in C++,
 // the generated deserialization will guarantee that the method is only invoked
 // with a base::Value of the correct subtype.
+[Stable]
 struct DictionaryValue {
   map<string, Value> storage;
 };
 
+[Stable]
 struct ListValue {
   array<Value> storage;
 };