Closed Bug 1276930 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Networking: Cache, defect, critical)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed
firefox51 --- affected

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Keywords: crash, Whiteboard: [necko-active])

Crash Data

Attachments

(1 file)

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.
Assignee: nobody → honzab.moz
Whiteboard: [necko-active]
Duplicate of this bug: 1271711
- 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
Attachment #8761171 - Flags: review?(michal.novotny)
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
Attachment #8761171 - Flags: review?(michal.novotny) → review+
(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.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff4669c8aaf
Bypass HTTP cache data read right after shutdown, r=michal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ff4669c8aaf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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)
You need to log in before you can comment on or make changes to this bug.