556214 - Make mozilla::Monitor non-reentrant, add mozilla::ReentrantMonitor

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

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

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

You mean explicit exits?

Comment 7

[Brendan Eich [:brendan]]

Who or what actually needs reentrant monitors?

/be

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Rename Monitor to ReentrantMonitor

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1.1: Fix existing Monitor users

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

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Create non-reentrant Monitor

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 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: IPC code wants to be using non-reentrant Monitor

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: TimerThread wants to be using non-reentrant Monitor

Comment 21

[Boris Zbarsky [:bzbarsky]]

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?

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Yep.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

sr ping.

Comment 25

[Brendan Eich [:brendan]]

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

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 28

[Philip Chee]

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