Make QuotaManager and FileService to be independent of each other

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: janv, Assigned: janv)

Tracking

(Blocks: 1 bug)

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 942542
(Assignee)

Comment 1

4 years ago
Created attachment 8553694 [details] [diff] [review]
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).
Assignee: nobody → Jan.Varga
Attachment #8553694 - Flags: review?(bent.mozilla)
(Assignee)

Comment 3

4 years ago
Comment on attachment 8553694 [details] [diff] [review]
patch

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

WaitForFileHandlesToFinishRunnable can be now removed, will fix that
(Assignee)

Comment 4

4 years ago
Created attachment 8561041 [details] [diff] [review]
patch

More stuff needed for directory locks to work properly.
Attachment #8553694 - Attachment is obsolete: true
Attachment #8553694 - Flags: review?(bent.mozilla)
Attachment #8561041 - Flags: review?(bent.mozilla)
(Assignee)

Comment 5

4 years ago
Created attachment 8583690 [details] [diff] [review]
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.
Attachment #8561041 - Attachment is obsolete: true
Attachment #8561041 - Flags: review?(bent.mozilla)
Attachment #8583690 - Flags: review?(bent.mozilla)
(Assignee)

Comment 6

4 years ago
(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?
Attachment #8583690 - Flags: review?(bent.mozilla) → review-
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.
Attachment #8583690 - Flags: review- → review+
(Assignee)

Comment 9

4 years ago
Created attachment 8591692 [details] [diff] [review]
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
Attachment #8591692 - Flags: review?(bent.mozilla)
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().
Attachment #8591692 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/c2c326d0ba80
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.