[ios] Fixes a shutdown crash when Omaha is not initialized.

M84 is crashing because OmahaService::Stop() is somehow called before
the OmahaService singleton is created. This causes Stop() to try and
create a new service object during shutdown, which crashes.

This CL sidesteps the crash by making it safe to create a new
OmahaService object, even at shutdown. The unsafe portions of the
constructor were already in StartInternal(), which now needs to be
explicitly invoked by OmahaService::Start().

BUG=1105799

Change-Id: Idd9531ab0ac79e4520deaa2145e9d3efc20bb01d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2339719Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795980}

ios/chrome/browser/omaha/omaha_service.h

@@ -172,6 +172,9 @@ class OmahaService {
       pending_url_loader_factory_;
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
+  // Whether the service has been started.
+  bool started_;
+
   // The timer that call this object back when needed.
   base::OneShotTimer timer_;
 

ios/chrome/browser/omaha/omaha_service.mm

@@ -332,6 +332,8 @@ void OmahaService::Start(std::unique_ptr<network::PendingSharedURLLoaderFactory>
   }
 
   OmahaService* service = GetInstance();
+  service->StartInternal();
+
   service->set_upgrade_recommended_callback(callback);
   // This should only be called once.
   DCHECK(!service->pending_url_loader_factory_ ||
@@ -353,23 +355,22 @@ void OmahaService::Stop() {
   service->StopInternal();
 }
 
-OmahaService::OmahaService()
-    : schedule_(true),
-      application_install_date_(0),
-      sending_install_event_(false) {
-  StartInternal();
-}
+OmahaService::OmahaService() : OmahaService(/*schedule=*/true) {}
 
 OmahaService::OmahaService(bool schedule)
-    : schedule_(schedule),
+    : started_(false),
+      schedule_(schedule),
       application_install_date_(0),
-      sending_install_event_(false) {
-  StartInternal();
-}
+      sending_install_event_(false) {}
 
 OmahaService::~OmahaService() {}
 
 void OmahaService::StartInternal() {
+  if (started_) {
+    return;
+  }
+  started_ = true;
+
   // Start the provider at the same time as the rest of the service.
   ios::GetChromeBrowserProvider()->GetOmahaServiceProvider()->Start();
 
@@ -432,6 +433,10 @@ void OmahaService::StartInternal() {
 }
 
 void OmahaService::StopInternal() {
+  if (!started_) {
+    return;
+  }
+
   ios::GetChromeBrowserProvider()->GetOmahaServiceProvider()->Stop();
 }
 

ios/chrome/browser/omaha/omaha_service_unittest.mm

@@ -115,6 +115,8 @@ TEST_F(OmahaServiceTest, PingMessageTest) {
       "<ping active=\"1\" ad=\"-2\" rd=\"-2\"/></app></request>";
 
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   std::string content = service.GetPingContent(
@@ -141,6 +143,8 @@ TEST_F(OmahaServiceTest, PingMessageTestWithUnknownInstallDate) {
       "<ping active=\"1\" ad=\"-2\" rd=\"-2\"/></app></request>";
 
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   std::string content = service.GetPingContent(
@@ -171,6 +175,8 @@ TEST_F(OmahaServiceTest, InstallEventMessageTest) {
 
   // First install.
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   CleanService(&service, "");
@@ -207,6 +213,8 @@ TEST_F(OmahaServiceTest, InstallEventMessageTest) {
 TEST_F(OmahaServiceTest, SendPingSuccess) {
   base::Time now = base::Time::Now();
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   service.InitializeURLLoaderFactory(test_shared_url_loader_factory_);
@@ -241,6 +249,8 @@ TEST_F(OmahaServiceTest, SendPingSuccess) {
 
 TEST_F(OmahaServiceTest, ParseAndEchoLastServerDate) {
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   service.InitializeURLLoaderFactory(test_shared_url_loader_factory_);
@@ -288,6 +298,8 @@ TEST_F(OmahaServiceTest, ParseAndEchoLastServerDate) {
 TEST_F(OmahaServiceTest, SendInstallEventSuccess) {
   base::Time now = base::Time::Now();
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   service.InitializeURLLoaderFactory(test_shared_url_loader_factory_);
@@ -320,6 +332,8 @@ TEST_F(OmahaServiceTest, SendInstallEventSuccess) {
 TEST_F(OmahaServiceTest, SendPingReceiveUpdate) {
   base::Time now = base::Time::Now();
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   service.InitializeURLLoaderFactory(test_shared_url_loader_factory_);
@@ -365,6 +379,8 @@ TEST_F(OmahaServiceTest, SendPingReceiveUpdate) {
 TEST_F(OmahaServiceTest, SendPingFailure) {
   base::Time now = base::Time::Now();
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   service.InitializeURLLoaderFactory(test_shared_url_loader_factory_);
@@ -416,6 +432,8 @@ TEST_F(OmahaServiceTest, PersistStatesTest) {
   std::string version_string = version_info::GetVersionNumber();
   base::Time now = base::Time::Now();
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   service.number_of_tries_ = 5;
@@ -426,6 +444,8 @@ TEST_F(OmahaServiceTest, PersistStatesTest) {
   service.PersistStates();
 
   OmahaService service2(false);
+  service2.StartInternal();
+
   EXPECT_EQ(service.number_of_tries_, 5);
   EXPECT_EQ(service2.last_sent_time_, now - base::TimeDelta::FromSeconds(1));
   EXPECT_EQ(service2.next_tries_time_, now + base::TimeDelta::FromSeconds(2));
@@ -449,6 +469,8 @@ TEST_F(OmahaServiceTest, BackoffTest) {
 TEST_F(OmahaServiceTest, ActivePingAfterInstallEventTest) {
   base::Time now = base::Time::Now();
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   service.InitializeURLLoaderFactory(test_shared_url_loader_factory_);
@@ -483,6 +505,8 @@ TEST_F(OmahaServiceTest, ActivePingAfterInstallEventTest) {
 TEST_F(OmahaServiceTest, NonSpammingTest) {
   base::Time now = base::Time::Now();
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   service.InitializeURLLoaderFactory(test_shared_url_loader_factory_);
@@ -516,6 +540,8 @@ TEST_F(OmahaServiceTest, NonSpammingTest) {
 
 TEST_F(OmahaServiceTest, InstallRetryTest) {
   OmahaService service(false);
+  service.StartInternal();
+
   service.set_upgrade_recommended_callback(
       base::Bind(&OmahaServiceTest::OnNeedUpdate, base::Unretained(this)));
   service.InitializeURLLoaderFactory(test_shared_url_loader_factory_);