Fix windows registry loader to carry over prefs from file thread correctly

chrome/browser/extensions/external_registry_loader_win.cc

Index: chrome/browser/extensions/external_registry_loader_win.cc
diff --git a/chrome/browser/extensions/external_registry_loader_win.cc b/chrome/browser/extensions/external_registry_loader_win.cc
index 0c1088480c2f2fc7d76b71ff0e094b469b24a53e..6f331080d34cf8a44a471ea63bb44eae9eb68c81 100644
--- a/chrome/browser/extensions/external_registry_loader_win.cc
+++ b/chrome/browser/extensions/external_registry_loader_win.cc
@@ -190,17 +190,20 @@ ExternalRegistryLoader::LoadPrefsOnFileThread() {
 
 void ExternalRegistryLoader::LoadOnFileThread() {
   base::TimeTicks start_time = base::TimeTicks::Now();
-  prefs_ = LoadPrefsOnFileThread();
+  std::unique_ptr<base::DictionaryValue> prefs = LoadPrefsOnFileThread();
   LOCAL_HISTOGRAM_TIMES("Extensions.ExternalRegistryLoaderWin",
                         base::TimeTicks::Now() - start_time);
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&ExternalRegistryLoader::CompleteLoadAndStartWatchingRegistry,
-                 this));
+                 this, base::Passed(&prefs)));
 }
 
-void ExternalRegistryLoader::CompleteLoadAndStartWatchingRegistry() {
+void ExternalRegistryLoader::CompleteLoadAndStartWatchingRegistry(
+    std::unique_ptr<base::DictionaryValue> prefs) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(prefs);
+  prefs_ = std::move(prefs);
   LoadFinished();

[Devlin, 2017/04/12 19:32:06]

I *think* this is safe - or rather, I think this won't crash.  But there's still
a risk of ExternalLoader passing or storing a raw ptr, which could then be
unexpectedly overwritten.

From ExternalLoader:

  // Used for passing the list of extensions from the method that loads them
  // to |LoadFinished|. To ensure thread safety, the rules are the following:
  // if this value is written on another thread than the UI, then it should
  // only be written in a task that was posted from |StartLoading|. After that,
  // this task should invoke |LoadFinished| with a PostTask. This scheme of
  // posting tasks will avoid concurrent access and imply the necessary memory
  // barriers.
  std::unique_ptr<base::DictionaryValue> prefs_;

That's... a bit crazy.

I think this would be a lot less error prone if we passed the
std::unique_ptr<DictionaryValue> to LoadFinished() directly, rather than trying
to set a protected member before it happens.  WDYT?  If that sounds good to you,
can you at least add a TODO here or in external_loader.h to do so?

[lazyboy, 2017/04/12 20:56:22]

I was actually thinking about that while writing this CL. Given the number of
usages of |prefs_|, I decided not to pursue that.
 If that sounds good to you,
Following up separately sounds good. Added TODO in external_loader.h

 
   // Attempt to watch registry if we haven't already.