1125102 - Make QuotaManager and FileService to be independent of each other

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jan Varga [:janv]]

So, as we discussed, new quota manager is more important than file handle on pbackground. However, some work related to file handle is still needed to be done.
Also, the original new quota manager patch is quite big, so I'm going to try to provide smaller ones and maybe even in separate bugs. This is one of them.

Comment 1

[Jan Varga [:janv]]

Attachment: patch

Description:
- FileService not using nsIOfflineStorage
- FileService not using stream transport service
- FileService created and destroyed by IDB only
- QuotaManager not using FileService

So it will be up to IDB to check active transactions and file handles before releasing a directory lock (new quota manager object used to coordinate access to origin directories).

Comment 2

[Jan Varga [:janv]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70b2477d374f

Those xpcshell failures are not related to this patch. See:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=494632b9afed

Comment 3

[Jan Varga [:janv]]

Comment on attachment 8553694 [details] [diff] [review]
patch

Review of attachment 8553694 [details] [diff] [review]:
-----------------------------------------------------------------

WaitForFileHandlesToFinishRunnable can be now removed, will fix that

Comment 4

[Jan Varga [:janv]]

Attachment: patch

More stuff needed for directory locks to work properly.

Comment 5

[Jan Varga [:janv]]

Attachment: patch

This patch is based on latest m-c plus all bent's patches for WAL, WITHOUT ROWID, etc.
I created a new helper class WaitForTransactionsHelper since the same logic is now used in other places in ActorsParent.cpp. I didn't merge it with QuotaClient::WaitForTransactionsRunnable since the latter will be removed by the patch for directory locks.

Also, see this post:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/v2MePAm0YFQ
It seems that creating own thread pool for file handles has other benefits, not just simplification of our code.

Comment 6

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #5)
> Also, see this post:
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/v2MePAm0YFQ
> It seems that creating own thread pool for file handles has other benefits,
> not just simplification of our code.

It turned out there was some confusion about socket/stream transport service.

Comment 7

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

Comment on attachment 8583690 [details] [diff] [review]
patch

Review of attachment 8583690 [details] [diff] [review]:
-----------------------------------------------------------------

This look fine, I would just rather we reuse WaitForTransactionsRunnable somehow, so r- for now.

::: dom/filehandle/FileService.cpp
@@ +16,4 @@
>  #include "nsNetCID.h"
>  #include "nsServiceManagerUtils.h"
>  #include "nsThreadUtils.h"
> +#include "nsXPCOMCIDInternal.h"

You could just include nsThreadPool.h directly and make a new one without going through the component manager.

@@ +22,5 @@
>  namespace dom {
>  
>  namespace {
>  
> +const uint32_t kThreadLimit = 20;

This seems excessive... 5?

@@ +30,1 @@
>  FileService* gInstance = nullptr;

Could make this a StaticAutoPtr? Then you wouldn't need a manual delete call below.

@@ +40,5 @@
>  
>  FileService::~FileService()
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(gInstance == this);

This assert will fail if you're being deleted from the nsAutoPtr (e.g. if Init() fails).

@@ +121,5 @@
>      return nullptr;
>    }
>  
>    if (!gInstance) {
> +    nsAutoPtr<FileService> service(new FileService);

Eek, add () to that constructor call

::: dom/indexedDB/ActorsParent.cpp
@@ +5470,5 @@
>    DeallocPBackgroundIDBDatabaseParent(PBackgroundIDBDatabaseParent* aActor)
>                                        override;
>  };
>  
> +class WaitForTransactionsHelper final

Hrm, isn't this pretty much a copy of QuotaClient::WaitForTransactionsRunnable? Can we not just have one?

@@ +5550,5 @@
>    const nsCString mOrigin;
>    const nsCString mId;
>    const nsString mFilePath;
>    const PersistenceType mPersistenceType;
> +  uint64_t mFileHandleCount;

uint32_t should be fine (we'll run out of memory long before you hit UINT32_MAX filehandles).

I'd also move it before mPersistenceType so this will pack better. Need to adjust the initializer list order too.

@@ +11615,5 @@
>  
>    mTransactions.RemoveEntry(aTransaction);
>  
> +  if (!mTransactions.Count() && !mFileHandleCount && IsClosed() &&
> +      mOfflineStorage) {

Nit: Since the whole clause won't fit on one line you should put each condition on its own line.

There's another spot just like this below.

@@ +12016,5 @@
> +bool
> +Database::RecvNewFileHandle()
> +{
> +  AssertIsOnBackgroundThread();
> +  MOZ_ASSERT(mOfflineStorage);

I think this will have to be ASSERT_UNLESS_FUZZING

@@ +12018,5 @@
> +{
> +  AssertIsOnBackgroundThread();
> +  MOZ_ASSERT(mOfflineStorage);
> +
> +  ++mFileHandleCount;

You should return false if mFileHandleCount would overflow here.

@@ +12028,5 @@
> +Database::RecvFileHandleFinished()
> +{
> +  AssertIsOnBackgroundThread();
> +
> +  --mFileHandleCount;

And here.

@@ +12031,5 @@
> +
> +  --mFileHandleCount;
> +
> +  if (!mTransactions.Count() && !mFileHandleCount && IsClosed() &&
> +      mOfflineStorage) {

Hrm, we have a few too many places that do this check now... How about make a Database::MaybeCloseConnection function that does this check?

Comment 8

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

Comment on attachment 8583690 [details] [diff] [review]
patch

Review of attachment 8583690 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/ActorsParent.cpp
@@ +5470,5 @@
>    DeallocPBackgroundIDBDatabaseParent(PBackgroundIDBDatabaseParent* aActor)
>                                        override;
>  };
>  
> +class WaitForTransactionsHelper final

So, since this is going away soon, I guess it's fine...

r=me with all the rest fixed.

Comment 9

[Jan Varga [:janv]]

Attachment: interdiff

Requesting review of the interdiff, just to be sure I did what you meant.

try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee54a557396b

Comment 10

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

Comment on attachment 8591692 [details] [diff] [review]
interdiff

Review of attachment 8591692 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: dom/indexedDB/ActorsParent.cpp
@@ +12006,5 @@
> +    return false;
> +  }
> +
> +  if (mFileHandleCount == UINT32_MAX) {
> +    return false;

ASSERT_UNLESS_FUZZING here too, and in RecvFileHandleFinished().

Comment 11

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c326d0ba80

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2c326d0ba80