Fix a crash that EnsureMediaStreamClient() does not guarantee media_stream_client_ get created

Fix a crash that EnsureMediaStreamClient() does not guarantee media_stream_client_ get created

If kDisableWebRtc flag is set, EnsureMediaStreamClient() will return early and media_stream_client_ will be NULL.
Subsequent calls to use media_stream_client_ will cause renderer to crash.

BUG=278490
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219629

[qinmin, 2013-08-23 20:56:23]

PTAL

[scherkus (not reviewing), 2013-08-23 21:10:43]

do we know when / why this happens?

there's two cases:
  1) RenderThreadImpl::current() returns null
  2) kDisableWebRTC is true

[qinmin, 2013-08-23 21:17:48]

On 2013/08/23 21:10:43, scherkus wrote:
> do we know when / why this happens?
> 
> there's two cases:
>   1) RenderThreadImpl::current() returns null
>   2) kDisableWebRTC is true

We only have the crash report, no repro step has been reported yet

[wjia(left Chromium), 2013-08-23 21:18:21]

lgtm

[scherkus (not reviewing), 2013-08-23 22:21:40]

based on crash analysis from reports it's due to kDisableWebRtc

qinmin: can you update the CL description to reflect what exactly we're fixing?

https://codereview.chromium.org/23171029/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23171029/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3003: EnsureMediaStreamClient();
I know you want to merge this so we should keep it simple... but the code now
looks a bit silly (and I know I'm to blame here) because we have a function
called EnsureMSC() yet we're still required to check the result :)

can you add a TODO to EMSC() to rename it because it in fact doesn't ensure MSC
is created? (bet yet, feel free to fix it in a follow up CL)

[qinmin, 2013-08-23 22:31:14]

On 2013/08/23 22:21:40, scherkus wrote:
> based on crash analysis from reports it's due to kDisableWebRtc
> 
> qinmin: can you update the CL description to reflect what exactly we're
fixing?
> 
>
https://codereview.chromium.org/23171029/diff/1/content/renderer/render_view_...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/23171029/diff/1/content/renderer/render_view_...
> content/renderer/render_view_impl.cc:3003: EnsureMediaStreamClient();
> I know you want to merge this so we should keep it simple... but the code now
> looks a bit silly (and I know I'm to blame here) because we have a function
> called EnsureMSC() yet we're still required to check the result :)
> 
> can you add a TODO to EMSC() to rename it because it in fact doesn't ensure
MSC
> is created? (bet yet, feel free to fix it in a follow up CL)

How about just rename it to InitializeMSC? Or will the behavior be changed later
to really ensure MSC get created?

[scherkus (not reviewing), 2013-08-23 22:33:42]

On 2013/08/23 22:31:14, qinmin wrote:
> On 2013/08/23 22:21:40, scherkus wrote:
> > based on crash analysis from reports it's due to kDisableWebRtc
> > 
> > qinmin: can you update the CL description to reflect what exactly we're
> fixing?
> > 
> >
>
https://codereview.chromium.org/23171029/diff/1/content/renderer/render_view_...
> > File content/renderer/render_view_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/23171029/diff/1/content/renderer/render_view_...
> > content/renderer/render_view_impl.cc:3003: EnsureMediaStreamClient();
> > I know you want to merge this so we should keep it simple... but the code
now
> > looks a bit silly (and I know I'm to blame here) because we have a function
> > called EnsureMSC() yet we're still required to check the result :)
> > 
> > can you add a TODO to EMSC() to rename it because it in fact doesn't ensure
> MSC
> > is created? (bet yet, feel free to fix it in a follow up CL)
> 
> How about just rename it to InitializeMSC? Or will the behavior be changed
later
> to really ensure MSC get created?

well the ideas I had was around changing the signature to perhaps return a bool,
etc... so let's just get the fix in w/ a TODO and we can debate the best way on
how to rename things later

[qinmin, 2013-08-23 22:40:45]

On 2013/08/23 22:33:42, scherkus wrote:
> On 2013/08/23 22:31:14, qinmin wrote:
> > On 2013/08/23 22:21:40, scherkus wrote:
> > > based on crash analysis from reports it's due to kDisableWebRtc
> > > 
> > > qinmin: can you update the CL description to reflect what exactly we're
> > fixing?
> > > 
> > >
> >
>
https://codereview.chromium.org/23171029/diff/1/content/renderer/render_view_...
> > > File content/renderer/render_view_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23171029/diff/1/content/renderer/render_view_...
> > > content/renderer/render_view_impl.cc:3003: EnsureMediaStreamClient();
> > > I know you want to merge this so we should keep it simple... but the code
> now
> > > looks a bit silly (and I know I'm to blame here) because we have a
function
> > > called EnsureMSC() yet we're still required to check the result :)
> > > 
> > > can you add a TODO to EMSC() to rename it because it in fact doesn't
ensure
> > MSC
> > > is created? (bet yet, feel free to fix it in a follow up CL)
> > 
> > How about just rename it to InitializeMSC? Or will the behavior be changed
> later
> > to really ensure MSC get created?
> 
> well the ideas I had was around changing the signature to perhaps return a
bool,
> etc... so let's just get the fix in w/ a TODO and we can debate the best way
on
> how to rename things later

Done. Cl description updated too.

[qinmin, 2013-08-23 22:46:02]

+creis for OWNER stamp

[scherkus (not reviewing), 2013-08-23 23:18:22]

lgtm -- thanks!

[Charlie Reis, 2013-08-26 16:33:35]

LGTM

[commit-bot: I haz the power, 2013-08-26 16:34:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/23171029/1002

[commit-bot: I haz the power, 2013-08-26 22:54:21]

Change committed as 219629