For the new Ogg video decoder backend (bug 531340), we'd like to have a stack based class to automatically exit a monitor in its constructor and re-enter the monitor in its destructor. The logical place to add this would be as a complementary class to mozilla::MonitorAutoEnter, in xpcom/glue/Monitor.h. I've also noticed that MonitorAutoEnter.NotifyAll() only Notify()'s its Monitor, I think the intention was that it should NotifyAll() its monitor.

Add mozilla::MonitorAutoExit, and fix mozilla::MonitorAutoEnter::NotifyAll().

(In reply to comment #0) > For the new Ogg video decoder backend (bug 531340), we'd like to have a stack > based class to automatically exit a monitor in its constructor and re-enter the > monitor in its destructor. The logical place to add this would be as a > complementary class to mozilla::MonitorAutoEnter, in xpcom/glue/Monitor.h. > This came up in bug 486606. My thoughts on MonitorAutoExit are in bug 486606 comment 17. But there was also a call to replace the Monitor here with mutex/condvar (bug 486606 comment 23). Might now be a good time to do that? > I've also noticed that MonitorAutoEnter.NotifyAll() only Notify()'s its > Monitor, I think the intention was that it should NotifyAll() its monitor. Oops! Thanks for the catch.

Discussed with Chris Jones on IRC. Proposal: 1) rename current Monitor to ReentrantMonitor -- renaming the existing users of Monitor, if we think they might need reentrancy: http://mxr.mozilla.org/mozilla-central/search?string=mozilla%3A%3AMonitor, http://mxr.mozilla.org/mozilla-central/search?string=monitorautoenter -- cjones says "geckochildprocesshost doesn't need reentrancy" 2) introduce Monitor as a non-reentrant monitor (mutex + condvar) 3) Monitor supports MonitorAutoExit 4) use the new Monitor in the new Ogg backend sound good?

It might be a good idea to make ReentrantMonitor not support exits at all.

(In reply to comment #3) > Discussed with Chris Jones on IRC. Proposal: > 1) rename current Monitor to ReentrantMonitor > -- renaming the existing users of Monitor, if we think they might need > reentrancy: > http://mxr.mozilla.org/mozilla-central/search?string=mozilla%3A%3AMonitor, > http://mxr.mozilla.org/mozilla-central/search?string=monitorautoenter > -- cjones says "geckochildprocesshost doesn't need reentrancy" Contrary to what I said on IRC it doesn't look like nsMaemoNetworkManager needs reentrancy either because it notifies observers outside the monitor. From a quick scan the layers stuff doesn't appear to need reentrancy either, but you know that better than me ;). > 2) introduce Monitor as a non-reentrant monitor (mutex + condvar) > 3) Monitor supports MonitorAutoExit > 4) use the new Monitor in the new Ogg backend > > sound good? Yes.

(In reply to comment #4) > It might be a good idea to make ReentrantMonitor not support exits at all. You mean explicit exits?

Who or what actually needs reentrant monitors? /be

I don't know, but the current users of Monitor at least need to be audited to make sure they don't.

(In reply to comment #7) > Who or what actually needs reentrant monitors? > > /be There's some scary old code in xpcom/ that hands out pointers to internal PRMonitors.

(In reply to comment #9) > (In reply to comment #7) > > Who or what actually needs reentrant monitors? > > > > /be > > There's some scary old code in xpcom/ that hands out pointers to internal > PRMonitors. The thought of changing PRMonitor sends chills down my spine... Is the best way to make mozilla::Monitor non-reentrant to have Monitor not inherit from BlockingResourceBase, but to merely contain a (appropriately named) mozilla::Mutex and a mozilla::CondVar? It doesn't really make sense for mozilla::Monitor to inherit from BlockingResourceBase and pass BlockingResourceBase's constructor eMonitor, as Monitor couldn't use a PRMonitor underneath.

(In reply to comment #10) > Is the best way to make mozilla::Monitor non-reentrant to have Monitor not > inherit from BlockingResourceBase, but to merely contain a (appropriately > named) mozilla::Mutex and a mozilla::CondVar? Yes.

I'd be happy to steal this, but I'd rather land bug 645263 first so we don't have two global rewrites pending.

Feel free! I'm not actively working on it.

I've got patches, just need to add some comments. Maybe better to spin off the content/media ReentrantMonitor->Monitor work into a followup.

Tryserver notified me that ImageLayer uses recursive locking. I'll spin that off too.

Sorry for all the massive goop, but it's all mechanical.

I've never come across a non-reentrant monitor before, but no where have I seen it written that such a thing can't exist. The API incompatibility with ReentrantMonitor is intentional, and for two reasons - Lock/Unlock vs. Enter/Exit to remind one of non-recursive mutex semantics - ReentrantMonitor<->Monitor switches need a bit more than global replace, hopefully leading folks to think about switched callsites

Comment on attachment 523521 [details] [diff] [review] part 4: TimerThread wants to be using non-reentrant Monitor r=me. This is just syntactic sugar for what we were already doing, right?

(In reply to comment #21) > r=me. This is just syntactic sugar for what we were already doing, right? Yep.

sr ping. Can sr=roc if that's preferable.

sr ping.

Comment on attachment 523519 [details] [diff] [review] part 2: Create non-reentrant Monitor Looks fine to me. Some styles (JS in particular) half-indent labels, including visibility section keywords like public in a class. Never mind if local style here does not. /be

Thanks. (In reply to comment #25) > Some styles (JS in particular) half-indent labels, including visibility section > keywords like public in a class. Never mind if local style here does not. It does not.

http://hg.mozilla.org/mozilla-central/rev/0912e0484031 http://hg.mozilla.org/mozilla-central/rev/54097e09fa35 http://hg.mozilla.org/mozilla-central/rev/85d0f53faa7a http://hg.mozilla.org/mozilla-central/rev/eb89a7783f33

This caused Bug 653865 [Thunderbird] Trunk builds broken by undeclared 'MonitorAutoEnter'