763854 - Check file references (cleanup stored files) only when needed

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jan Varga [:janv]]

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.

Comment 1

[Jan Varga [:janv]]

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.

Comment 2

[Jan Varga [:janv]]

Attachment: patch v0.1

Comment 3

[Jan Varga [:janv]]

Attachment: patch v0.2

rebased patch

Comment 4

[Jan Varga [:janv]]

Attachment: patch v0.3

Forgot to create journals for newly created file handles.

Comment 5

[Jan Varga [:janv]]

Attachment: patch v0.4

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

Comment 6

[Jan Varga [:janv]]

Attachment: interdiff 0.4 - 0.5

Comment 7

[Jan Varga [:janv]]

Attachment: patch 0.6

Rebased patch (patch for bug 780625 caused many conflicts)

Comment 8

[Jan Varga [:janv]]

Attachment: patch 0.6

rebased patch

Comment 9

[Jan Varga [:janv]]

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

Comment 10

[Jan Varga [:janv]]

Attachment: patch 0.6

- rebase (PR types conversion)

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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(...)

Comment 12

[Jan Varga [:janv]]

Attachment: patch v0.7

review comments addressed

Comment 13

[Jan Varga [:janv]]

Attachment: interdiff v0.6 - v0.7

Comment 14

[Jan Varga [:janv]]

(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 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!

Comment 16

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c44596f22cf

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c44596f22cf