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

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Updated

3 years ago
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Summary: Bypass mFile->Remove() on invalid/doomed after shutdown of HTTP cache → Completely bypass mFile->Remove() on invalid/doomed after shutdown of HTTP cache
Whiteboard: [necko-active]
(Assignee)

Comment 2

3 years ago
Created attachment 8758239 [details] [diff] [review]
[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
(Assignee)

Comment 3

3 years ago
Comment on attachment 8758239 [details] [diff] [review]
[wrong patch]

(not sure why I didn't ask for this immediately)
Attachment #8758239 - Flags: review?(michal.novotny)
Comment on attachment 8758239 [details] [diff] [review]
[wrong patch]

wrong patch?
Flags: needinfo?(honzab.moz)
Attachment #8758239 - Flags: review?(michal.novotny)
(Assignee)

Comment 5

3 years ago
Created attachment 8758777 [details] [diff] [review]
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..
Attachment #8758239 - Attachment is obsolete: true
Flags: needinfo?(honzab.moz)
Attachment #8758777 - Flags: review?(michal.novotny)
(Assignee)

Updated

3 years ago
Attachment #8758239 - Attachment description: v1 (not removing any file after shutdown) → [wrong patch]
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.
Attachment #8758777 - Flags: review?(michal.novotny) → review-
(Assignee)

Comment 7

3 years ago
(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.
Flags: needinfo?(michal.novotny)
(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.
Flags: needinfo?(michal.novotny)
(Assignee)

Comment 9

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

Comment 10

3 years ago
Created attachment 8759253 [details] [diff] [review]
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
Attachment #8758777 - Attachment is obsolete: true
Attachment #8759253 - Flags: review?(michal.novotny)
Attachment #8759253 - Flags: review?(michal.novotny) → review+
(Assignee)

Updated

3 years ago
Depends on: 1268569
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5c25393a80c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 13

3 years ago
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
Attachment #8759253 - Flags: approval-mozilla-beta?
status-firefox48: --- → affected
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
Attachment #8759253 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 15

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/545c87f8e7a0
status-firefox48: affected → fixed
You need to log in before you can comment on or make changes to this bug.