Closed Bug 1367309 Opened 7 years ago Closed 7 years ago

DOM Cache should update usage after removing files.

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: tt, Assigned: tt)

References

Details

Attachments

(2 files, 18 obsolete files)

7.40 KB, patch
Details | Diff | Splinter Review
18.70 KB, patch
Details | Diff | Splinter Review
Right now, DOM cache doesn't update its usage to QuotaManager while removing files. We should update it when removing files as well.

See Bug 1290481 comment 51, Bug 1290481 comment 52, Bug 1290481 comment 53, Bug 1290481 comment 54, Bug 1290481 comment 55
Attached patch opaque-p1.patch (obsolete) — Splinter Review
Approach: open the file through the writable stream and then remove it via nsIFile->Remove().

However, it will fail on Windows since the file is locked while removing it.
(In reply to Tom Tung [:tt] from comment #1)
Also I'm not sure if I can open a directory through the writable stream 
(e.g. nsCOMPtr<nsIOutputStream> fileStream = FileOutputStream::Create(PERSISTENCE_TYPE_DEFAULT, aQuotaInfo.mGroup, aQuotaInfo.mOrigin, aDirectory);).

I may need to deal with these cases since DOM Cache might remove directories [1].

[1] http://searchfox.org/mozilla-central/source/dom/cache/FileUtils.cpp#386-388
Attached patch opaque-p1.patch (obsolete) — Splinter Review
Fix locking problem in Windows based on Jan's feedback(close it before deleting and traverse all the entries when deleting a directory) at the meeting yesterday. 

Try to pass DEFER_OPEN, but it doesn't work. The reason is that it will make output stream not be able to open the file before close it. And, it means we will not update the usage to 0 when we delete that file.

Todo: 
nsIFile->Rename() seems to update its usage from the original file to the newer one to QuotaManager, but I'll take a look at it and make sure it does.
Attachment #8870691 - Attachment is obsolete: true
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b0eece59b8f341c5826502d0f22204b618074b3&selectedJob=101920605
 
Somehow DeleteFile() is linked to DeleteFileW() in Windows. I'll try to add namespace for preventing that.
Similar issue has happened many times with other Windows APIs too.
I think for example http://searchfox.org/mozilla-central/source/dom/interfaces/core/nsIDOMDocument.idl#11
(In reply to Olli Pettay [:smaug] from comment #5)
> Similar issue has happened many times with other Windows APIs too.
> I think for example
> http://searchfox.org/mozilla-central/source/dom/interfaces/core/
> nsIDOMDocument.idl#11

Oh, I see. Thanks for the example!
I was afraid that we should update the path for QuotaObject when renaming a file. However, I don't need to take care of it in DOM Cache.

After investigating, I reckon I don't need to take care of renaming morgue file in DOM Cache. The reason is that DOM Cache release outputStream which keeps the only reference for the QuotaObject right after writing the file. (OriginInfo keep its QuotaObject with weak reference) 

e.g.
Any cache.add/cache.put
- Create morgue/xxx.tmp and update its usage to quotaManger by QuotaObject -> Release the reference and remove that QuotaObject -> Rename morgue/xxx.tmp to morgue/xxx.final (QuotaManager know there is something occupies xxx kB but don't know who does that)

Any cache operation if read/write to that xxx.final file
- Update the usage to QuotaManager (maybe create a new quotaObject or just update usage by a function in QuotaManager).

Therefore, we won't have a QuotaObject with a wrong path for its morgue file after renaming a file. Moreover, the current mechanism for tracking usage works fine when renaming files in DOM Cache.
After considering, I would like to update usage by calling DecreaseUsageForOrigin() rather than opening it by outputStream. We can call DecreaseUsageForOrigin() after removing the file to avoid error occuring in removing. However, since opening file by output stream to update usage needs to "open" the file, we cannot remove it before opening it. Thus, I'd like to choose update usage by calling DecreaseUsageForOrigin().
In this patch, I update the file usage when deleting the file by calling QM::DecreaseUsageForOrigin(). The reason is to ensure the nsIFile->Remove() working, and then update the usage to QuotaManager.

Besides, to do this, I pass the QuotaInfo into FileUtils get the group and origin. 

I also wrote two utils function (DeleteFile(), DeleteDirectory()) to deal with the similar logic for removing.

Hi Ben, 
Could you help me to review this patch, especially the code to get & pass the QuotaInfo and the code to delete things behave the same? I want to make sure I do the correct things and do them in right way. Thanks!

Hi Jan, 
Could you help me to review this patch, especially the code for DelteFile() and DeleteDirectory()? I want to ensure I update the usage in an appropriate way. Thanks!
Attachment #8872270 - Attachment is obsolete: true
Attachment #8872312 - Flags: review?(jvarga)
Attachment #8872312 - Flags: review?(bkelly)
Comment on attachment 8872312 [details] [diff] [review]
Bug 1367309: Update usage to QuotaManager when a morgue file is deleted in DOM Cache.

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

Is there any way to write a test that shows the disk space is reclaimed in the QM?

::: dom/cache/FileUtils.cpp
@@ +21,5 @@
>  namespace mozilla {
>  namespace dom {
>  namespace cache {
>  
> +using namespace mozilla::dom::quota;

Please follow the existing style of a separate using statement for each symbol imported.  It makes it easier to find where things come from.

@@ +466,5 @@
>    nsCOMPtr<nsIFile> marker;
>    nsresult rv = GetMarkerFileHandle(aQuotaInfo, getter_AddRefs(marker));
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +  rv = DeleteFile(aQuotaInfo, marker);

This should probably be MOZ_ASSERT(NS_SUCCEEDED(rv)) like the other DeleteFile() calls in this file.

@@ +535,5 @@
> +DeleteFile(const QuotaInfo& aQuotaInfo, nsIFile* aFile)
> +{
> +  MOZ_DIAGNOSTIC_ASSERT(aFile);
> +
> +  int64_t fileSize;

Please init to 0.

::: dom/cache/Manager.cpp
@@ +975,5 @@
>    // accessed from any thread while mMutex locked
>    Mutex mMutex;
>    nsTArray<nsCOMPtr<nsISupports>> mCopyContextList;
> +
> +  QuotaInfo mQuotaInfo;

Please make this a Maybe<QuotaInfo>.  Then you can set it with mQuotaInfo.emplace(aQuotaInfo) and use it with mQuotaInfo.ref().  That way if we ever accidentally try to access it before its set then we will get an assertion.
Attachment #8872312 - Flags: review?(bkelly) → review+
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #11)

Thanks for the review and feedback! 

> Is there any way to write a test that shows the disk space is reclaimed in
> the QM?

I'll try to write a test to ensure the disk space is reclaimed.

Maybe I could use an API to monitor the disk space under morgue directory not to be double when adding same request and response pair twice. 

> ::: dom/cache/FileUtils.cpp
> @@ +21,5 @@
> >  namespace mozilla {
> >  namespace dom {
> >  namespace cache {
> >  
> > +using namespace mozilla::dom::quota;
> 
> Please follow the existing style of a separate using statement for each
> symbol imported.  It makes it easier to find where things come from.

Okay, sorry for that. I'll correct it!

> @@ +466,5 @@
> >    nsCOMPtr<nsIFile> marker;
> >    nsresult rv = GetMarkerFileHandle(aQuotaInfo, getter_AddRefs(marker));
> >    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> >  
> > +  rv = DeleteFile(aQuotaInfo, marker);
> 
> This should probably be MOZ_ASSERT(NS_SUCCEEDED(rv)) like the other
> DeleteFile() calls in this file.

Sure.

> @@ +535,5 @@
> > +DeleteFile(const QuotaInfo& aQuotaInfo, nsIFile* aFile)
> > +{
> > +  MOZ_DIAGNOSTIC_ASSERT(aFile);
> > +
> > +  int64_t fileSize;
> 
> Please init to 0.

Sure.

> ::: dom/cache/Manager.cpp
> @@ +975,5 @@
> >    // accessed from any thread while mMutex locked
> >    Mutex mMutex;
> >    nsTArray<nsCOMPtr<nsISupports>> mCopyContextList;
> > +
> > +  QuotaInfo mQuotaInfo;
> 
> Please make this a Maybe<QuotaInfo>.  Then you can set it with
> mQuotaInfo.emplace(aQuotaInfo) and use it with mQuotaInfo.ref().  That way
> if we ever accidentally try to access it before its set then we will get an
> assertion.

Oh, I see! Learn a lesson from it!
(In reply to Tom Tung [:tt] from comment #12)
> I'll try to write a test to ensure the disk space is reclaimed.
> 
> Maybe I could use an API to monitor the disk space under morgue directory
> not to be double when adding same request and response pair twice. 

I'm thinking to write a test to ensure we release the space by using getUsageForPrincipal()[1]. 

If we call this function without setting aGetGroupUsage, it will return origin usage and origin limit by calculating disk space.
If we set aGetGroupUsage to true, it will return group limit and group usage by calculating in-memory data.

We can ensure in-memory data and the disk storage have the same usage by calling it twice with/without setting the aGetGroupUsage.

[1] http://searchfox.org/mozilla-central/source/dom/quota/nsIQuotaManagerService.idl#74
Tom, can you just do something like this test?

https://dxr.mozilla.org/mozilla-central/source/dom/cache/test/mochitest/test_cache_orphaned_body.html

I forgot about this one.  It looks at how much storage is used to verify we cleanup orphaned body files.  It uses a QM reset, though.

It seems like maybe you could do something similar?
Flags: needinfo?(ttung)
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #14)
> Tom, can you just do something like this test?
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/cache/test/mochitest/
> test_cache_orphaned_body.html
> 
> I forgot about this one.  It looks at how much storage is used to verify we
> cleanup orphaned body files.  It uses a QM reset, though.
> 
> It seems like maybe you could do something similar?

Sorry, I'm not pretty sure what you meant. Did you mean having a test to verify the usage does decrease after adding the same request, response pair? If so, how about verifying it by checking the QM have the same usage from both in-memory record and the disk record?

DOM Cache does delete files by nsIFile::Remove(), but it doesn't update its decreasing usage to QM. So, I was thinking to write a test to verify DOM Cache does update its usage to QM. 

One way to do that is to ensure QM getting correct usage from in-memory data(OriginInfo[1]). To do this, I'd like to use API in QMS (getUsageFromPrincipal()) to check usage from both the disk[2] and in-memory[3] are the same. If they have the same value, it indicates DOM Cache update the usage each time when it adds/deletes file. 

Does it sound good to you?

This test can be applied to the other quota clients (e.g. indexedDB) to ensure they do update their usage correctly.

[1] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#555
[2] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#6735-6864
[3] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5252-5277
Flags: needinfo?(ttung) → needinfo?(bkelly)
Oh, I didn't realize the APIs used in that test bypassed the QM in-memory size estimate.  I'll defer to you and Jan on best way to write a test.  Thanks!
Flags: needinfo?(bkelly)
Apply Ben's comment and update the patch.

Sorry if you are reviewing the obsoleted patch, Jan.
Attachment #8872312 - Attachment is obsolete: true
Attachment #8872312 - Flags: review?(jvarga)
Attachment #8873270 - Flags: review?(jvarga)
(In reply to Tom Tung [:tt] from comment #19)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=512e449e1cbbf3c06ef59a7bb55fa949eb8d5ca8

It seems try server have an issue, so re-push try again.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=85f7b56412f0b5f3e4ee7968ef6906d8498d4337&selectedJob=103935825
Try looks good, so I'm sending the review request!

Hi Ben, Jan,

This test mainly ensures DOM Cache update its usage to QuotaManager when its operations create or delete files. To do this, I use getUsageFromPrincipal() with the different flag to calculate usage twice (from memory object at the first time and from the disk at the second time) and make sure they are the same in the test. Could you help me to review this patch? Thanks!

Note: this test will fail without P1 since DOM Cache didn't update its usage with delete morgue files.
Attachment #8873325 - Attachment is obsolete: true
Attachment #8873786 - Flags: review?(jvarga)
Attachment #8873786 - Flags: review?(bkelly)
Attachment #8873786 - Attachment description: cache-delete-p2.patch → Bug 1367309 - P2: Test for ensuring DOM Cache update usage to QuotaManager.
Comment on attachment 8873786 [details] [diff] [review]
Bug 1367309 - P2: Test for ensuring DOM Cache update usage to QuotaManager.

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

Looks good.  Thanks!

::: dom/cache/test/mochitest/test_cache_updateUsage.html
@@ +92,5 @@
> +SpecialPowers.pushPrefEnv({
> +  "set": [["dom.caches.enabled", true],
> +          ["dom.caches.testing.enabled", true],
> +          ["dom.quotaManager.testing", true]]
> +}, function() {

You could make this `async function()` and then use `await` instead of the .then() chain below.
Attachment #8873786 - Flags: review?(bkelly) → review+
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #22)
Thanks for the review!
 
> You could make this `async function()` and then use `await` instead of the
> .then() chain below.

Sure! It makes the test much easier to read, thanks!
Update patch to address Ben's comment.
Attachment #8873786 - Attachment is obsolete: true
Attachment #8873786 - Flags: review?(jvarga)
Attachment #8874287 - Flags: review?(jvarga)
Comment on attachment 8873270 [details] [diff] [review]
Bug 1367309 - P1: Update usage to QuotaManager when a morgue file is deleted in DOM Cache. r=bkelly r?janv

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

This looks mostly ok, but I'd like to see updated and retested patch before final r+.

::: dom/cache/FileUtils.cpp
@@ +80,5 @@
>  
>    rv = aBodyDir->Append(NS_LITERAL_STRING("morgue"));
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +  rv = DeleteDirectory(aQuotaInfo, aBodyDir);

What will happen if the body dir doesn't exist ?
The old code converts that case to NS_OK. What will DeleteDirectory() do ?

@@ +500,5 @@
> +  bool isDirectory;
> +  nsresult rv = aDirectory->IsDirectory(&isDirectory);
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +  if (!isDirectory) {

Ah, so this is actually not DeleteDirectory/RemoveDirectory.
It should be named RemoveNsIFileRecursively. aDirectory should be changed to aFile too.

@@ +516,5 @@
> +    nsCOMPtr<nsISupports> entry;
> +    rv = entries->GetNext(getter_AddRefs(entry));
> +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +    nsCOMPtr<nsIFile> childDirectory = do_QueryInterface(entry);

We don't know if it is a directory.

@@ +521,5 @@
> +    MOZ_ASSERT(childDirectory);
> +
> +    rv = DeleteDirectory(aQuotaInfo, childDirectory);
> +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +  }

You need:
if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
after the loop.

@@ +524,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +  }
> +
> +  // In the end, remove the folder
> +  rv = aDirectory->Remove(/* recursive */ true);

In the end, there shouldn't be any files/dirs in the folder. So we don't have to pass |true| here.

@@ +547,5 @@
> +  rv = aFile->Remove( /* recursive */ false);
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +  QuotaManager* quotaManager = QuotaManager::Get();
> +  if (quotaManager) {

Hm, this should be an assert. We shouldn't be touching stuff in <profile>/storage if there's no quota manager.
But maybe there's something in DOM cache I'm not aware of, so please test on try server first (the assertion).

::: dom/cache/FileUtils.h
@@ +67,5 @@
> +nsresult
> +DeleteDirectory(const QuotaInfo& aQuotaInfo, nsIFile* aDirectory);
> +
> +nsresult
> +DeleteFile(const QuotaInfo& aQuotaInfo, nsIFile* aFile);

These helpers could be RemoveDirectory() and RemoveFile(). Then you won't need to undef DeleteFile() in each file that includes FileUtils.h

Or, they can be named:
RemoveNsIFile()
RemoveNsIFileRecursively()
Attachment #8873270 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #25)
> This looks mostly ok, but I'd like to see updated and retested patch before
> final r+.

Thanks for the reveiw and feedback! 
 
> ::: dom/cache/FileUtils.cpp
> @@ +80,5 @@
> >  
> >    rv = aBodyDir->Append(NS_LITERAL_STRING("morgue"));
> >    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> >  
> > +  rv = DeleteDirectory(aQuotaInfo, aBodyDir);
> 
> What will happen if the body dir doesn't exist ?
> The old code converts that case to NS_OK. What will DeleteDirectory() do ?

Thanks for catching this! I thought it will be returned to deletefile(), but I was wrong.

> @@ +500,5 @@
> > +  bool isDirectory;
> > +  nsresult rv = aDirectory->IsDirectory(&isDirectory);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> > +
> > +  if (!isDirectory) {
> 
> Ah, so this is actually not DeleteDirectory/RemoveDirectory.
> It should be named RemoveNsIFileRecursively. aDirectory should be changed to
> aFile too.

Sure.
 
> @@ +516,5 @@
> > +    nsCOMPtr<nsISupports> entry;
> > +    rv = entries->GetNext(getter_AddRefs(entry));
> > +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> > +
> > +    nsCOMPtr<nsIFile> childDirectory = do_QueryInterface(entry);
> 
> We don't know if it is a directory.

Sure, I'll rename it to "file".

> @@ +521,5 @@
> > +    MOZ_ASSERT(childDirectory);
> > +
> > +    rv = DeleteDirectory(aQuotaInfo, childDirectory);
> > +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> > +  }
> 
> You need:
> if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> after the loop.

Oh, sorry for that... I shouldn't miss this.

> @@ +524,5 @@
> > +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> > +  }
> > +
> > +  // In the end, remove the folder
> > +  rv = aDirectory->Remove(/* recursive */ true);
> 
> In the end, there shouldn't be any files/dirs in the folder. So we don't
> have to pass |true| here.

Sure.

> @@ +547,5 @@
> > +  rv = aFile->Remove( /* recursive */ false);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> > +
> > +  QuotaManager* quotaManager = QuotaManager::Get();
> > +  if (quotaManager) {
> 
> Hm, this should be an assert. We shouldn't be touching stuff in
> <profile>/storage if there's no quota manager.
> But maybe there's something in DOM cache I'm not aware of, so please test on
> try server first (the assertion).

You're right. We init the QuotaManager before opening directory, so it should be the assertion. I'll change "if" to the assertion.

> ::: dom/cache/FileUtils.h
> @@ +67,5 @@
> > +nsresult
> > +DeleteDirectory(const QuotaInfo& aQuotaInfo, nsIFile* aDirectory);
> > +
> > +nsresult
> > +DeleteFile(const QuotaInfo& aQuotaInfo, nsIFile* aFile);
> 
> These helpers could be RemoveDirectory() and RemoveFile(). Then you won't
> need to undef DeleteFile() in each file that includes FileUtils.h
> 
> Or, they can be named:
> RemoveNsIFile()
> RemoveNsIFileRecursively()

I'll rename them to RemoveNsIFile() and RemoveNsIFileRecursively(). Thanks!
Update patch and address comment
Attachment #8873270 - Attachment is obsolete: true
Attachment #8875093 - Flags: review?(jvarga)
Comment on attachment 8875093 [details] [diff] [review]
Bug 1367309 - P1: Update usage to QuotaManager when a morgue file is deleted in DOM Cache. r=bkelly r?janv

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

::: dom/cache/DBAction.cpp
@@ +198,4 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
>    // Note, the -wal journal file will be automatically deleted by sqlite when
>    // the new database is created.  No need to explicitly delete it here.

Hm, I think we should remove the WAL file explicitly here (like DeleteDatabaseOp::VersionChangeOp::RunOnIOThread does in dom/indexedDB/ActorsParent.cpp).
But not in this bug. Feel free to file a new bug, though I don't think it's a high priority. Do it only if you have time for that.

Hm, DeleteDatabaseOp::VersionChangeOp::DeleteFile() is quite similar to you new helper methods, we might want to move these helpers to dom/quota and use them in quota clients.
Feel free to file a bug for that too.

::: dom/cache/FileUtils.cpp
@@ +349,5 @@
>      if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
>      // If a file got in here somehow, try to remove it and move on
>      if (NS_WARN_IF(!isDir)) {
> +      rv = RemoveNsIFile(aQuotaInfo, subdir);

Here ...

@@ +376,5 @@
>        // Delete all tmp files regardless of known bodies.  These are
>        // all considered orphans.
>        if (StringEndsWith(leafName, NS_LITERAL_CSTRING(".tmp"))) {
>          // remove recursively in case its somehow a directory
> +        rv = RemoveNsIFileRecursively(aQuotaInfo, file);

, here ...

@@ +399,5 @@
>        }
>  
>        if (!aKnownBodyIdList.Contains(id)) {
>          // remove recursively in case its somehow a directory
> +        rv = RemoveNsIFileRecursively(aQuotaInfo, file);

and here we don't check if |rv == NS_ERROR_FILE_NOT_FOUND| because we are actually looping over directory entries.
We could add a bool flag to the helper methods but I think it's not worth it.
Attachment #8875093 - Flags: review?(jvarga) → review+
Comment on attachment 8874287 [details] [diff] [review]
Bug 1367309 - P2: Test for ensuring DOM Cache update usage to QuotaManager. r=bkelly. r?janv

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

This looks good at first glance.

However I'm a bit worried about one thing:
await cache.add(url);
await verifyUsage();

Do we know if all I/O for cache.add() is done by the time we call verifyUsage() ?
If there are still I/O operations in progress, then verifyUsage can report failures
Disk usage and memoryUsage can be different when calculated at different points in time.

::: dom/cache/test/mochitest/test_cache_updateUsage.html
@@ +53,5 @@
> +  });
> +}
> +
> +// Although it returns group usage, it calculate the usage from in-memory data.
> +function storageUsageFromMemory() {

storageUsageFromDisk() and storageUsageFromMemory() are almost identical.
Could we have a one function with a bool flag instead ?
(In reply to Jan Varga [:janv] from comment #29)

Thanks for the reviews!

> ::: dom/cache/DBAction.cpp
> @@ +198,4 @@
> >    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> >  
> >    // Note, the -wal journal file will be automatically deleted by sqlite when
> >    // the new database is created.  No need to explicitly delete it here.
> 
> Hm, I think we should remove the WAL file explicitly here (like
> DeleteDatabaseOp::VersionChangeOp::RunOnIOThread does in
> dom/indexedDB/ActorsParent.cpp).
> But not in this bug. Feel free to file a new bug, though I don't think it's
> a high priority. Do it only if you have time for that.
> 
> Hm, DeleteDatabaseOp::VersionChangeOp::DeleteFile() is quite similar to you
> new helper methods, we might want to move these helpers to dom/quota and use
> them in quota clients.
> Feel free to file a bug for that too.

Sure, I'll file bugs for them.

> ::: dom/cache/FileUtils.cpp
> @@ +349,5 @@
> >      if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> >  
> >      // If a file got in here somehow, try to remove it and move on
> >      if (NS_WARN_IF(!isDir)) {
> > +      rv = RemoveNsIFile(aQuotaInfo, subdir);
> 
> Here ...
> 
> @@ +376,5 @@
> >        // Delete all tmp files regardless of known bodies.  These are
> >        // all considered orphans.
> >        if (StringEndsWith(leafName, NS_LITERAL_CSTRING(".tmp"))) {
> >          // remove recursively in case its somehow a directory
> > +        rv = RemoveNsIFileRecursively(aQuotaInfo, file);
> 
> , here ...
> 
> @@ +399,5 @@
> >        }
> >  
> >        if (!aKnownBodyIdList.Contains(id)) {
> >          // remove recursively in case its somehow a directory
> > +        rv = RemoveNsIFileRecursively(aQuotaInfo, file);
> 
> and here we don't check if |rv == NS_ERROR_FILE_NOT_FOUND| because we are
> actually looping over directory entries.
> We could add a bool flag to the helper methods but I think it's not worth it.

I noticed them, and I thought file shouldn't be not found in these cases. But after considering, I should keep the logic in the same. 

I'll add the flag for it.
(In reply to Tom Tung [:tt] from comment #31)
> I noticed them, and I thought file shouldn't be not found in these cases.
> But after considering, I should keep the logic in the same. 
> 
> I'll add the flag for it.

No, no, by "it's not worth it" I meant that you don't have to do it.
It was rather a self note.
(In reply to Jan Varga [:janv] from comment #30)

Thanks!

> However I'm a bit worried about one thing:
> await cache.add(url);
> await verifyUsage();
> 
> Do we know if all I/O for cache.add() is done by the time we call
> verifyUsage() ?

Sorry, I didn't think too much about it. I thought it should be done since dom::quota::GetUsageOp will touch IOThread before calculating and we do all the IO things in that thread. 

I'll check it out and make sure the ordering.

> If there are still I/O operations in progress, then verifyUsage can report
> failures
> Disk usage and memoryUsage can be different when calculated at different
> points in time.

Thanks for pointing it out!

> ::: dom/cache/test/mochitest/test_cache_updateUsage.html
> @@ +53,5 @@
> > +  });
> > +}
> > +
> > +// Although it returns group usage, it calculate the usage from in-memory data.
> > +function storageUsageFromMemory() {
> 
> storageUsageFromDisk() and storageUsageFromMemory() are almost identical.
> Could we have a one function with a bool flag instead ?

Sure. I was thinking to make them much readable by having different functions name, but I could have comments for it instead.
(In reply to Tom Tung [:tt] from comment #33)
> > However I'm a bit worried about one thing:
> > await cache.add(url);
> > await verifyUsage();
> > 
> > Do we know if all I/O for cache.add() is done by the time we call
> > verifyUsage() ?
> 
> Sorry, I didn't think too much about it. I thought it should be done since
> dom::quota::GetUsageOp will touch IOThread before calculating and we do all
> the IO things in that thread. 

Well, AFAIK, DOM cache does only initialization on that thread. I/O operations are
dispatched to the stream transport service IIRC.

> > @@ +53,5 @@
> > > +  });
> > > +}
> > > +
> > > +// Although it returns group usage, it calculate the usage from in-memory data.
> > > +function storageUsageFromMemory() {
> > 
> > storageUsageFromDisk() and storageUsageFromMemory() are almost identical.
> > Could we have a one function with a bool flag instead ?
> 
> Sure. I was thinking to make them much readable by having different
> functions name, but I could have comments for it instead.

Not a big deal, feel free to keep two separate functions.
(In reply to Jan Varga [:janv] from comment #34)
> > Sorry, I didn't think too much about it. I thought it should be done since
> > dom::quota::GetUsageOp will touch IOThread before calculating and we do all
> > the IO things in that thread. 
> 
> Well, AFAIK, DOM cache does only initialization on that thread. I/O
> operations are
> dispatched to the stream transport service IIRC.

I meant I have thought that we do any I/O operations in IOThread (e.g. write a file), and GetUsageOp() touches IO thread later. It implies the previous task in I/O thread is done. However, it might touch IO thread more than one times so I cannot guarantee that.
Please double check with bkelly.
(In reply to Jan Varga [:janv] from comment #30)
> However I'm a bit worried about one thing:
> await cache.add(url);
> await verifyUsage();
> 
> Do we know if all I/O for cache.add() is done by the time we call
> verifyUsage() ?
> If there are still I/O operations in progress, then verifyUsage can report
> failures
> Disk usage and memoryUsage can be different when calculated at different
> points in time.

Hi Ben,

Do we have any method to guarantee cache.add() done before checking usage in QuotaManger?

Jan discussed that with me in meeting. QMS::rest() [1] could do that, but it may abort ongoing operations. Or instead, I could implement some method in QMS which is similar to reset() but not abort ongoing operations.

[1] http://searchfox.org/mozilla-central/source/dom/quota/nsIQuotaManagerService.idl#116-117
Flags: needinfo?(bkelly)
(In reply to Tom Tung [:tt] from comment #37)
> (In reply to Jan Varga [:janv] from comment #30)
> Do we have any method to guarantee cache.add() done before checking usage in
> QuotaManger?

Cache.add() writes the body to the OS filesystem and completes its sqlite transaction before resolving its promise.  So the OS should know about all of those writes even if they are not flushed to disk.  I don't think you should have to do any additional waiting.
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #38)
> Cache.add() writes the body to the OS filesystem and completes its sqlite
> transaction before resolving its promise.  So the OS should know about all
> of those writes even if they are not flushed to disk.  I don't think you
> should have to do any additional waiting.

I see! Thanks for answering! 

BTW, I noticed that except the body, Cache.add() writes the header and some imporant information into caches.sqlite-wal file first and then resolve the promise. After a while, it'll move things in caches.sqlite-wal file to caches.sqlite and then remove the caches.sqlite-wal. I guessed that's why cache usage will decrease after a while.
It seems that Cache.delete() remove the body async [1][2], so the test in P2 might fail by running rr chaos mode. The failure happen when the orphaned body is removed between storageUsageFromMemory and storageUsageFromDisk.

I'll remove the verifyUsage between cache.delete() and resetStorage() in the P2. So, after that, I will only verify usage after resetStorage() for checking cache.delete().

[1] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#1016
[2] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#1920-1921
(In reply to Tom Tung [:tt] from comment #40)
> It seems that Cache.delete() remove the body async [1][2], so the test in P2
> might fail by running rr chaos mode. The failure happen when the orphaned
> body is removed between storageUsageFromMemory and storageUsageFromDisk.

So does the Cache.add() when deleting orphaned cached body (be stored in the previous Cache.add()) [3]. 

Thus, I will do the similar things for waiting Cache.delete finish to the Cache.add() at stage 4.2 to ensure removing action is completed.

[3] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#798
Attachment #8874287 - Flags: review?(jvarga)
Hi Jan,

I addressed your suggestion for merging two functions together. Besides, I also change the coded for making delete orphaned files in Cache (Cache.add() twice, Cache.delete()). I have run rr chaos mode 200 times without any error after applying this patch. Could you help me to review this patch? I will update interdiff patch later.

BTW, since Ben said that Cache.add() guarantee that writing the body is done before resolving the promise, I guess I don't need to change the test for that part.
Attachment #8874287 - Attachment is obsolete: true
Attachment #8875693 - Flags: review?(jvarga)
Attached patch interdiff patch for P2 (obsolete) — Splinter Review
Just curious, so the previous test sometimes failed in the chaos mode ?
(In reply to Jan Varga [:janv] from comment #44)
> Just curious, so the previous test sometimes failed in the chaos mode ?

Yeah, I put the reason in comment #40 comment #41. It's because Cache will not actually delete orphaned files before resolving the promise. And, it fails when Cache deleting them between getting usages.

So, I try to execute one more cache operation (Cache.match()) and use rest() to ensure it have deleted files in this patch.
Comment on attachment 8875693 [details] [diff] [review]
Bug 1367309 - P2 (v2): Test for ensuring DOM Cache update usage to QuotaManager. r=bkelly. r?janv

Hi Ben, 

I ran my patch in rr chaos mode and got the failure. I put the reason in comment #40 and comment #41.

Since I changed this test to make sure Cache deleting orphaned files before verifying usage. Could you help me review this patch again? Thanks!

I also updated inter-diff patch between the current patch and previous r+ patch.
Attachment #8875693 - Flags: review?(bkelly)
Update the commit message.
Attachment #8875093 - Attachment is obsolete: true
Attachment #8875953 - Flags: review+
Comment on attachment 8875694 [details] [diff] [review]
interdiff patch for P2

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

::: dom/cache/test/mochitest/test_cache_updateUsage.html
@@ +40,5 @@
>      request.callback = cb;
>    });
>  }
>  
> +function getStorageUsage(isFromMemoryData) {

Nit: Just "fromMemory" ?

@@ +59,5 @@
>  
>  async function verifyUsage() {
> +  // Although it returns group usage when passing true, it calculate the usage
> +  // from tracking usage object (in-memory object) in QuotaManager.
> +  let memoryUsage = await getStorageUsage(true);

Nit: let memoryUsage = await getStorageUsage(/* fromMemory */ true);

@@ +74,2 @@
>  
> +  // The following three lines ensure we've deleted orphaned body.

Nit: It would be nice to have a generic function that makes sure that all the runnables/IO operations are done.

@@ +163,5 @@
> +  // the action is done.
> +  // The following three lines ensure we've deleted orphaned body.
> +  response = await cache.match(url);
> +  ok(!!response, "Response shouldn't be undefined after cache.add.");
> +  await resetStorage();

I'm a bit surprised that resetStorage() doesn't cause any problems. resetStorage() flags existing operations as aborted/canceled first, then it waits for all operations to finish.

aExclusive is set to true here:
https://dxr.mozilla.org/mozilla-central/source/dom/quota/ActorsParent.cpp#1249

AbortOperations() called here (if aExclusive is set):
https://dxr.mozilla.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5035

So in theory, some existing operations can actually do no I/O.
This looks a bit fragile to me, but I'm not going to block this bug until the issue is addressed.

Something like Cache.close() and Cache.onclose would be ideal.
These would only exist if a pref is set.
(In reply to Jan Varga [:janv] from comment #49)
Thanks for explaining! 

> I'm a bit surprised that resetStorage() doesn't cause any problems.
> resetStorage() flags existing operations as aborted/canceled first, then it
> waits for all operations to finish.

I add this resetStorage() for some reasons. Let me explain to you.

Cache.add() resolves after it writes the body to morgue files and header(or some important) to cache.sqlite-wal. However, after a while, the cache.sqlite-wal will be replaced to cache.sqlite by TelemetryVFS. And usage changes at this time (write things into cache.sqlite first and then remove the cache.sqlite-wal).

I was afraid the verifyUsage() is executed among this replace(the usage change), so I add restStorage() to prevent this happen. I'll check out if I can remove this resetStorage().

> Something like Cache.close() and Cache.onclose would be ideal.
> These would only exist if a pref is set.

This sounds good to me. Should we have one similar to reset() but not abort the other operation in QMS/QM as you said in the last meeting?
(In reply to Tom Tung [:tt] from comment #50)
> > Something like Cache.close() and Cache.onclose would be ideal.
> > These would only exist if a pref is set.
> 
> This sounds good to me. Should we have one similar to reset() but not abort
> the other operation in QMS/QM as you said in the last meeting?

Yes, that might be another option, but we need to do more investigation before we start implementing it.
Please file a new bug for that.

Note, that resetStorage() affects everythig, all origins and all clients which is not ideal when running the browser.
It's fine in xpcshell tests.
Attachment #8875693 - Flags: review?(bkelly) → review+
Address comment so far and update patch. Besides, I remove part of the unnecessary code and add few more checks. I'll provide an interdiff patch to make it easier to review.

(In reply to Jan Varga [:janv] from comment #49)
> Nit: Just "fromMemory" ?

Done 
 
> @@ +59,5 @@
> Nit: let memoryUsage = await getStorageUsage(/* fromMemory */ true);

Done

> @@ +74,2 @@
> Nit: It would be nice to have a generic function that makes sure that all
> the runnables/IO operations are done.

Done, create an async function and name it to "cacheWaitIOComplete()"
 
> @@ +163,5 @@
> So in theory, some existing operations can actually do no I/O.
> This looks a bit fragile to me, but I'm not going to block this bug until
> the issue is addressed.
> 
> Something like Cache.close() and Cache.onclose would be ideal.
> These would only exist if a pref is set.

File Bug 1372454 for it, should it be tracked by meta bugs?
Attachment #8875693 - Attachment is obsolete: true
Attachment #8875693 - Flags: review?(jvarga)
Attachment #8877936 - Flags: review?(jvarga)
Attached patch interdiff-p2.patch (obsolete) — Splinter Review
Interdiff patch for P2
Attachment #8875694 - Attachment is obsolete: true
Comment on attachment 8877939 [details] [diff] [review]
interdiff-p2.patch

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

::: b/dom/cache/test/mochitest/test_cache_updateUsage.html
@@ +73,5 @@
> +  CACHE_ADD: 0,
> +  CACHE_DELETE: 1,
> +};
> +
> +async function cacheWaitIOComplete(cache, operation, request, otherRequest) {

What about just waitForIOToComplete(cache, requests) ?

@@ +92,5 @@
> +  // Finally, wait for -wal file finish its job.
> +  return await resetStorage();
> +}
> +
> +async function cacheDelete(cache, request, otherRequest){

I think this method doesn't buy us anything.

@@ +186,5 @@
>    info("Stage 5: Clean caches.");
>    await caches.delete(name);
>  
>    info("Stage 6: Final Check.");
> +  ok(emptyUsageFirst == emptyUsageSecond &&

Nit: What about just emptyUsage1, emptyUsage2, ... ?
Attachment #8877939 - Flags: review+
Attachment #8877936 - Flags: review?(jvarga) → review+
Address comment, but try to ensure the IO completed by creating one another task.
Attachment #8877936 - Attachment is obsolete: true
Attachment #8877939 - Attachment is obsolete: true
Comment on attachment 8881500 [details] [diff] [review]
Bug 1367309 - P2 (v4): Test for ensuring DOM Cache update usage to QuotaManager. r=bkelly. r?janv

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

::: dom/cache/test/mochitest/test_cache_updateUsage.html
@@ +68,5 @@
> +     "In-memory usage and disk usage should be the same.");
> +  return memoryUsage;
> +}
> +
> +async function cacheWaitIOComplete(cache, request) {

Don't forget to rename the method.
(In reply to Jan Varga [:janv] from comment #57)
> Don't forget to rename the method.

Oh, almost forget this... 
Thanks!
Comment on attachment 8881559 [details] [diff] [review]
[Final]Bug 1367309 - P2 : Test for ensuring DOM Cache update usage to QuotaManager. r=bkelly. r=janv

Update the helper funciton (waitForIOToComplete()) name.
Attachment #8881559 - Attachment description: Bug 1367309 - P2 (v4): Test for ensuring DOM Cache update usage to QuotaManager. r=bkelly. r=janv → [Final]Bug 1367309 - P2 : Test for ensuring DOM Cache update usage to QuotaManager. r=bkelly. r=janv
Attachment #8881559 - Flags: review+
Comment on attachment 8875953 [details] [diff] [review]
[Final]Bug 1367309 - P1: Update usage to QuotaManager when a file is deleted in DOM Cache. r=bkelly r=janv

># HG changeset patch
># User Tom Tung <shes050117@gmail.com>
># Date 1495535627 -28800
>#      Tue May 23 18:33:47 2017 +0800
># Node ID c186f0dcb5c82a35b167414e24d218498fb55654
># Parent  e3fbe145dafcc919baa422c1de5b2ed6935344e6
>Bug 1367309 - P1: Update usage to QuotaManager when a file is deleted in DOM Cache. r=bkelly r=janv
>
>This patch mainly to update DOM Cahce's usage to the QuotaManager when a file
>is deleted. This patch implement two helper function to delete the nsIFile
>recursively or directly and update usage to QuotaManager.
>
>diff --git a/dom/cache/DBAction.cpp b/dom/cache/DBAction.cpp
>--- a/dom/cache/DBAction.cpp
>+++ b/dom/cache/DBAction.cpp
>@@ -154,55 +154,59 @@ DBAction::OpenConnection(const QuotaInfo
>   rv = ss->OpenDatabaseWithFileURL(dbFileUrl, getter_AddRefs(conn));
>   if (rv == NS_ERROR_FILE_CORRUPTED) {
>     NS_WARNING("Cache database corrupted. Recreating empty database.");
> 
>     conn = nullptr;
> 
>     // There is nothing else we can do to recover.  Also, this data can
>     // be deleted by QuotaManager at any time anyways.
>-    rv = WipeDatabase(dbFile, aDBDir);
>+    rv = WipeDatabase(aQuotaInfo, dbFile, aDBDir);
>     if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>     rv = ss->OpenDatabaseWithFileURL(dbFileUrl, getter_AddRefs(conn));
>   }
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>   // Check the schema to make sure it is not too old.
>   int32_t schemaVersion = 0;
>   rv = conn->GetSchemaVersion(&schemaVersion);
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>   if (schemaVersion > 0 && schemaVersion < db::kFirstShippedSchemaVersion) {
>     conn = nullptr;
>-    rv = WipeDatabase(dbFile, aDBDir);
>+    rv = WipeDatabase(aQuotaInfo, dbFile, aDBDir);
>     if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>     rv = ss->OpenDatabaseWithFileURL(dbFileUrl, getter_AddRefs(conn));
>     if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>   }
> 
>   rv = db::InitializeConnection(conn);
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>   conn.forget(aConnOut);
> 
>   return rv;
> }
> 
> nsresult
>-DBAction::WipeDatabase(nsIFile* aDBFile, nsIFile* aDBDir)
>+DBAction::WipeDatabase(const QuotaInfo& aQuotaInfo, nsIFile* aDBFile,
>+                       nsIFile* aDBDir)
> {
>-  nsresult rv = aDBFile->Remove(false);
>+  MOZ_DIAGNOSTIC_ASSERT(aDBFile);
>+  MOZ_DIAGNOSTIC_ASSERT(aDBDir);
>+
>+  nsresult rv = RemoveNsIFile(aQuotaInfo, aDBFile);
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>   // Note, the -wal journal file will be automatically deleted by sqlite when
>   // the new database is created.  No need to explicitly delete it here.
> 
>   // Delete the morgue as well.
>-  rv = BodyDeleteDir(aDBDir);
>+  rv = BodyDeleteDir(aQuotaInfo, aDBDir);
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>   return rv;
> }
> 
> SyncDBAction::SyncDBAction(Mode aMode)
>   : DBAction(aMode)
> {
>diff --git a/dom/cache/DBAction.h b/dom/cache/DBAction.h
>--- a/dom/cache/DBAction.h
>+++ b/dom/cache/DBAction.h
>@@ -44,17 +44,18 @@ protected:
> private:
>   virtual void
>   RunOnTarget(Resolver* aResolver, const QuotaInfo& aQuotaInfo,
>               Data* aOptionalData) override;
> 
>   nsresult OpenConnection(const QuotaInfo& aQuotaInfo, nsIFile* aQuotaDir,
>                           mozIStorageConnection** aConnOut);
> 
>-  nsresult WipeDatabase(nsIFile* aDBFile, nsIFile* aDBDir);
>+  nsresult WipeDatabase(const QuotaInfo& aQuotaInfo, nsIFile* aDBFile,
>+                        nsIFile* aDBDir);
> 
>   const Mode mMode;
> };
> 
> class SyncDBAction : public DBAction
> {
> protected:
>   explicit SyncDBAction(Mode aMode);
>diff --git a/dom/cache/FileUtils.cpp b/dom/cache/FileUtils.cpp
>--- a/dom/cache/FileUtils.cpp
>+++ b/dom/cache/FileUtils.cpp
>@@ -2,16 +2,17 @@
> /* vim: set ts=8 sts=2 et sw=2 tw=80: */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "mozilla/dom/cache/FileUtils.h"
> 
> #include "mozilla/dom/quota/FileStreams.h"
>+#include "mozilla/dom/quota/QuotaManager.h"
> #include "mozilla/SnappyCompressOutputStream.h"
> #include "mozilla/Unused.h"
> #include "nsIFile.h"
> #include "nsIUUIDGenerator.h"
> #include "nsNetCID.h"
> #include "nsISimpleEnumerator.h"
> #include "nsServiceManagerUtils.h"
> #include "nsString.h"
>@@ -19,16 +20,17 @@
> 
> namespace mozilla {
> namespace dom {
> namespace cache {
> 
> using mozilla::dom::quota::FileInputStream;
> using mozilla::dom::quota::FileOutputStream;
> using mozilla::dom::quota::PERSISTENCE_TYPE_DEFAULT;
>+using mozilla::dom::quota::QuotaManager;
> 
> namespace {
> 
> enum BodyFileType
> {
>   BODY_FILE_FINAL,
>   BODY_FILE_TMP
> };
>@@ -58,32 +60,28 @@ BodyCreateDir(nsIFile* aBaseDir)
>   }
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>   return rv;
> }
> 
> // static
> nsresult
>-BodyDeleteDir(nsIFile* aBaseDir)
>+BodyDeleteDir(const QuotaInfo& aQuotaInfo, nsIFile* aBaseDir)
> {
>   MOZ_DIAGNOSTIC_ASSERT(aBaseDir);
> 
>   nsCOMPtr<nsIFile> aBodyDir;
>   nsresult rv = aBaseDir->Clone(getter_AddRefs(aBodyDir));
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>   rv = aBodyDir->Append(NS_LITERAL_STRING("morgue"));
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>-  rv = aBodyDir->Remove(/* recursive = */ true);
>-  if (rv == NS_ERROR_FILE_NOT_FOUND ||
>-      rv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) {
>-    rv = NS_OK;
>-  }
>+  rv = RemoveNsIFileRecursively(aQuotaInfo, aBodyDir);
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>   return rv;
> }
> 
> // static
> nsresult
> BodyGetCacheDir(nsIFile* aBaseDir, const nsID& aId, nsIFile** aCacheDirOut)
>@@ -206,16 +204,19 @@ BodyFinalizeWrite(nsIFile* aBaseDir, con
>   nsCOMPtr<nsIFile> finalFile;
>   rv = BodyIdToFile(aBaseDir, aId, BODY_FILE_FINAL, getter_AddRefs(finalFile));
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>   nsAutoString finalFileName;
>   rv = finalFile->GetLeafName(finalFileName);
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>+  // It's fine to not notify the QuotaManager that the path has been changed,
>+  // because its path will be updated and its size will be recalculated when
>+  // opening file next time.
>   rv = tmpFile->RenameTo(nullptr, finalFileName);
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>   return rv;
> }
> 
> // static
> nsresult
>@@ -242,48 +243,39 @@ BodyOpen(const QuotaInfo& aQuotaInfo, ns
> 
>   fileStream.forget(aStreamOut);
> 
>   return rv;
> }
> 
> // static
> nsresult
>-BodyDeleteFiles(nsIFile* aBaseDir, const nsTArray<nsID>& aIdList)
>+BodyDeleteFiles(const QuotaInfo& aQuotaInfo, nsIFile* aBaseDir,
>+                const nsTArray<nsID>& aIdList)
> {
>   nsresult rv = NS_OK;
> 
>   for (uint32_t i = 0; i < aIdList.Length(); ++i) {
>     nsCOMPtr<nsIFile> tmpFile;
>     rv = BodyIdToFile(aBaseDir, aIdList[i], BODY_FILE_TMP,
>                       getter_AddRefs(tmpFile));
>     if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>-    rv = tmpFile->Remove(false /* recursive */);
>-    if (rv == NS_ERROR_FILE_NOT_FOUND ||
>-        rv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) {
>-      rv = NS_OK;
>-    }
>-
>+    rv = RemoveNsIFile(aQuotaInfo, tmpFile);
>     // Only treat file deletion as a hard failure in DEBUG builds.  Users
>     // can unfortunately hit this on windows if anti-virus is scanning files,
>     // etc.
>     MOZ_ASSERT(NS_SUCCEEDED(rv));
> 
>     nsCOMPtr<nsIFile> finalFile;
>     rv = BodyIdToFile(aBaseDir, aIdList[i], BODY_FILE_FINAL,
>                       getter_AddRefs(finalFile));
>     if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>-    rv = finalFile->Remove(false /* recursive */);
>-    if (rv == NS_ERROR_FILE_NOT_FOUND ||
>-        rv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) {
>-      rv = NS_OK;
>-    }
>-
>+    rv = RemoveNsIFile(aQuotaInfo, finalFile);
>     // Again, only treat removal as hard failure in debug build.
>     MOZ_ASSERT(NS_SUCCEEDED(rv));
>   }
> 
>   return NS_OK;
> }
> 
> namespace {
>@@ -316,17 +308,18 @@ BodyIdToFile(nsIFile* aBaseDir, const ns
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>   return rv;
> }
> 
> } // namespace
> 
> nsresult
>-BodyDeleteOrphanedFiles(nsIFile* aBaseDir, nsTArray<nsID>& aKnownBodyIdList)
>+BodyDeleteOrphanedFiles(const QuotaInfo& aQuotaInfo, nsIFile* aBaseDir,
>+                        nsTArray<nsID>& aKnownBodyIdList)
> {
>   MOZ_DIAGNOSTIC_ASSERT(aBaseDir);
> 
>   // body files are stored in a directory structure like:
>   //
>   //  /morgue/01/{01fdddb2-884d-4c3d-95ba-0c8062f6c325}.final
>   //  /morgue/02/{02fdddb2-884d-4c3d-95ba-0c8062f6c325}.tmp
> 
>@@ -352,17 +345,17 @@ BodyDeleteOrphanedFiles(nsIFile* aBaseDi
>     nsCOMPtr<nsIFile> subdir = do_QueryInterface(entry);
> 
>     bool isDir = false;
>     rv = subdir->IsDirectory(&isDir);
>     if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>     // If a file got in here somehow, try to remove it and move on
>     if (NS_WARN_IF(!isDir)) {
>-      rv = subdir->Remove(false /* recursive */);
>+      rv = RemoveNsIFile(aQuotaInfo, subdir);
>       MOZ_ASSERT(NS_SUCCEEDED(rv));
>       continue;
>     }
> 
>     nsCOMPtr<nsISimpleEnumerator> subEntries;
>     rv = subdir->GetDirectoryEntries(getter_AddRefs(subEntries));
>     if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>@@ -379,17 +372,17 @@ BodyDeleteOrphanedFiles(nsIFile* aBaseDi
>       nsAutoCString leafName;
>       rv = file->GetNativeLeafName(leafName);
>       if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>       // Delete all tmp files regardless of known bodies.  These are
>       // all considered orphans.
>       if (StringEndsWith(leafName, NS_LITERAL_CSTRING(".tmp"))) {
>         // remove recursively in case its somehow a directory
>-        rv = file->Remove(true /* recursive */);
>+        rv = RemoveNsIFileRecursively(aQuotaInfo, file);
>         MOZ_ASSERT(NS_SUCCEEDED(rv));
>         continue;
>       }
> 
>       nsCString suffix(NS_LITERAL_CSTRING(".final"));
> 
>       // Otherwise, it must be a .final file.  If its not, then just
>       // skip it.
>@@ -402,17 +395,17 @@ BodyDeleteOrphanedFiles(nsIFile* aBaseDi
>       // the ignore the file.
>       nsID id;
>       if (NS_WARN_IF(!id.Parse(leafName.BeginReading()))) {
>         continue;
>       }
> 
>       if (!aKnownBodyIdList.Contains(id)) {
>         // remove recursively in case its somehow a directory
>-        rv = file->Remove(true /* recursive */);
>+        rv = RemoveNsIFileRecursively(aQuotaInfo, file);
>         MOZ_ASSERT(NS_SUCCEEDED(rv));
>       }
>     }
>   }
> 
>   return rv;
> }
> 
>@@ -463,21 +456,18 @@ CreateMarkerFile(const QuotaInfo& aQuota
> 
> nsresult
> DeleteMarkerFile(const QuotaInfo& aQuotaInfo)
> {
>   nsCOMPtr<nsIFile> marker;
>   nsresult rv = GetMarkerFileHandle(aQuotaInfo, getter_AddRefs(marker));
>   if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>-  rv = marker->Remove(/* recursive = */ false);
>-  if (rv == NS_ERROR_FILE_NOT_FOUND ||
>-      rv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) {
>-    rv = NS_OK;
>-  }
>+  rv = RemoveNsIFile(aQuotaInfo, marker);
>+  MOZ_ASSERT(NS_SUCCEEDED(rv));
> 
>   // Again, no fsync is necessary.  If the OS crashes before the file
>   // removal is flushed, then the Cache will search for stale data on
>   // startup.  This will cause the next Cache access to be a bit slow, but
>   // it seems appropriate after an OS crash.
> 
>   return NS_OK;
> }
>@@ -491,11 +481,83 @@ MarkerFileExists(const QuotaInfo& aQuota
> 
>   bool exists = false;
>   rv = marker->Exists(&exists);
>   if (NS_WARN_IF(NS_FAILED(rv))) { return false; }
> 
>   return exists;
> }
> 
>+// static
>+nsresult
>+RemoveNsIFileRecursively(const QuotaInfo& aQuotaInfo, nsIFile* aFile)
>+{
>+  MOZ_DIAGNOSTIC_ASSERT(aFile);
>+
>+  bool isDirectory = false;
>+  nsresult rv = aFile->IsDirectory(&isDirectory);
>+  if (rv == NS_ERROR_FILE_NOT_FOUND ||
>+      rv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) {
>+    return NS_OK;
>+  }
>+  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>+
>+  if (!isDirectory) {
>+    return RemoveNsIFile(aQuotaInfo, aFile);
>+  }
>+
>+  // Unfortunately, we need to traverse all the entries and delete files one by
>+  // one to update their usages to the QuotaManager.
>+  nsCOMPtr<nsISimpleEnumerator> entries;
>+  rv = aFile->GetDirectoryEntries(getter_AddRefs(entries));
>+  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>+
>+  bool hasMore = false;
>+  while (NS_SUCCEEDED((rv = entries->HasMoreElements(&hasMore))) && hasMore) {
>+    nsCOMPtr<nsISupports> entry;
>+    rv = entries->GetNext(getter_AddRefs(entry));
>+    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>+
>+    nsCOMPtr<nsIFile> file = do_QueryInterface(entry);
>+    MOZ_ASSERT(file);
>+
>+    rv = RemoveNsIFileRecursively(aQuotaInfo, file);
>+    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>+  }
>+  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>+
>+  // In the end, remove the folder
>+  rv = aFile->Remove(/* recursive */ false);
>+  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>+
>+  return rv;
>+}
>+
>+// static
>+nsresult
>+RemoveNsIFile(const QuotaInfo& aQuotaInfo, nsIFile* aFile)
>+{
>+  MOZ_DIAGNOSTIC_ASSERT(aFile);
>+
>+  int64_t fileSize = 0;
>+  nsresult rv = aFile->GetFileSize(&fileSize);
>+  if (rv == NS_ERROR_FILE_NOT_FOUND ||
>+      rv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) {
>+    return NS_OK;
>+  }
>+  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>+
>+  rv = aFile->Remove( /* recursive */ false);
>+  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>+
>+  QuotaManager* quotaManager = QuotaManager::Get();
>+  MOZ_DIAGNOSTIC_ASSERT(quotaManager);
>+
>+  quotaManager->DecreaseUsageForOrigin(PERSISTENCE_TYPE_DEFAULT,
>+                                       aQuotaInfo.mGroup, aQuotaInfo.mOrigin,
>+                                       fileSize);
>+
>+  return rv;
>+}
>+
> } // namespace cache
> } // namespace dom
> } // namespace mozilla
>diff --git a/dom/cache/FileUtils.h b/dom/cache/FileUtils.h
>--- a/dom/cache/FileUtils.h
>+++ b/dom/cache/FileUtils.h
>@@ -21,17 +21,17 @@ namespace cache {
> 
> nsresult
> BodyCreateDir(nsIFile* aBaseDir);
> 
> // Note that this function can only be used during the initialization of the
> // database.  We're unlikely to be able to delete the DB successfully past
> // that point due to the file being in use.
> nsresult
>-BodyDeleteDir(nsIFile* aBaseDir);
>+BodyDeleteDir(const QuotaInfo& aQuotaInfo, nsIFile* aBaseDir);
> 
> nsresult
> BodyGetCacheDir(nsIFile* aBaseDir, const nsID& aId, nsIFile** aCacheDirOut);
> 
> nsresult
> BodyStartWriteStream(const QuotaInfo& aQuotaInfo, nsIFile* aBaseDir,
>                      nsIInputStream* aSource, void* aClosure,
>                      nsAsyncCopyCallbackFun aCallback, nsID* aIdOut,
>@@ -43,27 +43,35 @@ BodyCancelWrite(nsIFile* aBaseDir, nsISu
> nsresult
> BodyFinalizeWrite(nsIFile* aBaseDir, const nsID& aId);
> 
> nsresult
> BodyOpen(const QuotaInfo& aQuotaInfo, nsIFile* aBaseDir, const nsID& aId,
>          nsIInputStream** aStreamOut);
> 
> nsresult
>-BodyDeleteFiles(nsIFile* aBaseDir, const nsTArray<nsID>& aIdList);
>+BodyDeleteFiles(const QuotaInfo& aQuotaInfo, nsIFile* aBaseDir,
>+                const nsTArray<nsID>& aIdList);
> 
> nsresult
>-BodyDeleteOrphanedFiles(nsIFile* aBaseDir, nsTArray<nsID>& aKnownBodyIdList);
>+BodyDeleteOrphanedFiles(const QuotaInfo& aQuotaInfo, nsIFile* aBaseDir,
>+                        nsTArray<nsID>& aKnownBodyIdList);
> 
> nsresult
> CreateMarkerFile(const QuotaInfo& aQuotaInfo);
> 
> nsresult
> DeleteMarkerFile(const QuotaInfo& aQuotaInfo);
> 
> bool
> MarkerFileExists(const QuotaInfo& aQuotaInfo);
> 
>+nsresult
>+RemoveNsIFileRecursively(const QuotaInfo& aQuotaInfo, nsIFile* aFile);
>+
>+nsresult
>+RemoveNsIFile(const QuotaInfo& aQuotaInfo, nsIFile* aFile);
>+
> } // namespace cache
> } // namespace dom
> } // namespace mozilla
> 
> #endif // mozilla_dom_cache_FileUtils_h
>diff --git a/dom/cache/Manager.cpp b/dom/cache/Manager.cpp
>--- a/dom/cache/Manager.cpp
>+++ b/dom/cache/Manager.cpp
>@@ -77,25 +77,25 @@ public:
>       nsresult rv = db::FindOrphanedCacheIds(aConn, orphanedCacheIdList);
>       if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>       for (uint32_t i = 0; i < orphanedCacheIdList.Length(); ++i) {
>         AutoTArray<nsID, 16> deletedBodyIdList;
>         rv = db::DeleteCacheId(aConn, orphanedCacheIdList[i], deletedBodyIdList);
>         if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
>-        rv = BodyDeleteFiles(aDBDir, deletedBodyIdList);
>+        rv = BodyDeleteFiles(aQuotaInfo, aDBDir, deletedBodyIdList);
>         if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>       }
> 
>       // Clean up orphaned body objects
>       AutoTArray<nsID, 64> knownBodyIdList;
>       rv = db::GetKnownBodyIds(aConn, knownBodyIdList);
> 
>-      rv = BodyDeleteOrphanedFiles(aDBDir, knownBodyIdList);
>+      rv = BodyDeleteOrphanedFiles(aQuotaInfo, aDBDir, knownBodyIdList);
>       if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>     }
> 
>     return rv;
>   }
> };
> 
> // ----------------------------------------------------------------------------
>@@ -131,17 +131,17 @@ public:
>     }
> 
>     rv = dbDir->Append(NS_LITERAL_STRING("cache"));
>     if (NS_WARN_IF(NS_FAILED(rv))) {
>       aResolver->Resolve(rv);
>       return;
>     }
> 
>-    rv = BodyDeleteFiles(dbDir, mDeletedBodyIdList);
>+    rv = BodyDeleteFiles(aQuotaInfo, dbDir, mDeletedBodyIdList);
>     Unused << NS_WARN_IF(NS_FAILED(rv));
> 
>     aResolver->Resolve(rv);
>   }
> 
> private:
>   nsTArray<nsID> mDeletedBodyIdList;
> };
>@@ -656,16 +656,17 @@ private:
>     // We should be pre-initialized to expect one async completion.  This is
>     // the "manual" completion we call at the end of this method in all
>     // cases.
>     MOZ_DIAGNOSTIC_ASSERT(mExpectedAsyncCopyCompletions == 1);
> 
>     mResolver = aResolver;
>     mDBDir = aDBDir;
>     mConn = aConn;
>+    mQuotaInfo.emplace(aQuotaInfo);
> 
>     // File bodies are streamed to disk via asynchronous copying.  Start
>     // this copying now.  Each copy will eventually result in a call
>     // to OnAsyncCopyComplete().
>     nsresult rv = NS_OK;
>     for (uint32_t i = 0; i < mList.Length(); ++i) {
>       rv = StartStreamCopy(aQuotaInfo, mList[i], RequestStream,
>                            &mExpectedAsyncCopyCompletions);
>@@ -927,17 +928,17 @@ private:
>     {
>       MutexAutoLock lock(mMutex);
>       MOZ_ASSERT(mCopyContextList.IsEmpty());
>     }
> #endif
> 
>     // Clean up any files we might have written before hitting the error.
>     if (NS_FAILED(aRv)) {
>-      BodyDeleteFiles(mDBDir, mBodyIdWrittenList);
>+      BodyDeleteFiles(mQuotaInfo.ref(), mDBDir, mBodyIdWrittenList);
>     }
> 
>     // Must be released on the target thread where it was opened.
>     mConn = nullptr;
> 
>     // Drop our ref to the target thread as we are done with this thread.
>     // Also makes our thread assertions catch any incorrect method calls
>     // after resolve.
>@@ -969,16 +970,18 @@ private:
> 
>   // Written to on target thread, accessed on initiating thread after target
>   // thread activity is guaranteed complete
>   nsTArray<nsID> mDeletedBodyIdList;
> 
>   // accessed from any thread while mMutex locked
>   Mutex mMutex;
>   nsTArray<nsCOMPtr<nsISupports>> mCopyContextList;
>+
>+  Maybe<QuotaInfo> mQuotaInfo;
> };
> 
> // ----------------------------------------------------------------------------
> 
> class Manager::CacheDeleteAction final : public Manager::BaseAction
> {
> public:
>   CacheDeleteAction(Manager* aManager, ListenerId aListenerId,
Attachment #8875953 - Attachment description: Bug 1367309 - P1: Update usage to QuotaManager when a file is deleted in DOM Cache. r=bkelly r=janv → [Final]Bug 1367309 - P1: Update usage to QuotaManager when a file is deleted in DOM Cache. r=bkelly r=janv
I don't know if I touch anything, so I update the patch from my local repo.
Attachment #8875953 - Attachment is obsolete: true
Attachment #8881794 - Flags: review+
The previous one is wrong...
Attachment #8881794 - Attachment is obsolete: true
Attachment #8881795 - Flags: review+
Mark as checkin-needed, since the try looks good and there is no any failure happens while running rr chaos mode 30 times.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/760472ae7ec8
Part 1: Update usage to QuotaManager when a file is deleted in DOM Cache. r=bkelly, r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dfe04d978fa
Part 2: Test for ensuring DOM Cache update usage to QuotaManager. r=bkelly, r=janv
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/760472ae7ec8
https://hg.mozilla.org/mozilla-central/rev/0dfe04d978fa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.