Currently, during origin initialization, we have to traverse all stored files on disk and check db references for every database, even if we need to open just one of them. I think I found out how to minimize opening of all databases at least (and if opening is needed, don't check all stored files, just files that were not finished, for example unfinished copy or file that was referenced only from JS and we didn't have a chance to delete it after a GC) Traversing is still needed to initialize quota.

Possible causes of orphaned files: - a crash during file copy or when a file is only referenced from JS - a file only referenced from JS won't be removed even during clean shutdown, since release builds won't run GC in near future Unified quota will likely need to initialize (or at least count file sizes) all origins, that's why we need to improve origin initialization code. Current implementation also doesn't scale well, imagine 10 000 stored files. Solution: Create a journal file (kind of a mark) for stored files that are not (yet) referenced from database in a separate directory. For example, in a subdirectory called "journals". Existence of a journal file (during origin initialization) in the journals subdirectory means, that we have to check if there are any database references for the stored file. If there are references -> just delete the journal If there are no references -> remove stored file and then the journal We can't optimize this by integrating with SQLite journal, because even when the transaction (in which all db references are removed) is successfully committed we can't remove the stored file if it has references from JS.

rebased patch

Forgot to create journals for newly created file handles.

- rebased (nsnull -> nullptr changes) - removed unused local variable "fileManagers"

Rebased patch (patch for bug 780625 caused many conflicts)

rebased patch

latest try results: https://tbpl.mozilla.org/?tree=Try&rev=a266b4b3cdb3

- rebase (PR types conversion)

Comment on attachment 654726 [details] [diff] [review] patch 0.6 Review of attachment 654726 [details] [diff] [review]: ----------------------------------------------------------------- This looks good! Just a few nits and a small change to two methods. r=me with these fixed. ::: dom/indexedDB/FileManager.cpp @@ +54,5 @@ > } // anonymous namespace > > nsresult > FileManager::Init(nsIFile* aDirectory, > + mozIStorageConnection* aConnection) Nit: Assert both params. @@ +82,5 @@ > + nsCOMPtr<nsIFile> journalDirectory; > + rv = aDirectory->Clone(getter_AddRefs(journalDirectory)); > + NS_ENSURE_SUCCESS(rv, rv); > + > + rv = journalDirectory->Append(NS_LITERAL_STRING("journals")); Nit: You use "journals" in several places, please make this a #define so that we will only ever have to change this in one spot in the future. @@ +95,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + NS_ENSURE_TRUE(isDirectory, NS_ERROR_FAILURE); > + } > + else { > + rv = journalDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755); Hm. Why do you need to create this here? Can't you wait until you are actually making a journal entry? It will mean that you do extra work at the next startup (since you wouldn't try to enumerate the directory otherwise). @@ +237,5 @@ > +// static > +nsresult > +FileManager::InitDirectory(mozIStorageServiceQuotaManagement* aService, > + nsIFile* aDirectory, nsIFile* aDatabaseFile, > + FactoryPrivilege aPrivilege) Nit: One arg per line. And please assert them. ::: dom/indexedDB/IDBDatabase.cpp @@ +904,5 @@ > > mFileInfo = fileManager->GetNewFileInfo(); > NS_ENSURE_TRUE(mFileInfo, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > + int64_t fileId = mFileInfo->Id(); Nit: make this a const ref? No need for a stack var. @@ +906,5 @@ > NS_ENSURE_TRUE(mFileInfo, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > + int64_t fileId = mFileInfo->Id(); > + > + nsCOMPtr<nsIFile> directory = fileManager->GetJournalDirectory(); FYI if you do lazy "journals" dir creation then you'll need to create the dir here if it doesn't exist. ::: dom/indexedDB/IDBObjectStore.cpp @@ +2664,5 @@ > nsCOMPtr<nsIFile> nativeFile = > + fileManager->GetFileForId(journalDirectory, id); > + NS_ENSURE_TRUE(nativeFile, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > + > + rv = nativeFile->Create(nsIFile::NORMAL_FILE_TYPE, 0644); You'll have to do the dir creation here too. ::: dom/indexedDB/IDBTransaction.cpp @@ +1059,5 @@ > + nsresult rv = function.ErrorCode(); > + NS_ENSURE_SUCCESS(rv, rv); > + > + rv = CreateJournals(mJournalsToCreateBeforeCommit, > + mJournalsToRemoveAfterAbort); So it seems to me that CreateJournals doesn't need to take any args at all. It's a method and has access to all these args. @@ +1070,5 @@ > +UpdateRefcountFunction::DidCommit() > +{ > + mFileInfoEntries.EnumerateRead(FileInfoUpdateCallback, nullptr); > + > + nsresult rv = RemoveJournals(mJournalsToRemoveAfterCommit); Same here. ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +1034,5 @@ > > void > +IndexedDatabaseManager::AddFileManager(const nsACString& aOrigin, > + const nsAString& aDatabaseName, > + FileManager* aFileManager) Nit: Assert aFileManager @@ +1040,5 @@ > + nsTArray<nsRefPtr<FileManager> >* array; > + if (!mFileManagers.Get(aOrigin, &array)) { > + nsAutoPtr<nsTArray<nsRefPtr<FileManager> > > newArray( > + new nsTArray<nsRefPtr<FileManager> >()); > + mFileManagers.Put(aOrigin, newArray); Nit: Put() is infallible now so the autoptr is not necessary. @@ +1885,5 @@ > > return NS_OK; > } > > +IndexedDatabaseManager::AsyncDeleteFileRunnable::AsyncDeleteFileRunnable( Nit: IndexedDatabaseManager:: AsyncDeleteFileRunnable::AsyncDeleteFileRunnable(...)

review comments addressed

(In reply to ben turner [:bent] from comment #11) > ::: dom/indexedDB/IDBTransaction.cpp > @@ +1059,5 @@ > > + nsresult rv = function.ErrorCode(); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + rv = CreateJournals(mJournalsToCreateBeforeCommit, > > + mJournalsToRemoveAfterAbort); > > So it seems to me that CreateJournals doesn't need to take any args at all. > It's a method and has access to all these args. ok, fixed > > @@ +1070,5 @@ > > +UpdateRefcountFunction::DidCommit() > > +{ > > + mFileInfoEntries.EnumerateRead(FileInfoUpdateCallback, nullptr); > > + > > + nsresult rv = RemoveJournals(mJournalsToRemoveAfterCommit); > > Same here. > can't do that here, since we need to go through different arrays in DidCommit() and DidAbort()

Comment on attachment 655037 [details] [diff] [review] patch v0.7 Review of attachment 655037 [details] [diff] [review]: ----------------------------------------------------------------- Just add an assertion to EnsureJournal that you're not on the main thread. Looks good!