Skip to content

Commit

Permalink
Fix disk space logging in ARC++ and extension install events
Browse files Browse the repository at this point in the history 
DiskMountManager depends on dbus calls to cros-disks. A recent change crrev.com/c/2422018 blocked access to internal disks through cros-disks.

BUG=b:173102552

Change-Id: I4c368f3f934371bbb3d5a777a82fdf3721d67eb4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2537535
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Reviewed-by: Sergei Datsenko <dats@chromium.org>
Commit-Queue: Yi Xie <yixie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828833}
  • Loading branch information
@imxieyi
imxieyi authored and Commit Bot committed Nov 18, 2020
1 parent 0b50dab commit 9ee0b74
Show file tree
Hide file tree
Showing 8 changed files with 223 additions and 108 deletions.
40 changes: 20 additions & 20 deletions chrome/browser/chromeos/policy/app_install_event_logger.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
#include "chrome/browser/policy/profile_policy_connector.h"
#include "chrome/browser/profiles/profile.h"
#include "chromeos/disks/disk.h"
#include "chromeos/disks/disk_mount_manager.h"
#include "components/arc/arc_prefs.h"
#include "components/policy/core/common/policy_map.h"
#include "components/policy/core/common/policy_namespace.h"
Expand Down Expand Up @@ -66,24 +65,17 @@ std::set<std::string> GetDifference(const std::set<std::string>& first,
}

