Skip to content

Commit

Permalink
Separate Kiosk extension install error for off store extensions
Browse files Browse the repository at this point in the history 
According to data from Extensions.*_ForceInstalledFailureReason3 UMA
extension install failure reasons differ significantly between Web Store
and off store extensions. We want to separate them for
Kiosk.Extensions.InstallError as well.

Bug: b:221407736
Change-Id: Ic909268be3d74f1b641181e7694836c6823e2f09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3571924
Reviewed-by: Polina Bondarenko <pbond@chromium.org>
Reviewed-by: Caitlin Fischer <caitlinfischer@google.com>
Reviewed-by: Oleg Davydov <burunduk@chromium.org>
Commit-Queue: Yi Xie <yixie@chromium.org>
Cr-Commit-Position: refs/heads/main@{#990177}
  • Loading branch information
@imxieyi
imxieyi authored and Chromium LUCI CQ committed Apr 7, 2022
1 parent 421a263 commit 6465104
Show file tree
Hide file tree
Showing 7 changed files with 83 additions and 21 deletions.
18 changes: 12 additions & 6 deletions chrome/browser/ash/login/app_mode/kiosk_launch_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,15 @@ void RecordKioskLaunchUMA(bool is_auto_launch) {
}

void RecordKioskExtensionInstallError(
extensions::InstallStageTracker::FailureReason reason) {
UMA_HISTOGRAM_ENUMERATION(
"Kiosk.Extensions.InstallError", reason,
extensions::InstallStageTracker::FailureReason::kMaxValue);
extensions::InstallStageTracker::FailureReason reason,
bool is_from_store) {
if (is_from_store) {
base::UmaHistogramEnumeration("Kiosk.Extensions.InstallError.WebStore",
reason);
} else {
base::UmaHistogramEnumeration("Kiosk.Extensions.InstallError.OffStore",
reason);
}
}

void RecordKioskExtensionInstallTimedOut(bool timeout) {
Expand Down Expand Up @@ -648,8 +653,9 @@ void KioskLaunchController::OnForceInstalledExtensionsReady() {

void KioskLaunchController::OnForceInstalledExtensionFailed(
const extensions::ExtensionId& extension_id,
extensions::InstallStageTracker::FailureReason reason) {
RecordKioskExtensionInstallError(reason);
extensions::InstallStageTracker::FailureReason reason,
bool is_from_store) {
RecordKioskExtensionInstallError(reason, is_from_store);
}

void KioskLaunchController::OnOwnerSigninSuccess() {
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/ash/login/app_mode/kiosk_launch_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ class KioskLaunchController
void OnForceInstalledExtensionsReady() override;
void OnForceInstalledExtensionFailed(
const extensions::ExtensionId& extension_id,
extensions::InstallStageTracker::FailureReason reason) override;
extensions::InstallStageTracker::FailureReason reason,
bool is_from_store) override;

void OnOwnerSigninSuccess();

Expand Down
63 changes: 55 additions & 8 deletions chrome/browser/ash/login/app_mode/kiosk_launch_controller_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,13 @@ const char kExtensionName[] = "extension_name";
const char kInvalidExtensionId[] = "invalid_id";
const char kInvalidExtensionName[] = "invalid_name";

// URL of Chrome Web.
const char kExtensionUpdateUrl[] =
// URL of Chrome Web Store.
const char kWebStoreExtensionUpdateUrl[] =
"https://clients2.google.com/service/update2/crx";

// URL of off store extensions.
const char kOffStoreExtensionUpdateUrl[] = "https://example.com/crx";

class MockKioskProfileLoadFailedObserver
: public KioskLaunchController::KioskProfileLoadFailedObserver {
public:
Expand Down Expand Up @@ -373,20 +376,21 @@ INSTANTIATE_TEST_SUITE_P(All,
class KioskLaunchControllerWithExtensionTest
: public KioskLaunchControllerTest {
public:
void SetupForceList(const std::string& extension_id) {
void SetupForceList(const std::string& extension_id,
const std::string& update_url) {
std::unique_ptr<base::Value> dict =
extensions::DictionaryBuilder()
.Set(extension_id,
extensions::DictionaryBuilder()
.Set(extensions::ExternalProviderImpl::kExternalUpdateUrl,
kExtensionUpdateUrl)
update_url)
.Build())
.Build();
ProfileManager::GetPrimaryUserProfile()->GetPrefs()->Set(
extensions::pref_names::kInstallForceList, std::move(*dict));

base::Value list(base::Value::Type::LIST);
list.Append(base::StrCat({extension_id, ";", kExtensionUpdateUrl}));
list.Append(base::StrCat({extension_id, ";", update_url}));
policy::PolicyMap map;
map.Set(policy::key::kExtensionInstallForcelist,
policy::POLICY_LEVEL_MANDATORY, policy::POLICY_SCOPE_USER,
Expand All @@ -401,7 +405,7 @@ class KioskLaunchControllerWithExtensionTest
}

void PreRunTestOnMainThread() override {
SetupForceList(kExtensionId);
SetupForceList(kExtensionId, kWebStoreExtensionUpdateUrl);
InProcessBrowserTest::PreRunTestOnMainThread();
}

Expand Down Expand Up @@ -518,7 +522,7 @@ IN_PROC_BROWSER_TEST_P(KioskLaunchControllerWithExtensionTest,
content::FetchHistogramsFromChildProcesses();
metrics::SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
histogram.ExpectUniqueSample(
"Kiosk.Extensions.InstallError",
"Kiosk.Extensions.InstallError.WebStore",
extensions::InstallStageTracker::FailureReason::INVALID_ID, 1);
}

Expand All @@ -541,7 +545,7 @@ class KioskLaunchControllerWithInvalidExtensionTest
}

void PreRunTestOnMainThread() override {
SetupForceList(kInvalidExtensionId);
SetupForceList(kInvalidExtensionId, kWebStoreExtensionUpdateUrl);
InProcessBrowserTest::PreRunTestOnMainThread();
}
};
Expand All @@ -568,4 +572,47 @@ INSTANTIATE_TEST_SUITE_P(All,
KioskAppType::kChromeApp,
KioskAppType::kWebApp));

class KioskLaunchControllerWithOffStoreExtensionTest
: public KioskLaunchControllerWithExtensionTest {
public:
void PreRunTestOnMainThread() override {
SetupForceList(kExtensionId, kOffStoreExtensionUpdateUrl);
InProcessBrowserTest::PreRunTestOnMainThread();
}
};

IN_PROC_BROWSER_TEST_P(KioskLaunchControllerWithOffStoreExtensionTest,
ExtensionFailedAfterAppPrepared) {
base::HistogramTester histogram;

RunUntilAppPrepared();
ExpectState(AppState::kInstallingExtensions, NetworkUIState::kNotShowing);
ExpectViewState(
AppLaunchSplashScreenView::AppLaunchState::kInstallingExtension);

SetExtensionFailed(
kExtensionId, kExtensionName,
extensions::InstallStageTracker::FailureReason::INVALID_ID);

EXPECT_CALL(*launcher(), LaunchApp()).Times(1);
FireSplashScreenTimer();

launch_controls()->OnAppLaunched();
ExpectState(AppState::kLaunched, NetworkUIState::kNotShowing);
ExpectViewState(AppLaunchSplashScreenView::AppLaunchState::kWaitingAppWindow);
EXPECT_TRUE(session_manager::SessionManager::Get()->IsSessionStarted());

content::FetchHistogramsFromChildProcesses();
metrics::SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
histogram.ExpectUniqueSample(
"Kiosk.Extensions.InstallError.OffStore",
extensions::InstallStageTracker::FailureReason::INVALID_ID, 1);
}

INSTANTIATE_TEST_SUITE_P(All,
KioskLaunchControllerWithOffStoreExtensionTest,
testing::Values(KioskAppType::kArcApp,
KioskAppType::kChromeApp,
KioskAppType::kWebApp));

} // namespace ash
3 changes: 2 additions & 1 deletion chrome/browser/extensions/forced_extensions/force_installed_tracker.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,9 @@ void ForceInstalledTracker::OnExtensionInstallationFailed(
item->second.status == ExtensionStatus::kReady)
return;
ChangeExtensionStatus(extension_id, ExtensionStatus::kFailed);
bool is_from_store = item->second.is_from_store;
for (auto& obs : observers_)
obs.OnForceInstalledExtensionFailed(extension_id, reason);
obs.OnForceInstalledExtensionFailed(extension_id, reason, is_from_store);
MaybeNotifyObservers();
}

Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/extensions/forced_extensions/force_installed_tracker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ class ForceInstalledTracker : public ExtensionRegistryObserver,
// Can be called multiple times, one for each failed extension install.
virtual void OnForceInstalledExtensionFailed(
const ExtensionId& extension_id,
InstallStageTracker::FailureReason reason) {}
InstallStageTracker::FailureReason reason,
bool is_from_store) {}

// Called when cache status is retrieved from InstallationStageTracker.
virtual void OnExtensionDownloadCacheStatusRetrieved(
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/extensions/forced_extensions/force_installed_tracker_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ class ForceInstalledTrackerTest : public ForceInstalledTestBase,
void OnForceInstalledExtensionsReady() override { ready_called_ = true; }
void OnForceInstalledExtensionFailed(
const ExtensionId& extension_id,
InstallStageTracker::FailureReason reason) override {
InstallStageTracker::FailureReason reason,
bool is_from_store) override {
error_reason_[extension_id] = reason;
}

Expand Down
11 changes: 8 additions & 3 deletions tools/metrics/histograms/metadata/others/histograms.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6529,13 +6529,18 @@ chromium-metrics-reviews@google.com.
</summary>
</histogram>

<histogram name="Kiosk.Extensions.InstallError"
enum="ExtensionInstallationFailureReason" expires_after="2022-09-01">
<histogram name="Kiosk.Extensions.InstallError.{InstallSource}"
enum="ExtensionInstallationFailureReason" expires_after="2022-10-01">
<owner>yixie@chromium.org</owner>
<owner>chromeos-kiosk-eng@google.com</owner>
<summary>
Records the reason for a Kiosk forced extension install error.
Records the reason for a Kiosk forced {InstallSource} extension install
error. (Chrome OS only)
</summary>
<token key="InstallSource">
<variant name="OffStore" summary="non-Web-Store"/>
<variant name="WebStore" summary="Web Store"/>
</token>
</histogram>

<histogram name="Kiosk.Extensions.InstallTimedOut" enum="BooleanYesNo"
Expand Down

0 comments on commit 6465104

Please sign in to comment.