628967 - Reduce thread usage of WAV decoder

Status: RESOLVED DUPLICATE of bug 592833

(Core :: Audio/Video, defect)

Comment 1

[Julian Seward [:jseward]]

Darn.  That'll teach me to press Enter less often.

STR
1. go to http://luser.github.com

2. click on the green box to start asteroids-like game

3. find PID of firefox-bin process

4. attach GDB to it (gdb /proc/pid/exe pid)

5. continue in gdb (cont)

6. click on space bar to fire gun.  Each firing creates
   10 new threads but only 4 exit:

   [New Thread 0x7fa6d49e3710 (LWP 8608)]
   [New Thread 0x7fa6d29df710 (LWP 8609)]
   [New Thread 0x7fa6d21de710 (LWP 8610)]
   [New Thread 0x7fa6ce9de710 (LWP 8611)]
   [New Thread 0x7fa6ce1dd710 (LWP 8612)]
   [New Thread 0x7fa6cd9dc710 (LWP 8613)]
   [New Thread 0x7fa6cd1db710 (LWP 8614)]
   [New Thread 0x7fa6c79de710 (LWP 8615)]
   [New Thread 0x7fa6c71dd710 (LWP 8616)]
   [New Thread 0x7fa6c69dc710 (LWP 8617)]
   [Thread 0x7fa6cd1db710 (LWP 8614) exited]
   [Thread 0x7fa6d29df710 (LWP 8609) exited]
   [Thread 0x7fa6d21de710 (LWP 8610) exited]
   [Thread 0x7fa6d49e3710 (LWP 8608) exited]

7. After a while, use pstree to find out how many threads
   there are.  I got this after only a few seconds of playing:

    ├─bash─┬─firefox-ff-opt───firefox-bin───159*[{firefox-bin}]
           │      └─gdb

   here we see firefox-bin and it's controlling gdb.  The
   firefox-bin has 159 child threads.

8. Some profiling with "valgrind --tool=massif --pages-as-heap=yes"
   makes it pretty clear that most of these threads came from
   nsWaveDecoder::Load 
   -> NS_NewThread_P 
      -> nsThreadManager::NewThread 
         -> nsThread::Init 
            -> PR_CreateThread 
               -> PR_CreateThread 
                  -> pthread_create
   and about 1/8 as many again came from 
   nsWaveDecoder::StartStateMachineThread

Marking s-s because if true it's an easy remote OOM-DoS.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think we have existing bugs on it taking us 4+ threads to play any media element...

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Do we kill the threads eventually (after page unload and/or cycle/garbage collection)?

Comment 4

[Julian Seward [:jseward]]

I played 2 rounds of the game and extracted the following from the
resulting heap profile.  This is the number of threads simultaneously
live at the maximum point:

# thr    created by, approximately
-----    -------------------------

  127    nsWaveDecoder::Load
   47    pa_thread_new
   13    nsWaveDecoder::StartStateMachineThread
    5    nsThreadPool::PutEvent
    3    mozilla::storage::Connection::getAsyncExecutionTarget
    2    nsPSMBackgroundThread::startThread
    1    NS_InitXPCOM2_P
    1    XPCJSRuntime::XPCJSRuntime
    1    js::GCHelperThread::init
    1    nsUrlClassifierDBService::Init
    1    TimerThread::Init
    1    nsSocketTransportService::Init
    1    nsCycleCollector_startup
    1    nsUrlClassifierDBService::Init
    1    nsCacheService::Init
    1     nsHtml5Module::GetStreamParserThread
  ---
  207

Comment 5

[Julian Seward [:jseward]]

(In reply to comment #3)
> Do we kill the threads eventually (after page unload and/or cycle/garbage
> collection)?

Hard to tell for sure, but:

* at the end of the game there's a message 
  "click anywhere to exit/press esc to quit"
  and when I do that, about half the threads disappear

* after some number of seconds of idleness (maybe 1/2 min)
  a whole bunch more (perhaps 1/2 the remaining) disappear

Comment 6

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

Unless there are thread being leaked by the nsWaveStateMachine, this is a known issue. The threads should be destroyed when playback of the resource ends.

Our existing bug on reducing media thread usage is bug 592833, but that only applies to the nsBuiltinDecoderStateMachine, not the nsWaveStateMachine. We plan to convert the WAV decoder to use the nsBuiltin* architecture (Matthew Gregan has a partial patch for this IIRC), so once we do that, this bug essentially becomes a dupe of bug 592833. Until then, it's a separate issue, since the WAV and Ogg/WebM decoders are separate.

Comment 7

[Benjamin Smedberg]

Why is this marked security-sensitive?

Comment 8

[Boris Zbarsky [:bzbarsky]]

See last line of comment 1?

Comment 9

[Julian Seward [:jseward]]

Yeah, just to clarify: each thread on Linux soaks up 8MB address space.
I generated 200+ threads from a single page with zero effort, which
jacked up the process size by 1.6GB.  It doesn't take much of a stretch
to see I could easily have hit the 3GB default limit.

On OSX and Windows presumably thread stacks are smaller (I don't know)
and so an attacker would have to work a bit harder.  TedM mentioned
though that he'd managed to crash with over 700 live threads using the
same test case.

Comment 10

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

Do we really want security DoS bugs for all APIs attackers can use to cause OOM? That's a lot of APIs :-)

Comment 11

[cajbir (:cajbir)]

Bug 635649 is the wave refactoring bug mentioned in comment 6.

Comment 12

[cajbir (:cajbir)]

Bug 635649 has landed (refactoring of wave backend) which makes this bug a dupe of 592833 as per comment 6.