The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla6

Status

()

Core
XPCOM
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: cpearce, Assigned: cjones)

Tracking

Trunk
mozilla6
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Created attachment 436145 [details] [diff] [review]
Patch v1

Add mozilla::MonitorAutoExit, and fix mozilla::MonitorAutoEnter::NotifyAll().
Assignee: nobody → chris
Attachment #436145 - Flags: review?
(Reporter)

Updated

7 years ago
Attachment #436145 - Flags: review? → review?(jones.chris.g)
(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.
(Reporter)

Updated

7 years ago
Summary: Stack based mozilla::MonitorAutoExit → Make mozilla::Monitor non-reentrant, add mozilla::ReentrantMonitor
(Reporter)

Updated

7 years ago
Attachment #436145 - Attachment is obsolete: true
Attachment #436145 - Flags: review?(jones.chris.g)
(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.
(Reporter)

Comment 10

7 years ago
(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.
(Reporter)

Comment 13

6 years ago
Feel free! I'm not actively working on it.
Assignee: chris → nobody
I've got patches, just need to add some comments.  Maybe better to spin off the content/media ReentrantMonitor->Monitor work into a followup.
Assignee: nobody → jones.chris.g
Tryserver notified me that ImageLayer uses recursive locking.  I'll spin that off too.
Created attachment 523516 [details] [diff] [review]
part 1: Rename Monitor to ReentrantMonitor
Attachment #523516 - Flags: review?(roc)
Created attachment 523517 [details] [diff] [review]
part 1.1: Fix existing Monitor users

Sorry for all the massive goop, but it's all mechanical.
Attachment #523517 - Flags: review?(roc)
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
Attachment #523519 - Flags: superreview?(brendan)
Attachment #523519 - Flags: review?(roc)
Created attachment 523520 [details] [diff] [review]
part 3: IPC code wants to be using non-reentrant Monitor
Attachment #523520 - Flags: review?(bent.mozilla)
Created attachment 523521 [details] [diff] [review]
part 4: TimerThread wants to be using non-reentrant Monitor
Attachment #523521 - Flags: review?(bzbarsky)
Attachment #523520 - Flags: review?(bent.mozilla) → review+
Attachment #523516 - Flags: review?(roc) → review+
Attachment #523517 - Flags: review?(roc) → review+
Attachment #523519 - Flags: review?(roc) → review+
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?
Attachment #523521 - Flags: review?(bzbarsky) → review+
(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
Attachment #523519 - Flags: superreview?(brendan) → superreview+
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 653865
Target Milestone: --- → mozilla6

Comment 28

6 years ago
This caused Bug 653865 [Thunderbird] Trunk builds broken by undeclared 'MonitorAutoEnter'
Depends on: 654976

Updated

6 years ago
Blocks: 696030
No longer blocks: 696030
Depends on: 696030
You need to log in before you can comment on or make changes to this bug.