Chromecast teardown race: SetCdm vs. pipeline destruction.

Chromecast teardown race: SetCdm vs. pipeline destruction.

SetCdm travels this path:
* CMFH::SetCdm (IO) --> CMFH::SetCdmOnUiThread (UI) --> MPH::SetCdm (CMA).
while teardown:
* CMFH::DestroyMedia (IO) --> ~MPH (CMA)

This CL adjusts teardown path to go IO --> UI --> CMA. It's a little longer,
but it is now the superset of all IPC-message paths.

R=lcwu@chromium.org,servolk@chromium.org
BUG=internal b/20188230

Committed: https://crrev.com/550e1652df915047ed2442eb19002fc70d3582dd
Cr-Commit-Position: refs/heads/master@{#325677}

[gunsch, 2015-04-16 23:56:10]

A couple notes on other things I toyed around with:

1) Have CmaMessageFilterHost live mostly on the CMA thread. Unfortunately, the
interaction with setting up the shmem-based pipe on the I/O thread made the
class more complex.

2) Maintain a CMA-thread map of media_id-->MPH* references in CMFH. This seems
to push the race condition to other areas, such as MPH creation or CMFH
destruction. I still think this approach might be workable if there's motivation
for it but will take some more time to get right.

[servolk, 2015-04-17 01:23:15]

On 2015/04/16 23:56:10, gunsch wrote:
> A couple notes on other things I toyed around with:
> 
> 1) Have CmaMessageFilterHost live mostly on the CMA thread. Unfortunately, the
> interaction with setting up the shmem-based pipe on the I/O thread made the
> class more complex.
> 
> 2) Maintain a CMA-thread map of media_id-->MPH* references in CMFH. This seems
> to push the race condition to other areas, such as MPH creation or CMFH
> destruction. I still think this approach might be workable if there's
motivation
> for it but will take some more time to get right.

This fix is lgtm
I'm also curious - how are we protected from BrowserCdmCast being destroyed
after we obtained the pointer to it on the UI thread? Could BrowserCdmCast get
destroyed in between the moment when UI thread forwards the call to CMA thread
and the moment when CMA thread gets a chance to actually execute MPH::SetCdm?

[gunsch, 2015-04-17 01:27:17]

On 2015/04/17 01:23:15, servolk wrote:
> On 2015/04/16 23:56:10, gunsch wrote:
> > A couple notes on other things I toyed around with:
> > 
> > 1) Have CmaMessageFilterHost live mostly on the CMA thread. Unfortunately,
the
> > interaction with setting up the shmem-based pipe on the I/O thread made the
> > class more complex.
> > 
> > 2) Maintain a CMA-thread map of media_id-->MPH* references in CMFH. This
seems
> > to push the race condition to other areas, such as MPH creation or CMFH
> > destruction. I still think this approach might be workable if there's
> motivation
> > for it but will take some more time to get right.
> 
> This fix is lgtm
> I'm also curious - how are we protected from BrowserCdmCast being destroyed
> after we obtained the pointer to it on the UI thread? Could BrowserCdmCast get
> destroyed in between the moment when UI thread forwards the call to CMA thread
> and the moment when CMA thread gets a chance to actually execute MPH::SetCdm?

BrowserCdmCast is in two parts:

BrowserCdmCastUi (lives on the UI thread and is registered with
BrowserCdmManager)
BrowserCdmCast (lives on the CMA thread, owned by BrowserCdmCastUi).

So the closest race should be this sequence:
1) [UI] CMFH::SetCdmOnUiThread (gets still-valid BrowserCdmCastUi instance from
BrowserCdmManager), posts task to CMA thread
2) [UI] ~BrowserCdmCastUi (posts task to delete BrowserCdmCast on CMA thread)
3) [CMA] MPH::SetCdm(still-valid BrowserCdmCast instance)
4) [CMA] ~BrowserCdmCast

[lcwu1, 2015-04-17 17:18:57]

lgtm

[gunsch, 2015-04-17 17:30:13]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2015-04-17 17:30:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1091903003/20001

[commit-bot: I haz the power, 2015-04-17 18:12:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-17 18:13:17]

Patchset 2 (id:??) landed as
https://crrev.com/550e1652df915047ed2442eb19002fc70d3582dd
Cr-Commit-Position: refs/heads/master@{#325677}