Last Comment Bug 556214 - Make mozilla::Monitor non-reentrant, add mozilla::ReentrantMonitor
: Make mozilla::Monitor non-reentrant, add mozilla::ReentrantMonitor
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla6
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on: 654976 696030
Blocks: 531340 653865
  Show dependency treegraph
 
Reported: 2010-03-31 01:24 PDT by Chris Pearce (:cpearce)
Modified: 2011-11-02 20:29 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.90 KB, patch)
2010-03-31 01:26 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Review
part 1: Rename Monitor to ReentrantMonitor (13.29 KB, patch)
2011-03-31 22:12 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: review+
Details | Diff | Review
part 1.1: Fix existing Monitor users (323.54 KB, patch)
2011-03-31 22:13 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: review+
Details | Diff | Review
part 2: Create non-reentrant Monitor (6.40 KB, patch)
2011-03-31 22:16 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: review+
brendan: superreview+
Details | Diff | Review
part 3: IPC code wants to be using non-reentrant Monitor (38.18 KB, patch)
2011-03-31 22:16 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Review
part 4: TimerThread wants to be using non-reentrant Monitor (7.78 KB, patch)
2011-03-31 22:16 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bzbarsky: review+
Details | Diff | Review

Description Chris Pearce (:cpearce) 2010-03-31 01:24:30 PDT
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) 2010-03-31 01:26:45 PDT
Created attachment 436145 [details] [diff] [review]
Patch v1

Add mozilla::MonitorAutoExit, and fix mozilla::MonitorAutoEnter::NotifyAll().
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-03-31 12:09:30 PDT
(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) (Exited; email my personal email if necessary) 2010-03-31 14:44:17 PDT
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) (Exited; email my personal email if necessary) 2010-03-31 14:44:49 PDT
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 2010-03-31 14:52:02 PDT
(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 2010-03-31 14:54:58 PDT
(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] 2010-03-31 15:03:16 PDT
Who or what actually needs reentrant monitors?

/be
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-31 15:07:02 PDT
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 2010-03-31 15:10:33 PDT
(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) 2010-03-31 15:36:07 PDT
(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 2010-03-31 15:45:12 PDT
(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 2011-03-29 20:13:06 PDT
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) 2011-03-29 20:14:30 PDT
Feel free! I'm not actively working on it.
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-31 02:39:23 PDT
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 2011-03-31 14:27:32 PDT
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 2011-03-31 22:12:39 PDT
Created attachment 523516 [details] [diff] [review]
part 1: Rename Monitor to ReentrantMonitor
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-31 22:13:15 PDT
Created attachment 523517 [details] [diff] [review]
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 2011-03-31 22:16:02 PDT
Created attachment 523519 [details] [diff] [review]
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 2011-03-31 22:16:24 PDT
Created attachment 523520 [details] [diff] [review]
part 3: IPC code wants to be using non-reentrant Monitor
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-31 22:16:44 PDT
Created attachment 523521 [details] [diff] [review]
part 4: TimerThread wants to be using non-reentrant Monitor
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-05 18:28:47 PDT
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 2011-04-07 14:03:38 PDT
(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 2011-04-20 16:31:39 PDT
sr ping.  Can sr=roc if that's preferable.
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-27 22:32:20 PDT
sr ping.
Comment 25 Brendan Eich [:brendan] 2011-04-28 18:22:28 PDT
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 2011-04-28 18:24:55 PDT
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 28 Philip Chee 2011-04-30 03:59:54 PDT
This caused Bug 653865 [Thunderbird] Trunk builds broken by undeclared 'MonitorAutoEnter'

Note You need to log in before you can comment on or make changes to this bug.