AesDecryptor callbacks now used in a thread-safe manner

AesDecryptor callbacks now used in a thread-safe manner

Add lock around use of callbacks.

BUG=329530
TEST=browser_tests for encrypted media pass
TBR=glider

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

[jrummell, 2013-12-19 18:24:44]

PTAL. I am unable to run TSAN locally (release build fails, debug build crashes
when starting test that doesn't happen on non_TSAN build).

[xhwang, 2013-12-19 19:15:44]

lgtm % nits

https://codereview.chromium.org/105933004/diff/1/media/cdm/aes_decryptor.h
File media/cdm/aes_decryptor.h (right):

https://codereview.chromium.org/105933004/diff/1/media/cdm/aes_decryptor.h#ne...
media/cdm/aes_decryptor.h:139: // main thread but called on the compositor
thread. http://crbug.com/329530
s/compositor/media/

Since you fixed this bug, no need to put the bug here.

https://codereview.chromium.org/105933004/diff/1/media/cdm/aes_decryptor.h#ne...
media/cdm/aes_decryptor.h:140: mutable base::Lock key_cb_lock_;
s/key_cb/new_key_cb/

[ddorwin, 2013-12-19 19:34:15]

https://codereview.chromium.org/105933004/diff/1/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/105933004/diff/1/media/cdm/aes_decryptor.cc#n...
media/cdm/aes_decryptor.cc:291: base::AutoLock auto_lock(key_cb_lock_);
Should we put this in {} down to 298 so session_ready_cb_ is not inside this
lock?

[Alexander Potapenko, 2013-12-19 19:47:49]

Did you build with use_aura=1? GTK tests deadlock with TSan.
On Dec 19, 2013 10:24 PM, <jrummell@chromium.org> wrote:

> Reviewers: xhwang, Alexander Potapenko,
>
> Message:
> PTAL. I am unable to run TSAN locally (release build fails, debug build
> crashes
> when starting test that doesn't happen on non_TSAN build).
>
>
> Description:
> AesDecryptor callbacks now used in a thread-safe manner
>
> Add lock around use of callbacks.
>
> BUG=329530
> TEST=browser_tests for encrypted media pass
>
> Please review this at https://codereview.chromium.org/105933004/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+8, -3 lines):
>   M media/cdm/aes_decryptor.h
>   M media/cdm/aes_decryptor.cc
>   M tools/valgrind/tsan_v2/suppressions.txt
>
>
> Index: media/cdm/aes_decryptor.cc
> diff --git a/media/cdm/aes_decryptor.cc b/media/cdm/aes_decryptor.cc
> index de6f83474f0febf766d07c9795273720ce2d4834..
> 17f1e5b8454f9382bee3603ebe234ee123118d06 100644
> --- a/media/cdm/aes_decryptor.cc
> +++ b/media/cdm/aes_decryptor.cc
> @@ -288,6 +288,8 @@ void AesDecryptor::UpdateSession(uint32 session_id,
>      }
>    }
>
> +  base::AutoLock auto_lock(key_cb_lock_);
> +
>    if (!new_audio_key_cb_.is_null())
>      new_audio_key_cb_.Run();
>
> @@ -313,6 +315,8 @@ Decryptor* AesDecryptor::GetDecryptor() {
>
>  void AesDecryptor::RegisterNewKeyCB(StreamType stream_type,
>                                      const NewKeyCB& new_key_cb) {
> +  base::AutoLock auto_lock(key_cb_lock_);
> +
>    switch (stream_type) {
>      case kAudio:
>        new_audio_key_cb_ = new_key_cb;
> Index: media/cdm/aes_decryptor.h
> diff --git a/media/cdm/aes_decryptor.h b/media/cdm/aes_decryptor.h
> index a72674c102da801ee351a6c0ef44fc65dd10d291..
> b7354db5546bdf330edfcce8b6e5f5bfc480ad08 100644
> --- a/media/cdm/aes_decryptor.h
> +++ b/media/cdm/aes_decryptor.h
> @@ -135,6 +135,10 @@ class MEDIA_EXPORT AesDecryptor : public MediaKeys,
> public Decryptor {
>    NewKeyCB new_audio_key_cb_;
>    NewKeyCB new_video_key_cb_;
>
> +  // Protect |new_audio_key_cb_| and |new_video_key_cb_| as they are set
> on the
> +  // main thread but called on the compositor thread.
> http://crbug.com/329530
> +  mutable base::Lock key_cb_lock_;
> +
>    DISALLOW_COPY_AND_ASSIGN(AesDecryptor);
>  };
>
> Index: tools/valgrind/tsan_v2/suppressions.txt
> diff --git a/tools/valgrind/tsan_v2/suppressions.txt
> b/tools/valgrind/tsan_v2/suppressions.txt
> index 1903ff6e0dd7b202669450c06629d1e9cc48de67..
> 065d4f3ff9763f25e60c3b886b1bf6680da646e2 100644
> --- a/tools/valgrind/tsan_v2/suppressions.txt
> +++ b/tools/valgrind/tsan_v2/suppressions.txt
> @@ -158,6 +158,3 @@ race:blink::currentTimeFunction
>
>  # http://crbug.com/329460
>  race:extensions::InfoMap::AddExtension
> -
> -# http://crbug.com/329530
> -race:media::AesDecryptor::UpdateSession
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jrummell, 2013-12-19 21:19:24]

On 2013/12/19 19:47:49, Alexander Potapenko wrote:
> Did you build with use_aura=1? GTK tests deadlock with TSan.

Yup. Followed the instructions from
http://dev.chromium.org/developers/testing/threadsanitizer-tsan-v2, changing
base_unittests to browser_tests.

[jrummell, 2013-12-19 23:15:39]

Updated due to comments.

Also got TSAN to run, as mentioned in the bug. No new issues reported:

ThreadSanitizer: Matched 5550 suppressions (pid=11906):
5549 race:third_party/ffmpeg/libavcodec/pthread_frame.c
1 race:PostponeInterruptsScope
ThreadSanitizer: Matched 32 suppressions (pid=11866):
1 race:libglib*.so
4 race:libevent/event.c
26 race:_M_rep
1 race:_M_is_leaked
[1/1] SRC_ClearKey/EncryptedMediaTest.Playback_AudioClearVideo_WebM/0 (14948 ms)
1 test run
Summary of all itest iterations:
End of the summary.
ThreadSanitizer: Matched 1 suppressions (pid=11862):
1 race:_M_rep

https://codereview.chromium.org/105933004/diff/1/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/105933004/diff/1/media/cdm/aes_decryptor.cc#n...
media/cdm/aes_decryptor.cc:291: base::AutoLock auto_lock(key_cb_lock_);
On 2013/12/19 19:34:16, ddorwin wrote:
> Should we put this in {} down to 298 so session_ready_cb_ is not inside this
> lock?

Done.

https://codereview.chromium.org/105933004/diff/1/media/cdm/aes_decryptor.h
File media/cdm/aes_decryptor.h (right):

https://codereview.chromium.org/105933004/diff/1/media/cdm/aes_decryptor.h#ne...
media/cdm/aes_decryptor.h:139: // main thread but called on the compositor
thread. http://crbug.com/329530
On 2013/12/19 19:15:44, xhwang wrote:
> s/compositor/media/
> 
> Since you fixed this bug, no need to put the bug here.

Done.

https://codereview.chromium.org/105933004/diff/1/media/cdm/aes_decryptor.h#ne...
media/cdm/aes_decryptor.h:140: mutable base::Lock key_cb_lock_;
On 2013/12/19 19:15:44, xhwang wrote:
> s/key_cb/new_key_cb/

Done.

[jrummell, 2014-01-06 18:44:24]

+glider@ for OWNERS review of supressions.txt

[commit-bot: I haz the power, 2014-01-09 22:28:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/105933004/50001

[commit-bot: I haz the power, 2014-01-10 00:25:27]

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

[commit-bot: I haz the power, 2014-01-10 01:23:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/105933004/50001

[commit-bot: I haz the power, 2014-01-10 02:54:09]

Change committed as 244056