Fix Finch Safe Mode #7725
Fix Finch Safe Mode #7725
Conversation
| } | ||
| } | ||
|
|
||
| void GlobalFeatures::Shutdown() { |
There was a problem hiding this comment.
Explicitly destroys the metrics manager. This is then hooked into the application's main lifecycle by calling it from CobaltBrowserMainParts::PostMainMessageLoopRun()
There was a problem hiding this comment.
I think it would be good idea to surface this as a comment to make it clear for future maintainers
There was a problem hiding this comment.
Agreed. Maybe add a comment explaining why we need to explicitly destroy it.
| } | ||
| } | ||
|
|
||
| void GlobalFeatures::Shutdown() { |
There was a problem hiding this comment.
I think it would be good idea to surface this as a comment to make it clear for future maintainers
| kEmptyConfig, | ||
| }; | ||
|
|
||
| // This class manages the content of the experiment config stored on disk. |
There was a problem hiding this comment.
Nit: Probably worth updating this class' description since it is now doing extra work as tracking both experiment config and the state of the system (Crashes)
There was a problem hiding this comment.
So the class hasn't functionally changed. I discovered that it was checking the crash streak in the wrong file experiment_config, instead of metrics_local_state (see ExperimentConfigManager params). The class itself functionally remains the same 🙂
| // the experiment config type. | ||
| experiment_config_manager_ = | ||
| std::make_unique<ExperimentConfigManager>(experiment_config_.get()); | ||
| experiment_config_manager_ = std::make_unique<ExperimentConfigManager>( |
There was a problem hiding this comment.
super-nit: Since this is only used for initializing the activeExperimentIds consider moving it to the InitializeActiveExperimentIds function to keep all relevant steps around initializing experiments all together.
There was a problem hiding this comment.
I believe setExperimentState also calls this config manager. I think it's ok either way (since this is a member variable) as long as the GlobalFeatures ctor initializes it
jammel-yeboah
force-pushed
the
fix_safe_mode
branch
from
f5023bb to
ef4d64e
Compare
|
|
||
| #include "cobalt/browser/cobalt_content_browser_client.h" | ||
|
|
||
| #include <iostream> |
| // the experiment config type. | ||
| experiment_config_manager_ = | ||
| std::make_unique<ExperimentConfigManager>(experiment_config_.get()); | ||
| experiment_config_manager_ = std::make_unique<ExperimentConfigManager>( |
There was a problem hiding this comment.
I believe setExperimentState also calls this config manager. I think it's ok either way (since this is a member variable) as long as the GlobalFeatures ctor initializes it
| } | ||
| } | ||
|
|
||
| void GlobalFeatures::Shutdown() { |
There was a problem hiding this comment.
Agreed. Maybe add a comment explaining why we need to explicitly destroy it.
| // CommitPendingWrite not called here to avoid excessive disk writes. | ||
| // Features and featureParams won't be applied until the next Cobalt cold | ||
| // start so the delay is acceptable. | ||
| global_features->metrics_local_state()->CommitPendingWrite(); |
There was a problem hiding this comment.
Is there a particular reason we need to call CommitPendingWrite here?
| void ClearMetricsServiceClientRawPtrForTest() { metrics_service_client_ = nullptr; } | ||
| void ClearMetricsServiceClient() { metrics_service_client_ = nullptr; } | ||
|
|
||
| void ClearMetricsServiceClientRawPtrForTest() { |
There was a problem hiding this comment.
What exactly is ClearMetricsServiceClientRawPtrForTest for? Are you planning to add tests in this PR?
There was a problem hiding this comment.
I was using this for debugging. Thanks for pointing it out, I'll remove it
3 participants
Safe mode is a crucial mechanism for Cobalt to fall back to a "safe" or "default" config if it receives a faulty config that causes a crash.
If we crash 3 times, we fall back to the safe config. If we crash 4 times, we fall back to the default, or "empty" config.
It was recently discovered that Safe mode was not functioning in Cobalt. This PR fixes it.
bug: 452149713