The current code doesn't work correctly if the thread is woken up spuriously.

Very good catch. So it's platform specific to OSX? Is this a known behavioral deviance of this system? Because this never happened to me on Windows. And it's something pretty unexpected, actually, and almost sounds like an OSX bug. Other question is, how is it possible it started to appear after the recent changes that are unrelated to this at all? A coincidence? A change in timing that influences this? Should we open a product-wide bug to audit the use of condvars/monitors? I think it would be good thing to do.

Comment on attachment 8731803 [details] [diff] [review] fix Review of attachment 8731803 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/cache2/CacheFileIOManager.cpp @@ +532,5 @@ > // Events > > class ShutdownEvent : public nsRunnable { > public: > + ShutdownEvent(mozilla::Mutex *aLock, mozilla::CondVar *aCondVar, bool *aNotified) instead of carrying all these (originally stack vars) by reference I would personally encapsulate them inside the event and just expose |void ShutdownEvent::Wait()| method that will do all the stuff itself. Also, could you migrate to Monitor when you are here? All these changes would make the code much more simple and clear.

ShutdownEvent uses monitor now.

Comment on attachment 8731836 [details] [diff] [review] patch v2 Review of attachment 8731836 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Please also uplift. Probably up to beta?

(In reply to Honza Bambas (:mayhemer) from comment #1) > So it's platform specific to OSX? Is this a known behavioral deviance of > this system? Because this never happened to me on Windows. And it's > something pretty unexpected, actually, and almost sounds like an OSX bug. It's not OSX specific but probably doesn't happen too often because these crashes started to appear recently and only on OSX 10.10. > Other question is, how is it possible it started to appear after the recent > changes that are unrelated to this at all? A coincidence? A change in > timing that influences this? I have no idea. > Should we open a product-wide bug to audit the use of condvars/monitors? I > think it would be good thing to do. Maybe, e.g. this one seems to be wrong: http://hg.mozilla.org/mozilla-central/annotate/3e04659fdf6a/media/mtransport/nr_socket_prsock.cpp#l1307

To save the information source found by Michal here: http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_cond_timedwait.html https://en.wikipedia.org/wiki/Spurious_wakeup

Michal, please let this be uplifted. Thanks.

Comment on attachment 8731836 [details] [diff] [review] patch v2 Approval Request Comment [Feature/regressing bug #]: it's in cache2 from the beginning [User impact if declined]: possible crash during shutdown and memory reporting [Describe test coverage new/current, TreeHerder]: any existing test that use disk cache [Risks and why]: low, it's a simple straightforward fix [String/UUID change made/needed]: none

What are the crash signatures we should look for here (that may exist now and will hopefully go away?)

Marking 46+ as affected.

Comment on attachment 8731836 [details] [diff] [review] patch v2 Fix for possible shutdown crashes for Mac. This should make it into beta 5 later this week.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14) > What are the crash signatures we should look for here (that may exist now > and will hopefully go away?) See bugs 1251130, 1252746, 1254720, 1257379 and 1255948.

I am unable to find any new reports for the signatures from the bugs mentioned in comment 17. Based on this output, marking here accordingly.