Stopgap fix for crash in issue 53867 comment 15

Stopgap fix for crash in issue 53867 comment 15

gwtquake lets the renderer create > 300 threads (one per audio element?), and eventually thread creation fails. This CL makes the media code more robust against thread creation failure (currently, it just crashes the renderer).

The Real Fix probably is to have a thread pool for media stuff instead of one thread per media object. And maybe threads just leak under some circumstances. I will file a follow-up bug for that case, hopefully with a reduced test case.

BUG=53867, 61293
TEST=Completing the first level in gwtquake shouldn't crash the renderer.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67824

[Nico, 2010-11-30 01:39:32]

This only papers over the crash (thread.Start() fails), and something else
crashes a little later, but not returning from the middle of a constructor seems
like the right thing anyway…

[Nico, 2010-12-01 03:13:32]

The CL now fixes all crashes that happen when you complete the first level in
gwtquake. Please take a look.

[scherkus (not reviewing), 2010-12-01 06:37:54]

LGTM w/ nits

thanks again for looking into this -- you are a brave soul for touching
media/base/pipeline_impl.cc :(

may also want to reference bug 61293

the thread pool thing is a known issue... we built the thing assuming "hey, why
would you want a ton of youtube videos at the same time!?!?" but it's the
internet and people like to do crazy things (like quake-gwt)

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:252: fprintf(stderr, "returning early\n");
looks like leftover debugging code

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.h:172: // Callers must call |initialization()|
before they can use the object.
|initialization()| -> initialize()

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.h:178: bool initialize(
nit: actually this method can be Chromium styled (i.e., Initialize vs.
initialize)

the camelCase methods are due to implementing WebMediaPlayer interface, but
since we don't really want to have to change that interface we can simply go
with Initialize() with a capital I

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.h:179: MediaResourceLoaderBridgeFactory*
bridge_factory_simple,
nit: indent by 4 spaces

[Nico, 2010-12-01 06:46:04]

Thanks for the review!

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:252: fprintf(stderr, "returning early\n");
On 2010/12/01 06:37:55, scherkus wrote:
> looks like leftover debugging code

Done.

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.h:172: // Callers must call |initialization()|
before they can use the object.
On 2010/12/01 06:37:55, scherkus wrote:
> |initialization()| -> initialize()

Done.

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.h:178: bool initialize(
On 2010/12/01 06:37:55, scherkus wrote:
> nit: actually this method can be Chromium styled (i.e., Initialize vs.
> initialize)
> 
> the camelCase methods are due to implementing WebMediaPlayer interface, but
> since we don't really want to have to change that interface we can simply go
> with Initialize() with a capital I

Done.

http://codereview.chromium.org/5362003/diff/18001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.h:179: MediaResourceLoaderBridgeFactory*
bridge_factory_simple,
On 2010/12/01 06:37:55, scherkus wrote:
> nit: indent by 4 spaces

Done.