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

chrome/browser/extensions/external_registry_loader_win.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_registry_loader_win.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
@@ skipping 172 matching lines @@
     prefs->SetBoolean(
         MakePrefName(id, ExternalProviderImpl::kMayBeUntrusted),
         true);
   }
 
   return prefs;
 }
 
 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.
   if (attempted_watching_registry_)
     return;
 
   LONG result = ERROR_SUCCESS;
   if ((result = hklm_key_.Create(HKEY_LOCAL_MACHINE, kRegistryExtensions,
                                  KEY_NOTIFY | KEY_WOW64_32KEY)) ==
       ERROR_SUCCESS) {
     base::win::RegKey::ChangeCallback callback =
@@ skipping 33 matching lines @@
   base::TimeTicks start_time = base::TimeTicks::Now();
   std::unique_ptr<base::DictionaryValue> prefs = LoadPrefsOnFileThread();
   LOCAL_HISTOGRAM_TIMES("Extensions.ExternalRegistryLoaderWinUpdate",
                         base::TimeTicks::Now() - start_time);
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(&ExternalRegistryLoader::OnUpdated, this,
                                      base::Passed(&prefs)));
 }
 
 }  // namespace extensions