Fix flakiness in RemoteFileSystemExtensionApiTests

We get in trouble because FileBrowserEventRouter creates gdata system service
early on (before we setup test service factory). When we create test system
service, the old one gets shutdown and destroyed, but they both use the same
gdata cache root path (which is created and destroyed async).

This patch avoid use of GDataSystemServiceFactory::SetTestingFactoryAndUse,
which cannot be called before an system service instance is already alive
(created early on by FileBroserEventRouter).

the flakiness was introduced by https://src.chromium.org/viewvc/chrome?view=rev&revision=147208

TEST=RemoteFileSystemExtensionApiTest.*
BUG=none

Review URL: https://chromiumcodereview.appspot.com/10805034

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@147980 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/extensions/external_filesystem_apitest.cc

@@ -193,21 +193,41 @@ class RemoteFileSystemExtensionApiTest : public ExtensionApiTest {
 
   virtual ~RemoteFileSystemExtensionApiTest() {}
 
-  // Sets up GDataFileSystem that will be used in the test.
-  // NOTE: Remote mount point should get added to mount point provider when
-  // getLocalFileSystem is called from filebrowser_component extension.
-  virtual void SetupGDataFileSystemForTest() {
-    // |mock_documents_service_| is owned by |system_service|.
+  virtual void SetUp() OVERRIDE {
+    // Set up cache root and documents service to be used when creating gdata
+    // system service. This has to be done early on (before the browser is
+    // created) because the system service instance is initialized very early
+    // by FileBrowserEventRouter.
+    FilePath tmp_dir_path;
+    PathService::Get(base::DIR_TEMP, &tmp_dir_path);
+    ASSERT_TRUE(test_cache_root_.CreateUniqueTempDirUnderPath(tmp_dir_path));
+    gdata::GDataSystemServiceFactory::set_cache_root_for_test(
+        test_cache_root_.path().value());
+
     mock_documents_service_ = new gdata::MockDocumentsService();
+
     operation_registry_.reset(new gdata::GDataOperationRegistry());
-    gdata::GDataSystemService* system_service =
-        gdata::GDataSystemServiceFactory::GetInstance()->
-        GetWithCustomDocumentsServiceForTesting(
-            browser()->profile(), mock_documents_service_);
-    EXPECT_TRUE(system_service);
+    // FileBrowserEventRouter will add and remove itself from operation registry
+    // observer list.
+    EXPECT_CALL(*mock_documents_service_, operation_registry()).
+        WillRepeatedly(Return(operation_registry_.get()));
+
+    // |mock_documents_service_| will eventually get owned by a system service.
+    gdata::GDataSystemServiceFactory::set_documents_service_for_test(
+        mock_documents_service_);
+
+    ExtensionApiTest::SetUp();
+  }
+
+  virtual void TearDown() OVERRIDE {
+    // Let's make sure we don't leak documents service.
+    gdata::GDataSystemServiceFactory::set_documents_service_for_test(NULL);
+    gdata::GDataSystemServiceFactory::set_cache_root_for_test(std::string());
+    ExtensionApiTest::TearDown();
   }
 
  protected:
+  ScopedTempDir test_cache_root_;
   gdata::MockDocumentsService* mock_documents_service_;
   scoped_ptr<gdata::GDataOperationRegistry> operation_registry_;
 };
@@ -258,8 +278,6 @@ IN_PROC_BROWSER_TEST_F(FileSystemExtensionApiTest,
 
 IN_PROC_BROWSER_TEST_F(RemoteFileSystemExtensionApiTest,
                        RemoteMountPoint) {
-  SetupGDataFileSystemForTest();
-
   EXPECT_CALL(*mock_documents_service_, GetAccountMetadata(_)).Times(1);
 
   // First, file browser will try to create new directory.
@@ -299,10 +317,6 @@ IN_PROC_BROWSER_TEST_F(RemoteFileSystemExtensionApiTest,
 
   // On exit, all operations in progress should be cancelled.
   EXPECT_CALL(*mock_documents_service_, CancelAll());
-  // This one is called on exit, but we don't care much about it, as long as it
-  // retunrs something valid (i.e. not NULL).
-  EXPECT_CALL(*mock_documents_service_, operation_registry()).
-      WillRepeatedly(Return(operation_registry_.get()));
 
   // All is set... RUN THE TEST.
   EXPECT_TRUE(RunExtensionTest("filesystem_handler")) << message_;
@@ -318,8 +332,6 @@ IN_PROC_BROWSER_TEST_F(RemoteFileSystemExtensionApiTest,
 #endif
 IN_PROC_BROWSER_TEST_F(RemoteFileSystemExtensionApiTest,
                        MAYBE_ContentSearch) {
-  SetupGDataFileSystemForTest();
-
   EXPECT_CALL(*mock_documents_service_, GetAccountMetadata(_)).Times(1);
 
   // First, test will get drive root directory, to init file system.
@@ -353,10 +365,6 @@ IN_PROC_BROWSER_TEST_F(RemoteFileSystemExtensionApiTest,
 
   // On exit, all operations in progress should be cancelled.
   EXPECT_CALL(*mock_documents_service_, CancelAll());
-  // This one is called on exit, but we don't care much about it, as long as it
-  // retunrs something valid (i.e. not NULL).
-  EXPECT_CALL(*mock_documents_service_, operation_registry()).
-      WillRepeatedly(Return(operation_registry_.get()));
 
   // All is set... RUN THE TEST.
   EXPECT_TRUE(RunExtensionSubtest("filebrowser_component", "remote_search.html",

chrome/browser/chromeos/gdata/gdata_system_service.cc

@@ -29,6 +29,10 @@ using content::BrowserThread;
 
 namespace {
 
+// Used in test to setup system service.
+gdata::DocumentsServiceInterface* g_test_documents_service = NULL;
+const std::string* g_test_cache_root = NULL;
+
 scoped_refptr<base::SequencedTaskRunner> GetTaskRunner(
     const base::SequencedWorkerPool::SequenceToken& sequence_token) {
   return BrowserThread::GetBlockingPool()->GetSequencedTaskRunner(
@@ -53,12 +57,13 @@ GDataSystemService::~GDataSystemService() {
 }
 
 void GDataSystemService::Initialize(
-    DocumentsServiceInterface* documents_service) {
+    DocumentsServiceInterface* documents_service,
+    const FilePath& cache_root) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   documents_service_.reset(documents_service);
   cache_ = GDataCache::CreateGDataCacheOnUIThread(
-      GDataCache::GetCacheRootPath(profile_),
+      cache_root,
       GetTaskRunner(sequence_token_));
   uploader_.reset(new GDataUploader(docs_service()));
   webapps_registry_.reset(new DriveWebAppsRegistry);
@@ -153,27 +158,37 @@ GDataSystemServiceFactory::~GDataSystemServiceFactory() {
 }
 
 // static
-ProfileKeyedService* GDataSystemServiceFactory::CreateInstance(
-    Profile* profile) {
-  return new GDataSystemService(profile);
+void GDataSystemServiceFactory::set_documents_service_for_test(
+    DocumentsServiceInterface* documents_service) {
+  if (g_test_documents_service)
+    delete g_test_documents_service;
+  g_test_documents_service = documents_service;
 }
 
-GDataSystemService*
-GDataSystemServiceFactory::GetWithCustomDocumentsServiceForTesting(
-    Profile* profile,
-    DocumentsServiceInterface* documents_service) {
-  GDataSystemService* service =
-      static_cast<GDataSystemService*>(GetInstance()->SetTestingFactoryAndUse(
-          profile,
-          &GDataSystemServiceFactory::CreateInstance));
-  service->Initialize(documents_service);
-  return service;
+// static
+void GDataSystemServiceFactory::set_cache_root_for_test(
+    const std::string& cache_root) {
+  if (g_test_cache_root)
+    delete g_test_cache_root;
+  g_test_cache_root = !cache_root.empty() ? new std::string(cache_root) : NULL;
 }
 
 ProfileKeyedService* GDataSystemServiceFactory::BuildServiceInstanceFor(
     Profile* profile) const {
   GDataSystemService* service = new GDataSystemService(profile);
-  service->Initialize(new DocumentsService);
+
+  DocumentsServiceInterface* documents_service =
+      g_test_documents_service ? g_test_documents_service :
+                                 new DocumentsService();
+  g_test_documents_service = NULL;
+
+  FilePath cache_root =
+      g_test_cache_root ? FilePath(*g_test_cache_root) :
+                          GDataCache::GetCacheRootPath(profile);
+  delete g_test_cache_root;
+  g_test_cache_root = NULL;
+
+  service->Initialize(documents_service, cache_root);
   return service;
 }
 

chrome/browser/chromeos/gdata/gdata_system_service.h

@@ -11,6 +11,8 @@
 #include "chrome/browser/profiles/profile_keyed_service.h"
 #include "chrome/browser/profiles/profile_keyed_service_factory.h"
 
+class FilePath;
+
 namespace gdata {
 
 class DocumentsServiceInterface;
@@ -53,7 +55,8 @@ class GDataSystemService : public ProfileKeyedService  {
 
   // Initializes the object. This function should be called before any
   // other functions.
-  void Initialize(DocumentsServiceInterface* documents_service);
+  void Initialize(DocumentsServiceInterface* documents_service,
+                  const FilePath& cache_root);
 
   // Registers remote file system proxy for drive mount point.
   void AddDriveMountPoint();
@@ -89,16 +92,20 @@ class GDataSystemServiceFactory : public ProfileKeyedServiceFactory {
   // Returns the GDataSystemServiceFactory instance.
   static GDataSystemServiceFactory* GetInstance();
 
-  // Just creates a new instance without initializing most of the fields.
-  // This is useful for tests such as injecting mocks.
-  static ProfileKeyedService* CreateInstance(Profile* profile);
-
-  // Returns GDataSystemService for testing, with |documents_service| injected
-  // to GDataFileSystem.
-  GDataSystemService* GetWithCustomDocumentsServiceForTesting(
-      Profile* profile,
+  // Sets documents service that should be used to initialize file system in
+  // test. Should be called before the service is created.
+  // Please, make sure |documents_service| gets deleted if no system service is
+  // created (e.g. by calling this method with NULL).
+  static void set_documents_service_for_test(
       DocumentsServiceInterface* documents_service);
 
+  // Sets root path for the cache used in test. Should be called before the
+  // service is created.
+  // If |cache_root| is not empty, new string object will be created. Please,
+  // make sure it gets deleted if no system service is created (e.g. by calling
+  // this method with empty string).
+  static void set_cache_root_for_test(const std::string& cache_root);
+
  private:
   friend struct DefaultSingletonTraits<GDataSystemServiceFactory>;