Fix a reentrance issue in DecryptingDemuxerStream.

Fix a reentrance issue in DecryptingDemuxerStream.

BUG=292590
TEST=none
NOTRY=true

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

[xhwang, 2013-09-17 00:53:49]

PTAL

https://codereview.chromium.org/24192002/diff/1/media/filters/decrypting_demu...
File media/filters/decrypting_demuxer_stream.cc (left):

https://codereview.chromium.org/24192002/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:141:
base::ResetAndReturn(&init_cb_).Run(DECODER_ERROR_NOT_SUPPORTED);
The caller could destroy |this| and create a new one here. If the new one's
address is the same as the old one, the state will be set to kUninitialized,
which triggers a DCHECK because we have a funny state.

[whywhat, 2013-09-17 01:00:39]

lgtm with nits

https://codereview.chromium.org/24192002/diff/1/content/renderer/media/androi...
File content/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/24192002/diff/1/content/renderer/media/androi...
content/renderer/media/android/media_source_delegate.cc:501: media_loop_,
set_decryptor_ready_cb_));
This seems unrelated?

https://codereview.chromium.org/24192002/diff/1/media/filters/decrypting_demu...
File media/filters/decrypting_demuxer_stream.cc (left):

https://codereview.chromium.org/24192002/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:141:
base::ResetAndReturn(&init_cb_).Run(DECODER_ERROR_NOT_SUPPORTED);
On 2013/09/17 00:53:49, xhwang wrote:
> The caller could destroy |this| and create a new one here. If the new one's
> address is the same as the old one, the state will be set to kUninitialized,
> which triggers a DCHECK because we have a funny state.

I'd at least leave a comment in the code saying that Run() should be the last
thing to happen before return (or post it to the message loop instead of relying
on fragile ordering).

https://codereview.chromium.org/24192002/diff/1/media/filters/decrypting_demu...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/24192002/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:132: set_decryptor_ready_cb_.Reset();
Hm, so you reset the callback, then call it and reset it again before it's
destructed? isn't it too much?

[xhwang, 2013-09-17 01:12:59]

https://codereview.chromium.org/24192002/diff/1/content/renderer/media/androi...
File content/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/24192002/diff/1/content/renderer/media/androi...
content/renderer/media/android/media_source_delegate.cc:501: media_loop_,
set_decryptor_ready_cb_));
On 2013/09/17 01:00:40, whywhat wrote:
> This seems unrelated?

Yep, just some other cleanups.

https://codereview.chromium.org/24192002/diff/1/media/filters/decrypting_demu...
File media/filters/decrypting_demuxer_stream.cc (left):

https://codereview.chromium.org/24192002/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:141:
base::ResetAndReturn(&init_cb_).Run(DECODER_ERROR_NOT_SUPPORTED);
I am forcing the post. See line 55 in the new code. But still setting state
before posting callback is a better style.

On 2013/09/17 01:00:40, whywhat wrote:
> On 2013/09/17 00:53:49, xhwang wrote:
> > The caller could destroy |this| and create a new one here. If the new one's
> > address is the same as the old one, the state will be set to kUninitialized,
> > which triggers a DCHECK because we have a funny state.
> 
> I'd at least leave a comment in the code saying that Run() should be the last
> thing to happen before return (or post it to the message loop instead of
relying
> on fragile ordering).

https://codereview.chromium.org/24192002/diff/1/media/filters/decrypting_demu...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/24192002/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:132: set_decryptor_ready_cb_.Reset();
On 2013/09/17 01:00:40, whywhat wrote:
> Hm, so you reset the callback, then call it and reset it again before it's
> destructed? isn't it too much?

Good catch. Done.

[scherkus (not reviewing), 2013-09-17 01:39:19]

any chance for a test?

we have a decrypting_demuxer_stream_unittest.cc

[xhwang, 2013-09-17 02:04:16]

On 2013/09/17 01:39:19, scherkus wrote:
> any chance for a test?
> 
> we have a decrypting_demuxer_stream_unittest.cc

I thought about that. The exact DHCHECK only happens when the address of two
objects are the same, which isn't interesting to test (as it's not reliable to
repro). A more general test case should be testing that there's "no-reentrancy"
into this class. I am not sure how we should check that condition. Adding a new
scoped flag which we set at the beginning at a function call and unset it when a
function exits? That'll work but seems a bit overkill. Thoughts?

[scherkus (not reviewing), 2013-09-17 02:21:11]

sgtm + lgtm -- thanks for the explanation

[commit-bot: I haz the power, 2013-09-17 02:36:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/24192002/7001

[commit-bot: I haz the power, 2013-09-17 09:32:34]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-09-17 11:25:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/24192002/7001

[commit-bot: I haz the power, 2013-09-17 12:14:05]

Change committed as 223594