ChromeExtensionLoader: Prevent redundant loading of unpacked extensions.

Unpacked extensions loaded through the ChromeTestExtensionLoader are loaded
twice. The redundant load is due to the  false default value of
|allow_file_access_|. However, unpacked extensions are by default, loaded with
file access. This mismatch causes the unpacked extensions to be reloaded in
ChromeTestExtensionLoader::CheckPermissions.

Prevent this reload by using Manifest::ShouldAllowFileAccess for cases where
clients don't explicitly set |allow_file_access_|.

BUG=757695

Change-Id: Ied4b25ccb89ff1410072186d360634609b8666e5
Reviewed-on: https://chromium-review.googlesource.com/625247Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496472}

chrome/browser/extensions/chrome_test_extension_loader.cc

@@ -71,8 +71,10 @@ scoped_refptr<const Extension> ChromeTestExtensionLoader::LoadExtension(
   // non-shared modules.
   // TODO(devlin): That's not good; we shouldn't be crashing.
   if (!SharedModuleInfo::IsSharedModule(extension.get())) {
+    CheckPermissions(extension.get());
+    // Make |extension| null since it may have been reloaded invalidating
+    // pointers to it.
     extension = nullptr;
-    CheckPermissions(extension_id_);
   }
 
   if (!install_param_.empty()) {
@@ -191,15 +193,25 @@ scoped_refptr<const Extension> ChromeTestExtensionLoader::LoadCrx(
   return extension;
 }
 
-void ChromeTestExtensionLoader::CheckPermissions(
-    const std::string& extension_id) {
-  std::string id = extension_id;
+void ChromeTestExtensionLoader::CheckPermissions(const Extension* extension) {
+  std::string id = extension->id();
+
+  // If the client explicitly set |allow_file_access_|, use that value. Else
+  // use the default as per the extensions manifest location.
+  if (!allow_file_access_) {
+    allow_file_access_ =
+        Manifest::ShouldAlwaysAllowFileAccess(extension->location());
+  }
+
+  // |extension| may be reloaded subsequently, invalidating the pointer. Hence
+  // make it null.
+  extension = nullptr;
 
   // Toggling incognito or file access will reload the extension, so wait for
   // the reload.
-  if (allow_file_access_ != util::AllowFileAccess(id, browser_context_)) {
+  if (*allow_file_access_ != util::AllowFileAccess(id, browser_context_)) {
     TestExtensionRegistryObserver registry_observer(extension_registry_, id);
-    util::SetAllowFileAccess(id, browser_context_, allow_file_access_);
+    util::SetAllowFileAccess(id, browser_context_, *allow_file_access_);
     registry_observer.WaitForExtensionLoaded();
   }
 

chrome/browser/extensions/chrome_test_extension_loader.h

@@ -10,6 +10,7 @@
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest.h"
 
@@ -91,7 +92,7 @@ class ChromeTestExtensionLoader {
       const base::FilePath& unpacked_path);
 
   // Checks that the permissions of the loaded extension are correct.
-  void CheckPermissions(const std::string& extension_id);
+  void CheckPermissions(const Extension* extension);
 
   // Checks for any install warnings associated with the extension.
   bool CheckInstallWarnings(const Extension& extension);
@@ -125,7 +126,8 @@ class ChromeTestExtensionLoader {
   // extension. Only used for crx installs.
   int creation_flags_ = Extension::NO_FLAGS;
 
-  // The install location of the added extension.
+  // The install location of the added extension. Not valid for unpacked
+  // extensions.
   Manifest::Location location_ = Manifest::INTERNAL;
 
   // Whether or not the extension load should fail.
@@ -144,7 +146,7 @@ class ChromeTestExtensionLoader {
   bool grant_permissions_ = true;
 
   // Whether or not to allow file access by default to the extension.
-  bool allow_file_access_ = false;
+  base::Optional<bool> allow_file_access_;
 
   // Whether or not to allow incognito access by default to the extension.
   bool allow_incognito_access_ = false;