1276930 - Crash in shutdownhang | CacheFileIOManager::ReadInternal - bypass reads after shutdown

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

This bug was filed from the Socorro interface and is 
report bp-96e5b1d7-9bfd-4339-a6fd-c1e3b2160523.
=============================================================

Reads should ultimately fail immediately after shutdown, there are no consumers anyway.

We only have to think about when this failed read leads through some code path to dooming the cache entry, which is something we respect regardless shutdown.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1 (no Read/ReadInternal immediately after shutdown)

- both read and readinternal are bypassed completely with a failure when after the shutdown point
- changing sShutdownDemandedTime to fully sequenced atomic just to make sure the flag when set is really visible to the IO thread ASAP and the crashes are not a threading issue on this flag ; it may be a perf hit, but after the shutdown crashes are cleared, we can change it back to Relaxed

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

Comment 3

[Michal Novotny [:michal]]

Comment on attachment 8761171 [details] [diff] [review]
v1 (no Read/ReadInternal immediately after shutdown)

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

r+ but I don't think that changing the atomic to SequentiallyConsistent is really needed

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #3)
> Comment on attachment 8761171 [details] [diff] [review]
> v1 (no Read/ReadInternal immediately after shutdown)
> 
> Review of attachment 8761171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ but I don't think that changing the atomic to SequentiallyConsistent is
> really needed

I only wanted to make sure (eliminate doubts) that the IO thread sees the flag set correctly.  We can revert when all the crashes are gone.  It's true this could have some influence on performance, but here the stability is more important at the moment.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff4669c8aaf
Bypass HTTP cache data read right after shutdown, r=michal

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff4669c8aaf

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ff4669c8aaf

Comment 8

[Andrei Vaida [:avaida]]

This is still reproducible on Fx50 in low volume, based on crash data from the last ~4 months.

  SIGNATURE   | shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | 
  		PR_WaitCondVar | mozilla::CondVar::Wait | 
                mozilla::net::ShutdownEvent::PostAndWait
  --------------------------------------------------------------------------
  CRASH STATS | http://tinyurl.com/heux9vm
  --------------------------------------------------------------------------
  OVERVIEW    | 0 crashes on nightly 52
	      | 1 crash on nightly 51
	      | 0 crashes on aurora 51
	      | 5 crashes on nightly 50
	      | 8 crashes on aurora 50
	      | 0 crashes on beta 50
  --------------------------------------------------------------------------
  LAST CRASH  | 2016-08-16 (on 50.0a2)