1276869 - Completely bypass mFile->Remove() on invalid/doomed after shutdown of HTTP cache

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

Bug 1268569 is still not enough.  We are still removing files we don't have filedescs open for.  For those there is no way we can set the mLeakIt flag - it's only set in ReleaseNSPRHandleInternal which is called only when the file's handle is still open.

Hence, instead of the mLeakIt flag I'd rather check directly for CacheObserver::IsShuttingDown().

Comment 1

[Honza Bambas (:mayhemer)]

Should fix:

https://crash-stats.mozilla.com/report/index/614812a7-787d-4bf8-b25c-700f62160528#allthreads

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: [wrong patch]

- mLeakIt flag is gone
- ReleaseNSPRHandleInternal now checks !aHandle internally, returns NS_OK if so
- ReleaseNSPRHandleInternal will fail when either of:
  - the file is considered OK to leak (the same condition as before) ; doesn't close the file then and any further operation on the handle's file should be bypassed (remove/move)
  - the file desc could not be closed from some reason (however improbable that might be)
- returns OK otherwise
- CacheFileIOManager::ShutdownInternal no longer removes ANY file, just closes those that were fully written (those are rarely hit here, tho)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f114037255d

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8758239 [details] [diff] [review]
[wrong patch]

(not sure why I didn't ask for this immediately)

Comment 4

[Michal Novotny [:michal]]

Comment on attachment 8758239 [details] [diff] [review]
[wrong patch]

wrong patch?

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1 (not removing any file after shutdown)

yes.  how could that happen!

please see comment 2 for comments on the patch.

try likes this patch..

Comment 6

[Michal Novotny [:michal]]

Comment on attachment 8758777 [details] [diff] [review]
v1 (not removing any file after shutdown)

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

Do I understand correctly that the only effective change is the removal of h->mFile->Remove(false) from ShutdownInternal and all other changes are just a code cleanup that's not in fact necessary?

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +1223,4 @@
>  
>      if (!h->IsSpecialFile() && !h->mIsDoomed &&
>          (h->mInvalid || !h->mFileExists)) {
>        CacheIndex::RemoveEntry(h->Hash());

You have to keep the entry in the index if you're not deleting the file, otherwise it is an inconsistency that would trigger index update once we detect it.

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #6)
> Comment on attachment 8758777 [details] [diff] [review]
> v1 (not removing any file after shutdown)
> 
> Review of attachment 8758777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do I understand correctly that the only effective change is the removal of
> h->mFile->Remove(false) from ShutdownInternal and all other changes are just
> a code cleanup that's not in fact necessary?

The major change is the omission of removing files during the internal shutdown, yes.  I think the logic is simpler and clearer now with the changes to ReleaseNSPRHandleInternal.  And actually makes it possible to perform the decision for those file that don't have fd currently open (which is tho buggy in this patch! - see below).  Otherwise the code would be more complicated. 

When I'm seeing the patch now, it seems like there is still a glitch: CacheFileIOManager::CloseHandleInternal will still delete the invalid/doomed files not having fd when we are past the shutdown!  I will update the patch, it will more show the idea I had with the ReleaseNSPRHandleInternal changes.

Should we even rename it to MaybeReleaseNSPRHandleInternal to make it more clear?

> 
> ::: netwerk/cache2/CacheFileIOManager.cpp
> @@ +1223,4 @@
> >  
> >      if (!h->IsSpecialFile() && !h->mIsDoomed &&
> >          (h->mInvalid || !h->mFileExists)) {
> >        CacheIndex::RemoveEntry(h->Hash());
> 
> You have to keep the entry in the index if you're not deleting the file,
> otherwise it is an inconsistency that would trigger index update once we
> detect it.

Aha, I was thinking of it, yes, makes sense.

Comment 8

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> When I'm seeing the patch now, it seems like there is still a glitch:
> CacheFileIOManager::CloseHandleInternal will still delete the invalid/doomed
> files not having fd when we are past the shutdown!  I will update the patch,
> it will more show the idea I had with the ReleaseNSPRHandleInternal changes.

What do you mean with "past shutdown"? Do you mean after ShutdownInternal is called or do you mean during the shutdown instead?
 
> Should we even rename it to MaybeReleaseNSPRHandleInternal to make it more
> clear?

Yes, it would probably better describe what the method does.

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #8)
> (In reply to Honza Bambas (:mayhemer) from comment #7)
> > When I'm seeing the patch now, it seems like there is still a glitch:
> > CacheFileIOManager::CloseHandleInternal will still delete the invalid/doomed
> > files not having fd when we are past the shutdown!  I will update the patch,
> > it will more show the idea I had with the ReleaseNSPRHandleInternal changes.
> 
> What do you mean with "past shutdown"? Do you mean after ShutdownInternal is
> called or do you mean during the shutdown instead?

Generally after shutdown start point, sorry for confusion.

>  
> > Should we even rename it to MaybeReleaseNSPRHandleInternal to make it more
> > clear?
> 
> Yes, it would probably better describe what the method does.

OK.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1.1 (not removing any file after shutdown)

- rename
- and now (finally!) checking the shutdown/shutdown timeout also for handles w/o fd
- and changed the index remove condition ; I just leave invalid files that are always left on disk (when already having a file), otherwise the condition is unchanged

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54d541493767

Comment 11

[Honza Bambas (:mayhemer)]

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

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8759253 [details] [diff] [review]
v1.1 (not removing any file after shutdown)

Approval Request Comment
[Feature/regressing bug #]: http cache v2
[User impact if declined]: shutdown crash/hang
[Describe test coverage new/current, TreeHerder]: on 49 (now m-a) since 2016-06-06, heavily exercised code
[Risks and why]: minimal, the patch isn't small but the change itself actually is
[String/UUID change made/needed]: none

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8759253 [details] [diff] [review]
v1.1 (not removing any file after shutdown)

Fix a shutdown issue, no obvious regression from the backing in aurora & nightly. Taking it.
Should be in 48 beta 3

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/545c87f8e7a0