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.

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

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 on attachment 8553694 [details] [diff] [review] patch Review of attachment 8553694 [details] [diff] [review]: ----------------------------------------------------------------- WaitForFileHandlesToFinishRunnable can be now removed, will fix that

More stuff needed for directory locks to work properly.

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.

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

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