Closed Bug 1257611 Opened 8 years ago Closed 8 years ago

Fix wrong CondVar::Wait() and Monitor::Wait() usage in netwerk/cache2

Categories

(Core :: Networking: Cache, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox46 --- verified
firefox47 --- verified
firefox48 --- verified

People

(Reporter: michal, Assigned: michal)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

Attached patch fix (obsolete) — Splinter Review
The current code doesn't work correctly if the thread is woken up spuriously.
Attachment #8731803 - Flags: review?(honzab.moz)
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.
Attachment #8731803 - Flags: review?(honzab.moz) → review+
Attached patch patch v2Splinter Review
ShutdownEvent uses monitor now.
Attachment #8731803 - Attachment is obsolete: true
Attachment #8731836 - Flags: review?(honzab.moz)
Comment on attachment 8731836 [details] [diff] [review]
patch v2

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

Thanks!

Please also uplift.  Probably up to beta?
Attachment #8731836 - Flags: review?(honzab.moz) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/5dbd76a73f7d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Michal, please let this be uplifted.  Thanks.
Flags: needinfo?(michal.novotny)
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
Flags: needinfo?(michal.novotny)
Attachment #8731836 - Flags: approval-mozilla-beta?
Attachment #8731836 - Flags: approval-mozilla-aurora?
What are the crash signatures we should look for here (that may exist now and will hopefully go away?)
Flags: qe-verify+
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.
Attachment #8731836 - Flags: approval-mozilla-beta?
Attachment #8731836 - Flags: approval-mozilla-beta+
Attachment #8731836 - Flags: approval-mozilla-aurora?
Attachment #8731836 - Flags: approval-mozilla-aurora+
(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.
See Also: → 1263199
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.