1268569 - Don't remove HTTP cache doomed files after shutdown

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

According some OSX crashes, we apparently are trying to remove files from the cache2/doomed directory after shutdown.  That is not necessary [Michal?], those can be left to be deleted on another browser run.

Bug 913822 and bug 1239687 fixes are still inefficient in this.

We need to set also mIsDoomed = false at https://hg.mozilla.org/mozilla-central/annotate/86730d0a82093d705e44f33a34973d28b269f1ea/netwerk/cache2/CacheFileIOManager.cpp#l2315

Comment 1

[Honza Bambas (:mayhemer)]

For ref, https://crash-stats.mozilla.com/report/index/acffb9c1-471c-452f-b858-af83e2160423#allthreads has both bugs mentioned in comment 0.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=012e52fd63d0

Comment 3

[Michal Novotny [:michal]]

Comment on attachment 8746691 [details] [diff] [review]
v1

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

This IMO isn't OK. mInvalid flag is used only to decide whether to remove file and entry from index when the handle is closed, so mangling this value is sort of OK. But changing mIsDoomed affects a lot of fundamental decisions in the cache code, e.g. the handle might be returned via CacheFileHandles::GetHandle(), entry in the index which either doesn't exist or there is already a new entry for a newer handle could be accessed, etc...

Either we could create a new flag (e.g. mLeakIt), set it here to true and check it at places where we are removing the file, or we must do the IsPastShutdownIOLag check at those places to decide whether to remove the file or not.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v2

- mLeakIt flag added to CacheFileHandle
- I also reorganized some of the bool flags to be one of const field, atomic, or single-thread-access bool field to lower the class footprint (same should be done for CacheFile once)

locally tested.

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

Comment 5

[Michal Novotny [:michal]]

Comment on attachment 8753961 [details] [diff] [review]
v2

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

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +183,5 @@
>    } else {
>      LOG(("CacheFileHandle::Log() - entry file [this=%p, hash=%08x%08x%08x%08x"
>           "%08x, isDoomed=%d, priority=%d, closed=%d, invalid=%d, fileExists=%d,"
>           " fileSize=%lld, leafName=%s, key=%s]", this, LOGSHA1(mHash),
> +         int(mIsDoomed), bool(mPriority), int(mClosed), bool(mInvalid), bool(mFileExists), mFileSize,

Please add logging of mLeakIt to CacheFileHandle::Log(). Maybe add pinning information too.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v2.1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6921df128a72

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v2.1 [actually updated to r]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=495e4ffca5fb

Comment 8

[Honza Bambas (:mayhemer)]

This one is right:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1c9de554922

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce4920b0faf

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ce4920b0faf

Comment 12

[Honza Bambas (:mayhemer)]

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

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> Definitely not fixed:
> https://crash-stats.mozilla.com/report/index/614812a7-787d-4bf8-b25c-
> 700f62160528#allthreads

Filed bug 1276869 on that.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8754874 [details] [diff] [review]
v2.1 [actually updated to r]

Approval Request Comment
[Feature/regressing bug #]: http cache v2
[User impact if declined]: shutdown hang/crash
[Describe test coverage new/current, TreeHerder]: a week on m-c, heavily exercised code
[Risks and why]: low to my knowledge, this only leaves file descriptors open (leaks) for certain files
[String/UUID change made/needed]: none

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8754874 [details] [diff] [review]
v2.1 [actually updated to r]

Fix a shutdown hang, taking it

Comment 16

[Carsten Book [:Tomcat]]

has problems to apply to aurora:

merging netwerk/cache2/CacheFileIOManager.h
warning: conflicts while merging netwerk/cache2/CacheFileIOManager.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Carsten Book [:Tomcat] from comment #16)
> has problems to apply to aurora:
> 
> merging netwerk/cache2/CacheFileIOManager.h
> warning: conflicts while merging netwerk/cache2/CacheFileIOManager.cpp!
> (edit, then use 'hg resolve --mark')
> abort: unresolved conflicts, can't continue
> (use 'hg resolve' and 'hg graft --continue')

a+ and land bug 1267980 first (if rejected then use the m-c landed patch for m-a).  I was trying to make that clear, it's also in the list of dependencies.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 8754874 [details] [diff] [review]
v2.1 [actually updated to r]

(In reply to Honza Bambas (:mayhemer) from comment #17)
> (In reply to Carsten Book [:Tomcat] from comment #16)
> > has problems to apply to aurora:
> > 
> > merging netwerk/cache2/CacheFileIOManager.h
> > warning: conflicts while merging netwerk/cache2/CacheFileIOManager.cpp!
> > (edit, then use 'hg resolve --mark')
> > abort: unresolved conflicts, can't continue
> > (use 'hg resolve' and 'hg graft --continue')
> 
> a+ and land bug 1267980 first (if rejected then use the m-c landed patch for
> m-a).  I was trying to make that clear, it's also in the list of
> dependencies.

The bug blocking this one has already landed on m-b, please try to reland the patch here now.  Thanks.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0e5e3a801d84

Comment 20

[Calixte Denizet (:calixte)]

Crash volume for signature 'shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_WaitCondVar | mozilla::CondVar::Wait | mozilla::net::ShutdownEvent::PostAndWait':
  - nightly (50): 6
  - release (47): 45118

Affected platform: Windows

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Calixte Denizet (:calixte) from comment #20)
> Crash volume for signature 'shutdownhang | WaitForSingleObjectEx |
> WaitForSingleObject | PR_WaitCondVar | mozilla::CondVar::Wait |
> mozilla::net::ShutdownEvent::PostAndWait':
>   - nightly (50): 6

There are few groups of Nightly crashes for this signature with buildid > 2016-06-23 (on which last fix around cache2 shutdown crashes landed):

- CacheFileIOManager::DoomFileInternal
  https://crash-stats.mozilla.com/report/index/df5c746f-aba0-4096-8791-356702160711#allthreads
- CacheFileIOManager::ReadInternal
  https://crash-stats.mozilla.com/report/index/bc245e5c-dd2e-46ee-9649-ed1d12160704#allthreads
- CacheFileIOManager::MaybeReleaseNSPRHandleInternal
  https://crash-stats.mozilla.com/report/index/c0c7a8de-91bf-4d70-8cfa-7632c2160710#allthreads
- mozilla::net::CacheFileContextEvictor::EvictEntries
  https://crash-stats.mozilla.com/report/index/fdab432f-6de2-4a1e-8b5c-ecff52160710#allthreads

and one interesting, but probably just coincidence (no other tread keeping the lock found):
- mozilla::net::CacheFileAutoLock::CacheFileAutoLock
  https://crash-stats.mozilla.com/report/index/0215c729-5fba-409c-9769-fb0502160630#allthreads

The first set, according the code, are general IO hangs, not related to shutdown.  We are hanging there a lot longer than before shutdown has even been initiated.

There seems to be some ways to cancel a synchronous IO on Windows (since Vista).  I'll file a new bug for that.