std::unique_ptr<em::AppInstallReportLogEvent> AddDiskSpaceInfoToEvent(
std::unique_ptr<em::AppInstallReportLogEvent> event) {
for (const auto& disk :
chromeos::disks::DiskMountManager::GetInstance()->disks()) {
if (!disk.second->IsStatefulPartition()) {
continue;
}
const base::FilePath stateful_path(disk.second->mount_path());
const int64_t stateful_total =
base::SysInfo::AmountOfTotalDiskSpace(stateful_path);
if (stateful_total >= 0) {
event->set_stateful_total(stateful_total);
}
const int64_t stateful_free =
base::SysInfo::AmountOfFreeDiskSpace(stateful_path);
if (stateful_free >= 0) {
event->set_stateful_free(stateful_free);
}
break;
std::unique_ptr<em::AppInstallReportLogEvent> event,
const base::FilePath& stateful_path) {
const int64_t stateful_total =
base::SysInfo::AmountOfTotalDiskSpace(stateful_path);
if (stateful_total >= 0) {
event->set_stateful_total(stateful_total);
}
const int64_t stateful_free =
base::SysInfo::AmountOfFreeDiskSpace(stateful_path);
if (stateful_free >= 0) {
event->set_stateful_free(stateful_free);
}
return event;
}
Expand Down Expand Up @@ -128,6 +120,8 @@ AppInstallEventLogger::AppInstallEventLogger(Delegate* delegate,
arc::ArcPolicyBridge::GetForBrowserContext(profile_);
bridge->AddObserver(this);
policy_service->AddObserver(policy::POLICY_DOMAIN_CHROME, this);

stateful_path_ = chromeos::disks::GetStatefulPartitionPath();
}

AppInstallEventLogger::~AppInstallEventLogger() {
Expand Down Expand Up @@ -230,6 +224,11 @@ void AppInstallEventLogger::OnComplianceReportReceived(
}
}

void AppInstallEventLogger::SetStatefulPathForTesting(
const base::FilePath& path) {
stateful_path_ = path;
}

std::set<std::string> AppInstallEventLogger::GetPackagesFromPref(
const std::string& pref_name) const {
std::set<std::string> packages;
Expand Down Expand Up @@ -305,7 +304,8 @@ void AppInstallEventLogger::AddForSetOfPackagesWithDiskSpaceInfo(
std::unique_ptr<em::AppInstallReportLogEvent> event) {
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE, {base::MayBlock()},
base::BindOnce(&AddDiskSpaceInfoToEvent, std::move(event)),
base::BindOnce(&AddDiskSpaceInfoToEvent, std::move(event),
stateful_path_),
base::BindOnce(&AppInstallEventLogger::AddForSetOfPackages,
weak_factory_.GetWeakPtr(), packages));
}
Expand Down
7 changes: 7 additions & 0 deletions chrome/browser/chromeos/policy/app_install_event_logger.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <set>
#include <string>

#include "base/files/file_path.h"
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "chrome/browser/chromeos/arc/policy/arc_policy_bridge.h"
Expand Down Expand Up @@ -98,6 +99,9 @@ class AppInstallEventLogger : public AppInstallEventLogCollector::Delegate,
void OnComplianceReportReceived(
const base::Value* compliance_report) override;

// Set stateful partition path for unit tests.
void SetStatefulPathForTesting(const base::FilePath& path);

private:
// Loads a list of packages from a pref.
std::set<std::string> GetPackagesFromPref(const std::string& pref_name) const;
Expand Down Expand Up @@ -146,6 +150,9 @@ class AppInstallEventLogger : public AppInstallEventLogCollector::Delegate,
// remove itself as observer in the destructor.
bool observing_ = false;

// Path for stateful partition.
base::FilePath stateful_path_;

// The app push-install requests that were most recently sent to CloudDPC.
std::set<std::string> requested_in_arc_;

Expand Down
126 changes: 84 additions & 42 deletions chrome/browser/chromeos/policy/app_install_event_logger_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <stdint.h>

#include "base/files/file_path.h"
#include "base/json/json_writer.h"
#include "base/time/time.h"
#include "base/values.h"
Expand All @@ -28,6 +29,7 @@
#include "testing/gtest/include/gtest/gtest.h"

using testing::_;
using testing::DoAll;
using testing::Invoke;
using testing::Mock;
using testing::WithArgs;
Expand All @@ -38,7 +40,7 @@ namespace policy {

namespace {

constexpr char kStatefulMountPath[] = "/mnt/stateful_partition";
constexpr char kStatefulPath[] = "/tmp";
constexpr char kPackageName[] = "com.example.app";
constexpr char kPackageName2[] = "com.example.app2";
constexpr char kPackageName3[] = "com.example.app3";
Expand All @@ -64,6 +66,38 @@ MATCHER_P(MatchEventExceptTimestamp, expected, "event matches") {
expected_event.SerializePartialAsString();
}

MATCHER_P(MatchEventExceptDiskSpace, expected, "event matches") {
em::AppInstallReportLogEvent actual_event;
actual_event.MergeFrom(arg);
actual_event.clear_stateful_total();
actual_event.clear_stateful_free();

em::AppInstallReportLogEvent expected_event;
expected_event.MergeFrom(expected);
expected_event.clear_stateful_total();
expected_event.clear_stateful_free();

return actual_event.SerializePartialAsString() ==
expected_event.SerializePartialAsString();
}

MATCHER_P(MatchEventExceptTimestampAndDiskSpace, expected, "event matches") {
em::AppInstallReportLogEvent actual_event;
actual_event.MergeFrom(arg);
actual_event.clear_timestamp();
actual_event.clear_stateful_total();
actual_event.clear_stateful_free();

em::AppInstallReportLogEvent expected_event;
expected_event.MergeFrom(expected);
expected_event.clear_timestamp();
expected_event.clear_stateful_total();
expected_event.clear_stateful_free();

return actual_event.SerializePartialAsString() ==
expected_event.SerializePartialAsString();
}

ACTION_TEMPLATE(SaveTimestamp,
HAS_1_TEMPLATE_PARAMS(int, k),
AND_1_VALUE_PARAMS(out)) {
Expand All @@ -76,6 +110,18 @@ ACTION_TEMPLATE(SaveAndroidId,
*out = testing::get<k>(args).android_id();
}

ACTION_TEMPLATE(SaveStatefulTotal,
HAS_1_TEMPLATE_PARAMS(int, k),
AND_1_VALUE_PARAMS(out)) {
*out = testing::get<k>(args).stateful_total();
}

ACTION_TEMPLATE(SaveStatefulFree,
HAS_1_TEMPLATE_PARAMS(int, k),
AND_1_VALUE_PARAMS(out)) {
*out = testing::get<k>(args).stateful_free();
}

int64_t GetCurrentTimestamp() {
return (base::Time::Now() - base::Time::UnixEpoch()).InMicroseconds();
}
Expand Down Expand Up @@ -120,18 +166,6 @@ class AppInstallEventLoggerTest : public testing::Test {
chromeos::PowerManagerClient::InitializeFake();

chromeos::NetworkHandler::Initialize();

disk_mount_manager_ = new chromeos::disks::MockDiskMountManager;
chromeos::disks::DiskMountManager::InitializeForTesting(
disk_mount_manager_);
disk_mount_manager_->CreateDiskEntryForMountDevice(
chromeos::disks::DiskMountManager::MountPointInfo(
"/dummy/device/usb", kStatefulMountPath,
chromeos::MOUNT_TYPE_DEVICE, chromeos::disks::MOUNT_CONDITION_NONE),
"device_id", "device_label", "vendor", "product",
chromeos::DEVICE_TYPE_UNKNOWN, 1 << 20 /* total_size_in_bytes */,
false /* is_parent */, false /* has_media */, true /* on_boot_device */,
true /* on_removable_device */, "ext4");
}

void TearDown() override {
Expand All @@ -140,7 +174,6 @@ class AppInstallEventLoggerTest : public testing::Test {
chromeos::PowerManagerClient::Shutdown();
chromeos::NetworkHandler::Shutdown();
chromeos::DBusThreadManager::Shutdown();
chromeos::disks::DiskMountManager::Shutdown();
TestingBrowserProcess::GetGlobal()->SetLocalState(nullptr);
}

Expand Down Expand Up @@ -192,9 +225,6 @@ class AppInstallEventLoggerTest : public testing::Test {
TestingProfile profile_;
TestingPrefServiceSimple pref_service_;

// Owned by chromeos::disks::DiskMountManager.
chromeos::disks::MockDiskMountManager* disk_mount_manager_ = nullptr;

MockAppInstallEventLoggerDelegate delegate_;

em::AppInstallReportLogEvent event_;
Expand Down Expand Up @@ -297,72 +327,78 @@ TEST_F(AppInstallEventLoggerTest, DoesNotAddsAndroidId) {
}

// Adds an event with a timestamp, requesting that disk space information be
// added to it. Verifies that a background task is posted that consults the disk
// mount manager. Then, verifies that after the background task has run, the
// event is added.
//
// It is not possible to test that disk size information is retrieved correctly
// as a mounted stateful partition cannot be simulated in unit tests.
// added to it. Verifies that after the background task has run, the event is
// added with valid disk space info.
TEST_F(AppInstallEventLoggerTest, AddSetsDiskSpaceInfo) {
CreateLogger();
logger_->SetStatefulPathForTesting(base::FilePath(kStatefulPath));

event_.set_timestamp(kTimestamp);
std::unique_ptr<em::AppInstallReportLogEvent> event =
std::make_unique<em::AppInstallReportLogEvent>();
event->MergeFrom(event_);
event->clear_stateful_total();
event->clear_stateful_free();

EXPECT_CALL(*disk_mount_manager_, disks()).Times(0);
EXPECT_CALL(delegate_, Add(_, _)).Times(0);
logger_->Add(kPackageName, true /* gather_disk_space_info */,
std::move(event));
Mock::VerifyAndClearExpectations(disk_mount_manager_);
Mock::VerifyAndClearExpectations(&delegate_);

EXPECT_CALL(*disk_mount_manager_, disks());
int64_t stateful_total = 0;
int64_t stateful_free = 0;
SetAndroidId(kAndroidId);
EXPECT_CALL(delegate_,
Add(std::set<std::string>{kPackageName}, MatchProto(event_)));
EXPECT_CALL(delegate_, Add(std::set<std::string>{kPackageName},
MatchEventExceptDiskSpace(event_)))
.WillOnce(DoAll(SaveStatefulTotal<1>(&stateful_total),
SaveStatefulFree<1>(&stateful_free)));
task_environment_.RunUntilIdle();

EXPECT_GT(stateful_total, 0);
EXPECT_GT(stateful_free, 0);
}

// Adds an event without a timestamp, requesting that disk space information be
// added to it. Verifies that a background task is posted that consults the disk
// mount manager. Then, verifies that after the background task has run, the
// event is added and its timestamp is set to the current time before posting
// the background task.
//
// It is not possible to test that disk size information is retrieved correctly
// as a mounted stateful partition cannot be simulated in unit tests.
// added to it. Verifies that after the background task has run, the event is
// added with valid disk space info and its timestamp is set to the current time
// before posting the background task.
TEST_F(AppInstallEventLoggerTest, AddSetsTimestampAndDiskSpaceInfo) {
CreateLogger();
logger_->SetStatefulPathForTesting(base::FilePath(kStatefulPath));

std::unique_ptr<em::AppInstallReportLogEvent> event =
std::make_unique<em::AppInstallReportLogEvent>();
event->MergeFrom(event_);
event->clear_stateful_total();
event->clear_stateful_free();

EXPECT_CALL(*disk_mount_manager_, disks()).Times(0);
EXPECT_CALL(delegate_, Add(_, _)).Times(0);
const int64_t before = GetCurrentTimestamp();
logger_->Add(kPackageName, true /* gather_disk_space_info */,
std::move(event));
const int64_t after = GetCurrentTimestamp();
Mock::VerifyAndClearExpectations(disk_mount_manager_);
Mock::VerifyAndClearExpectations(&delegate_);

int64_t timestamp = 0;
EXPECT_CALL(*disk_mount_manager_, disks());
int64_t stateful_total = 0;
int64_t stateful_free = 0;
SetAndroidId(kAndroidId);
EXPECT_CALL(delegate_, Add(std::set<std::string>{kPackageName},
MatchEventExceptTimestamp(event_)))
.WillOnce(SaveTimestamp<1>(&timestamp));
MatchEventExceptTimestampAndDiskSpace(event_)))
.WillOnce(DoAll(SaveTimestamp<1>(&timestamp),
SaveStatefulTotal<1>(&stateful_total),
SaveStatefulFree<1>(&stateful_free)));
task_environment_.RunUntilIdle();

EXPECT_GT(stateful_total, 0);
EXPECT_GT(stateful_free, 0);
EXPECT_LE(before, timestamp);
EXPECT_GE(after, timestamp);
}

TEST_F(AppInstallEventLoggerTest, UpdatePolicy) {
CreateLogger();
logger_->SetStatefulPathForTesting(base::FilePath(kStatefulPath));

policy::PolicyMap new_policy_map;

Expand Down Expand Up @@ -410,13 +446,19 @@ TEST_F(AppInstallEventLoggerTest, UpdatePolicy) {

// Expected new packages added with disk info.
event_.set_event_type(em::AppInstallReportLogEvent::SERVER_REQUEST);
int64_t stateful_total = 0;
int64_t stateful_free = 0;
SetAndroidId(kAndroidId);
EXPECT_CALL(delegate_, Add(std::set<std::string>{kPackageName, kPackageName3},
MatchEventExceptTimestamp(event_)));
EXPECT_CALL(*disk_mount_manager_, disks());
MatchEventExceptTimestampAndDiskSpace(event_)))
.WillOnce(DoAll(SaveStatefulTotal<1>(&stateful_total),
SaveStatefulFree<1>(&stateful_free)));
task_environment_.RunUntilIdle();
Mock::VerifyAndClearExpectations(&delegate_);

EXPECT_GT(stateful_total, 0);
EXPECT_GT(stateful_free, 0);

// To avoid extra logging.
g_browser_process->local_state()->SetBoolean(prefs::kWasRestarted, true);
}
Expand Down
Loading

0 comments on commit 9ee0b74

Please sign in to comment.