Skip to content

Commit

Permalink
Revert "Fix an edge case in the Extended Variations Safe Mode experim…
Browse files Browse the repository at this point in the history 
…ent."

This reverts commit 6be9fc8.

Reason for revert: crbug/1245367

Original change's description:
> Fix an edge case in the Extended Variations Safe Mode experiment.
>
> When the CleanExitBeacon is created, the beacon file contents are read
> and used regardless of whether the client is in the group that uses this
> file. It is possible for a client to have the new file and to not be in
> the SignalAndWriteViaFileUtil group if the client was previously in that
> group and then later switched groups, e.g. via kResetVariationState.
>
> This change fixes the edge case by moving experiment group assignment
> from the VariationsFieldTrialCreator to the CleanExitBeacon ctor. This
> requires plumbing the channel through to the CleanExitBeacon because
> this Variations Safe Mode client-side field trial is active on only
> certain channels.
>
> Bug: 1244334
> Change-Id: I3c2720ac62f04ebac353cd86947476a871f3621c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3120545
> Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Reviewed-by: Evan Stade <estade@chromium.org>
> Reviewed-by: Ilya Sherman <isherman@chromium.org>
> Commit-Queue: Caitlin Fischer <caitlinfischer@google.com>
> Cr-Commit-Position: refs/heads/main@{#916809}

Bug: 1244334
Change-Id: I904a419b685031b24ec480322831d62e4e2825fc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3133021
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Commit-Queue: Caitlin Fischer <caitlinfischer@google.com>
Cr-Commit-Position: refs/heads/main@{#916968}
  • Loading branch information
Caitlin Fischer authored and Chromium LUCI CQ committed Aug 31, 2021
1 parent dd30256 commit a92b8a1
Show file tree
Hide file tree
Showing 17 changed files with 222 additions and 319 deletions.
7 changes: 2 additions & 5 deletions chrome/browser/metrics/chrome_metrics_services_manager_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
#include "chrome/browser/metrics/variations/ui_string_overrider_factory.h"
#include "chrome/browser/net/system_network_context_manager.h"
#include "chrome/browser/ui/browser_otr_state.h"
#include "chrome/common/channel_info.h"
#include "chrome/common/chrome_paths.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/installer/util/google_update_settings.h"
Expand All @@ -35,7 +34,6 @@
#include "components/prefs/pref_service.h"
#include "components/variations/service/variations_service.h"
#include "components/variations/variations_associated_data.h"
#include "components/version_info/channel.h"
#include "components/version_info/version_info.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/network_service_instance.h"
Expand Down Expand Up @@ -63,8 +61,8 @@
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

namespace metrics {
namespace internal {

namespace internal {
// Metrics reporting feature. This feature, along with user consent, controls if
// recording and reporting are enabled. If the feature is enabled, but no
// consent is given, then there will be no recording or reporting.
Expand Down Expand Up @@ -284,8 +282,7 @@ ChromeMetricsServicesManagerClient::GetMetricsStateManager() {
metrics_state_manager_ = metrics::MetricsStateManager::Create(
local_state_, enabled_state_provider_.get(), GetRegistryBackupKey(),
user_data_dir, base::BindRepeating(&PostStoreMetricsClientInfo),
base::BindRepeating(&GoogleUpdateSettings::LoadMetricsClientInfo),
chrome::GetChannel());
base::BindRepeating(&GoogleUpdateSettings::LoadMetricsClientInfo));
}
return metrics_state_manager_.get();
}
Expand Down
5 changes: 2 additions & 3 deletions components/embedder_support/android/metrics/android_metrics_service_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,7 @@ void AndroidMetricsServiceClient::RegisterPrefs(PrefRegistrySimple* registry) {

void AndroidMetricsServiceClient::Initialize(
const base::FilePath& user_data_dir,
PrefService* pref_service,
version_info::Channel channel) {
PrefService* pref_service) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!init_finished_);

Expand All @@ -252,7 +251,7 @@ void AndroidMetricsServiceClient::Initialize(
metrics_state_manager_ = MetricsStateManager::Create(
pref_service_, this, std::wstring(), user_data_dir,
base::BindRepeating(&StoreClientInfo),
base::BindRepeating(&LoadClientInfo), channel);
base::BindRepeating(&LoadClientInfo));

init_finished_ = true;

Expand Down
12 changes: 2 additions & 10 deletions components/embedder_support/android/metrics/android_metrics_service_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include "components/metrics/metrics_log_uploader.h"
#include "components/metrics/metrics_service_client.h"
#include "components/version_info/android/channel_getter.h"
#include "components/version_info/channel.h"
#include "components/version_info/version_info.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
Expand Down Expand Up @@ -110,15 +109,8 @@ class AndroidMetricsServiceClient : public MetricsServiceClient,
//
// |user_data_dir| is the path to the client's user data directory. If empty,
// a separate file will not be used for Variations Safe Mode prefs.
//
// TODO(crbug.com/1241702): Remove |channel| at the end of the Extended
// Variations Safe Mode experiment. |channel| is used to enable the experiment
// on only certain channels. The experiment is always disabled on unknown
// channels.
void Initialize(
const base::FilePath& user_data_dir,
PrefService* pref_service,
version_info::Channel channel = version_info::Channel::UNKNOWN);
void Initialize(const base::FilePath& user_data_dir,
PrefService* pref_service);
void SetHaveMetricsConsent(bool user_consent, bool app_consent);
void SetFastStartupForTesting(bool fast_startup_for_testing);
void SetUploadIntervalForTesting(const base::TimeDelta& upload_interval);
Expand Down
1 change: 0 additions & 1 deletion components/metrics/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,6 @@ source_set("unit_tests") {
"//components/sync/base",
"//components/sync/driver:test_support",
"//components/variations",
"//components/variations:test_support",
"//components/variations/service:constants",
"//extensions/buildflags",
"//mojo/public/cpp/bindings",
Expand Down
48 changes: 5 additions & 43 deletions components/metrics/clean_exit_beacon.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@
namespace metrics {
namespace {

using ::variations::kControlGroup;
using ::variations::kDefaultGroup;
using ::variations::kExtendedSafeModeTrial;
using ::variations::kSignalAndWriteSynchronouslyViaPrefServiceGroup;
using ::variations::kSignalAndWriteViaFileUtilGroup;
Expand Down Expand Up @@ -118,9 +116,6 @@ bool DidPreviousSessionExitCleanly(base::Value* beacon_file_contents,
// kResetVariationState.
std::unique_ptr<base::Value> MaybeGetFileContents(
const base::FilePath& beacon_file_path) {
if (beacon_file_path.empty())
return nullptr;

JSONFileValueDeserializer deserializer(beacon_file_path);
std::unique_ptr<base::Value> beacon_file_contents = deserializer.Deserialize(
/*error_code=*/nullptr, /*error_message=*/nullptr);
Expand All @@ -140,52 +135,19 @@ std::unique_ptr<base::Value> MaybeGetFileContents(
return nullptr;
}

// Sets up the Extended Variations Safe Mode experiment, which is enabled on
// only some channels. If assigned to an experiment group, returns the name of
// the group name, e.g. "Control"; otherwise, returns the empty string.
std::string SetUpExtendedSafeModeTrial(version_info::Channel channel) {
if (channel != version_info::Channel::CANARY &&
channel != version_info::Channel::DEV) {
return std::string();
}

int default_group;
scoped_refptr<base::FieldTrial> trial(
base::FieldTrialList::FactoryGetFieldTrial(
kExtendedSafeModeTrial, 100, kDefaultGroup,
base::FieldTrial::ONE_TIME_RANDOMIZED, &default_group));

trial->AppendGroup(kControlGroup, 25);
trial->AppendGroup(kWriteSynchronouslyViaPrefServiceGroup, 25);
trial->AppendGroup(kSignalAndWriteSynchronouslyViaPrefServiceGroup, 25);
trial->AppendGroup(kSignalAndWriteViaFileUtilGroup, 25);

const int assigned_group = trial->group();
DCHECK_NE(assigned_group, default_group);
return trial->group_name();
}

} // namespace

CleanExitBeacon::CleanExitBeacon(const std::wstring& backup_registry_key,
const base::FilePath& user_data_dir,
PrefService* local_state,
version_info::Channel channel)
PrefService* local_state)
: local_state_(local_state),
initial_browser_last_live_timestamp_(
local_state->GetTime(prefs::kStabilityBrowserLastLiveTimeStamp)),
backup_registry_key_(backup_registry_key) {
DCHECK_NE(PrefService::INITIALIZATION_STATUS_WAITING,
local_state_->GetInitializationStatus());

std::string group;
if (!user_data_dir.empty()) {
// Platforms that pass an empty path do so deliberately. They should not
// participate in this experiment.
group = SetUpExtendedSafeModeTrial(channel);
}

if (group == kSignalAndWriteViaFileUtilGroup)
if (!user_data_dir.empty())
beacon_file_path_ = user_data_dir.Append(variations::kVariationsFilename);

std::unique_ptr<base::Value> beacon_file_contents =
Expand Down Expand Up @@ -299,15 +261,15 @@ void CleanExitBeacon::WriteBeaconValue(bool exited_cleanly,
} else if (group_name == kSignalAndWriteViaFileUtilGroup) {
SCOPED_UMA_HISTOGRAM_TIMER_MICROS(
"Variations.ExtendedSafeMode.WritePrefsTime");
WriteBeaconFile(exited_cleanly);
WriteVariationsSafeModeFile(exited_cleanly);
}
} else {
local_state_->CommitPendingWrite();
if (group_name == kSignalAndWriteViaFileUtilGroup) {
// Clients in this group also write to the Variations Safe Mode file. This
// is because the file will be used in the next session, and thus, should
// be updated whenever kStabilityExitedCleanly is.
WriteBeaconFile(exited_cleanly);
WriteVariationsSafeModeFile(exited_cleanly);
}
}

Expand Down Expand Up @@ -372,7 +334,7 @@ void CleanExitBeacon::SkipCleanShutdownStepsForTesting() {
g_skip_clean_shutdown_steps = true;
}

void CleanExitBeacon::WriteBeaconFile(bool exited_cleanly) const {
void CleanExitBeacon::WriteVariationsSafeModeFile(bool exited_cleanly) const {
DCHECK_EQ(base::FieldTrialList::FindFullName(kExtendedSafeModeTrial),
kSignalAndWriteViaFileUtilGroup);
base::Value dict(base::Value::Type::DICTIONARY);
Expand Down
11 changes: 2 additions & 9 deletions components/metrics/clean_exit_beacon.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include "base/macros.h"
#include "base/time/time.h"
#include "build/build_config.h"
#include "components/version_info/channel.h"

class PrefRegistrySimple;
class PrefService;
Expand All @@ -32,15 +31,9 @@ class CleanExitBeacon {
//
// |user_data_dir| is the path to the client's user data directory. If empty,
// a separate file will not be used for Variations Safe Mode prefs.
//
// TODO(crbug.com/1241702): Remove |channel| at the end of the Extended
// Variations Safe Mode experiment. |channel| is used to enable the experiment
// on only certain channels. The experiment is always disabled on unknown
// channels.
CleanExitBeacon(const std::wstring& backup_registry_key,
const base::FilePath& user_data_dir,
PrefService* local_state,
version_info::Channel channel);
PrefService* local_state);

~CleanExitBeacon();

Expand Down Expand Up @@ -104,7 +97,7 @@ class CleanExitBeacon {
private:
// Writes |exited_cleanly| and the crash streak to the file located at
// |beacon_file_path_|.
void WriteBeaconFile(bool exited_cleanly) const;
void WriteVariationsSafeModeFile(bool exited_cleanly) const;

#if defined(OS_IOS)
// Checks if the NSUserDefault clean exit beacon value is set.
Expand Down
Loading

0 comments on commit a92b8a1

Please sign in to comment.