Install default apps if sync is disabled.

chrome/browser/extensions/external_pref_loader.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/external_pref_loader.h"
 
 #include "base/bind.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/prefs/pref_service_syncable.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/common/chrome_paths.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 
 namespace {
 
 base::FilePath::CharType kExternalExtensionJson[] =
     FILE_PATH_LITERAL("external_extensions.json");
 
@@ skipping 83 matching lines @@
 }
 
 void ExternalPrefLoader::StartLoading() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (options_ & DELAY_LOAD_UNTIL_PRIORITY_SYNC) {
     if (!PostLoadIfPrioritySyncReady()) {
       DCHECK(profile_);
       PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
       DCHECK(prefs);
       syncable_pref_observer_.Add(prefs);
+      ProfileSyncService* service =
+          ProfileSyncServiceFactory::GetForProfile(profile_);

[Andrew T Wilson (Slow), 2015/02/25 14:47:31]

You can't depend on ProfileSyncService existing, as it could be disabled by a
cmd-line flag or via GPO. I think this code will cause a crash if you run with
--disable-sync.

Maybe try calling profile->IsSyncAccessible() first, and if not, treat this like
sync is disabled.

[Ivan Podogov, 2015/02/25 16:34:14]

Funny thing is, IsSyncAccessible() call would also crash in that case. I've
adjusted patch a bit, so that it would behave better, but then again, on test
accounts I have I get some crashes even on an unpatched master when using
--disable-sync.

[Andrew T Wilson (Slow), 2015/02/25 17:47:22]

If you find those, please file bugs. These regress sometimes and we need to fix
them.

I don't think IsSyncAccessible() should ever crash - what's the crash you are
seeing when calling that?

[Ivan Podogov, 2015/02/26 12:50:14]

My bad, I mistook somethig: profile was nullptr, and it was not a crash, but a
mere DCHECK fail with SIGTRAP, that happens independently of --disable-sync...
Brought the IsSyncAccessible back, it should be enough to keep service available
in all cases.

+      DCHECK(service);
+      service->AddObserver(this);
     }
   } else {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         base::Bind(&ExternalPrefLoader::LoadOnFileThread, this));
   }
 }
 
 void ExternalPrefLoader::OnIsSyncingChanged() {
   PostLoadIfPrioritySyncReady();
 }
 
+void ExternalPrefLoader::OnStateChanged() {
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  DCHECK(service);
+
+  ProfileSyncService::SyncStatusSummary status =
+      service->QuerySyncStatusSummary();
+  if (status == ProfileSyncService::SyncStatusSummary::NOT_ENABLED ||
+      status == ProfileSyncService::SyncStatusSummary::UNRECOVERABLE_ERROR) {

[Andrew T Wilson (Slow), 2015/02/25 14:47:31]

Not sure if perhaps you should be checking for UNKNOWN_ERROR or some of the
other error states? Perhaps ping nzea?

[Ivan Podogov, 2015/02/25 16:34:14]

UNKNOWN_ERROR corresponds to some state that even ProfileSyncService authors are
not sure it is possible to be in... I'm not sure what course of action would be
the best in that case.

[Andrew T Wilson (Slow), 2015/02/25 17:47:21]

What I want to make sure is we aren't left in a state where we never load
default apps. What happens if you treat it like an unrecoverable error (what's
the worst case scenario?)

+    PostLoadAndRemoveObservers();
+  }
+}
+
 bool ExternalPrefLoader::PostLoadIfPrioritySyncReady() {
   DCHECK(options_ & DELAY_LOAD_UNTIL_PRIORITY_SYNC);
   DCHECK(profile_);
 
   PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
   DCHECK(prefs);
   if (prefs->IsPrioritySyncing()) {
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        base::Bind(&ExternalPrefLoader::LoadOnFileThread, this));
-    syncable_pref_observer_.Remove(prefs);
+    PostLoadAndRemoveObservers();
     return true;
   }
 
   return false;
 }
 
+void ExternalPrefLoader::PostLoadAndRemoveObservers() {
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&ExternalPrefLoader::LoadOnFileThread, this));
+
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
+  DCHECK(prefs);
+  syncable_pref_observer_.Remove(prefs);
+
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  DCHECK(service);
+  service->RemoveObserver(this);
+}
+
 void ExternalPrefLoader::LoadOnFileThread() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   scoped_ptr<base::DictionaryValue> prefs(new base::DictionaryValue);
 
   // TODO(skerner): Some values of base_path_id_ will cause
   // PathService::Get() to return false, because the path does
   // not exist.  Find and fix the build/install scripts so that
   // this can become a CHECK().  Known examples include chrome
   // OS developer builds and linux install packages.
@@ skipping 123 matching lines @@
   LoadFinished();
 }
 
 ExternalTestingLoader::~ExternalTestingLoader() {}
 
 const base::FilePath ExternalTestingLoader::GetBaseCrxFilePath() {
   return fake_base_path_;
 }
 
 }  // namespace extensions