Drive: Removes unused cache files after the initial feed fetch.

chrome/browser/chromeos/gdata/gdata_file_system.h

 // 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.
 
 #ifndef CHROME_BROWSER_CHROMEOS_GDATA_GDATA_FILE_SYSTEM_H_
 #define CHROME_BROWSER_CHROMEOS_GDATA_GDATA_FILE_SYSTEM_H_
 
 #include <map>
 #include <set>
 #include <string>
@@ skipping 16 matching lines @@
 namespace base {
 class SequencedTaskRunner;
 }
 
 namespace gdata {
 
 class DocumentsServiceInterface;
 class DriveWebAppsRegistryInterface;
 class GDataUploaderInterface;
 class GDataWapiFeedLoader;
+class StaleCacheFilesRemover;
 struct UploadFileInfo;
 
 // The production implementation of GDataFileSystemInterface.
 class GDataFileSystem : public GDataFileSystemInterface,
                         public GDataWapiFeedLoader::Observer,
                         public content::NotificationObserver {
  public:
   GDataFileSystem(Profile* profile,
                   GDataCache* cache,
                   DocumentsServiceInterface* documents_service,
@@ skipping 755 matching lines @@
       GDataFileError error,
       const std::string& resource_id,
       const std::string& md5,
       const FilePath& local_cache_path);
   void CheckLocalModificationAndRunAfterGetFileInfo(
       scoped_ptr<GDataEntryProto> entry_proto,
       const GetEntryInfoCallback& callback,
       base::PlatformFileInfo* file_info,
       bool* get_file_info_result);
 
-  // All members should be accessed only on UI thread. Do not post tasks to
-  // other threads with base::Unretained(this).
-  scoped_ptr<GDataDirectoryService> directory_service_;
-
   // The profile hosts the GDataFileSystem via GDataSystemService.
   Profile* profile_;
 
   // The cache owned by GDataSystemService.
   GDataCache* cache_;
 
   // The uploader owned by GDataSystemService.
   GDataUploaderInterface* uploader_;
 
   // The document service owned by GDataSystemService.
   DocumentsServiceInterface* documents_service_;
 
   // The webapps registry owned by GDataSystemService.
   DriveWebAppsRegistryInterface* webapps_registry_;
 
   // Periodic timer for checking updates.
   base::Timer update_timer_;
 
   // True if hosted documents should be hidden.
   bool hide_hosted_docs_;
 
   // The set of paths opened by OpenFile but not yet closed by CloseFile.
   std::set<FilePath> open_files_;
 
+  // Note: |observers_| should be placed before all the objects this class owns,
+  // because some of them may add themselves into the observers, so this should
+  // be constructed before them and be destructed after them.

[satorux1, 2012/08/21 20:57:06]

Wow, that's subtle. If so, why don't you move this to the very end before
WeakPtr stuff? StaleCacheFilesRemover is owned by this class, but placed below.
why?

[satorux1, 2012/08/21 21:29:06]

Sorry, I misunderstood. So the observer list has to be created *before*
|stale_cache_files_remover_| and destructed *after*
|stale_cache_files_remover_|, right?

I think the issue we were hit was not a memory leak but a use-after-free.
file_system_->RemoveObserver(this) was called after the observer list was
destructed, right?

What about moving the ownership of |stale_cache_files_remover_| to
GDataSystemService?

+  ObserverList<GDataFileSystemInterface::Observer> observers_;
+
+  // All members should be accessed only on UI thread. Do not post tasks to
+  // other threads with base::Unretained(this).
+  scoped_ptr<GDataDirectoryService> directory_service_;
+
   scoped_ptr<PrefChangeRegistrar> pref_registrar_;
 
   // The loader is used to load the feeds.
   scoped_ptr<GDataWapiFeedLoader> feed_loader_;
 
-  ObserverList<GDataFileSystemInterface::Observer> observers_;
+  // The remover is used to remove stale cache files.
+  scoped_ptr<StaleCacheFilesRemover> stale_cache_files_remover_;
 
   scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_;
 
   // WeakPtrFactory and WeakPtr bound to the UI thread.
   // Note: These should remain the last member so they'll be destroyed and
   // invalidate the weak pointers before any other members are destroyed.
   base::WeakPtrFactory<GDataFileSystem> ui_weak_ptr_factory_;
   // Unlike other classes, we need this as we need this to redirect a task
   // from IO thread to UI thread.
   base::WeakPtr<GDataFileSystem> ui_weak_ptr_;
 };
 
 }  // namespace gdata
 
 #endif  // CHROME_BROWSER_CHROMEOS_GDATA_GDATA_FILE_SYSTEM_H_