Modify nsWaveDecoder to use mozilla::Mutex/CondVar

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: cjones, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

PRMonitor is dying out, so code using it should be upgraded to the new mozilla:: synchronization.  It's probably worthwhile to also substitute a mutex+condvar for the monitor.  There are two main reasons:

 (1) Current code uses nsAutoMonitor.Exit/Enter extensively, and these functions have been deprecated.  They're not available on mozilla::MonitorAutoEnter.

 (2) Mutex is non-recursive, whereas Monitor is recursive.  This means that when Monitor.Exit() is called, there's no guarantee you've actually unlocked it.  For this code, that's just asking for a nasty performance bug (or deadlock), and non-recursive acquisition therefore must be an implicit invariant.  Mutex makes this an *explicit* invariant.

I don't mind doing the rewrite, as it should hopefully be relatively easy.

Rob requested this be delayed until after 3.5 ships.

Comment 1

9 years ago
Can you give an example of what the rewritten code will look like? For example, much of the usage of nsMonitor looks like:

{
  nsAutoMonitor mon(...);
  while (1) {
    ...do stuff...
    mon.Exit();
    ..do blocking stuff..
    mon.Enter();
    ..do more stuff..
  }
}

Does 'Wait' still exist? Code uses Wait and NotifyAll in cases like:

Thread A:
  {
    nsAutoMonitor mon(...);
    if (are we buffering?())
      mon.Wait();

    ...decode...
  }

Thread B:
  {
    if (buffering) {
      nsAutoMonitor mon(...);
      ...do stuff...
      mon.NotifyAll();
     }
  }
(In reply to comment #1)
> Can you give an example of what the rewritten code will look like?

Sure:

 {
   MutexAutoLock _(lock);
   while (1) {
     ... do stuff ...
     {
       MutexAutoExit __(lock);
       ... do blocking stuff ...
     }
     // we get the lock back when exiting the above scope
     ... do more stuff ...
   }
 }

> Does 'Wait' still exist? Code uses Wait and NotifyAll in cases like:
> 

Yes, they're methods of the CondVar.  So you'll do:

Thread A:
 {
   MutexAutoLock _(lock);
   ...
   condVar.wait();
   ...
 }

Thread B:
 {
   MutexAutoLock _(lock);
   ...
   condVar.NotifyAll();
   ...
 }

Also, Mutex provides the methods |AssertCurrentThreadOwns()| and |AssertNotCurrentThreadOwns()|, and CondVar provides |AssertCurrentThreadOwnsMutex()| and |AssertNotCurrentThreadOwnsMutex()|, all of which can be used to enforce locking discipline.
>        MutexAutoExit __(lock);

Whoops, that should be |MutexAutoUnlock|.

Updated

9 years ago
Component: Content → DOM
QA Contact: content → general
(Probably subsumed by some other rewrite.  Please close if so.)
Summary: Modify nsWaveDecoder and nsOggDecoder to use mozilla::Mutex/CondVar → Modify nsWaveDecoder to use mozilla::Mutex/CondVar
(In reply to comment #4)
> (Probably subsumed by some other rewrite.  Please close if so.)

Nope, still there.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.