Remove LazyInstance usage from media/

Remove LazyInstance usage from media/

We have thread safe statics now, there's no reason to use lazy
instance anymore.

BUG=686866
TEST=compiles.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2668813002
Cr-Commit-Position: refs/heads/master@{#449830}
Committed: https://chromium.googlesource.com/chromium/src/+/c3af50906850b5d9c9439edd80d996e236996c4d

[DaleCurtis, 2017-01-31 20:50:42]

Description was changed from

==========
Remove LazyInstance usage from media/

We have thread safe statics now, there's no reason to use lazy
instance anymore.

BUG=686866
TEST=compiles.
==========

to

==========
Remove LazyInstance usage from media/

We have thread safe statics now, there's no reason to use lazy
instance anymore.

BUG=686866
TEST=compiles.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[DaleCurtis, 2017-01-31 20:52:00]

dalecurtis@chromium.org changed reviewers:
+ scottmg@chromium.org

[DaleCurtis, 2017-01-31 20:52:12]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 20:52:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2017-01-31 21:10:34]

https://codereview.chromium.org/2668813002/diff/1/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:265: key_systems->UpdateIfNeeded();
Are multiple calls to UpdateIfNeeded() a problem? If so, something like:

static KeySystemImpl* key_systems = []() {
  auto ret = new KeySystemsImpl();
  ret->UpdateIfNeeded();
  return ret;
}();
return key_systems;

https://codereview.chromium.org/2668813002/diff/1/media/base/media.cc
File media/base/media.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/base/media.cc#newcode58
media/base/media.cc:58: ~MediaInitializer() { NOTREACHED() << "MediaInitializer
should be leaky!"; }
Probably better just to remove this?

https://codereview.chromium.org/2668813002/diff/1/media/base/win/mf_initializ...
File media/base/win/mf_initializer.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/base/win/mf_initializ...
media/base/win/mf_initializer.cc:12: static bool mf_started =
MFStartup(MF_VERSION, MFSTARTUP_LITE) == S_OK;
Probably going to be a warning here on some compiler.

https://codereview.chromium.org/2668813002/diff/1/media/filters/ffmpeg_glue.cc
File media/filters/ffmpeg_glue.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/filters/ffmpeg_glue.c...
media/filters/ffmpeg_glue.cc:101: static bool initialized =
InitializeInternal();
Could consider inlining the Internal() function here with a [](){} similar to
the other one.

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.cc
File media/gpu/vaapi_wrapper.cc (left):

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.cc#...
media/gpu/vaapi_wrapper.cc:107: base::LazyInstance<VaapiWrapper::VADisplayState>
Just confirming that you really meant for these to go from non-Leaky non-static
to static leaky.

[commit-bot: I haz the power, 2017-01-31 21:14:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 21:14:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scottmg, 2017-01-31 21:14:51]

lgtm

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.cc
File media/gpu/vaapi_wrapper.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.cc#...
media/gpu/vaapi_wrapper.cc:196: if (!profile_infos_->IsProfileSupported(mode,
va_profile)) {
Oh, actually, I think this won't work too. So maybe va_display_state_ and
profile_infos_ are meant to be static which might make things less odd.

[Mark Mentovai, 2017-01-31 21:33:54]

mark@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2017-01-31 21:33:56]

This is great, Dale! Thanks for taking care of this in your corner of the
codebase.

https://codereview.chromium.org/2668813002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2668813002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_peer_connection_handler.cc:1100: CHECK(client_),
GetPeerConnectionHandlers()->insert(this);
; would be more usual than , here.

https://codereview.chromium.org/2668813002/diff/1/content/renderer/media/vide...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/2668813002/diff/1/content/renderer/media/vide...
content/renderer/media/video_track_recorder.cc:104: static CodecEnumerator*
GetCodecEnumerator() {
No need to write “static” in an unnamed namespace.

https://codereview.chromium.org/2668813002/diff/1/media/audio/audio_manager.cc
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/audio/audio_manager.c...
media/audio/audio_manager.cc:262: static AudioManagerHelper* GetHelper() {
No need to write “static” here in an anonymous namespace.

https://codereview.chromium.org/2668813002/diff/1/media/audio/simple_sources.cc
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/audio/simple_sources....
media/audio/simple_sources.cc:96: static BeepContext* GetBeepContext() {
Here too.

https://codereview.chromium.org/2668813002/diff/1/media/base/media.cc
File media/base/media.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/base/media.cc#newcode58
media/base/media.cc:58: ~MediaInitializer() { NOTREACHED() << "MediaInitializer
should be leaky!"; }
Use

  ~MediaInitializer() = delete;

instead. That’ll make accidental deletion a build-time error, which is better
than a run-time error.

https://codereview.chromium.org/2668813002/diff/1/media/base/media.cc#newcode60
media/base/media.cc:60: bool has_platform_decoder_support_ = false;
I don’t think anything is going to be smart enough to optimize this away on
non-Android, even though it looks like this is only used/usable on Android. It’s
not a huge deal in this case, and doesn’t even have any impact on memory
consumption because even if it were gone, MediaInitializer wouldn’t be allowed
to be zero-sized, but I’m mentioning it in case this pattern is in use
elsewhere.

https://codereview.chromium.org/2668813002/diff/1/media/capture/video/linux/v...
File media/capture/video/linux/video_capture_device_chromeos.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/capture/video/linux/v...
media/capture/video/linux/video_capture_device_chromeos.cc:109:
GetCameraConfig()->GetCameraFacing(device_descriptor.device_id,
Since GetCameraConfig() is only called from this one constructor, consider
putting the static in-line in the constructor body and getting rid of the helper
function. This will only be palatable if you can initialize (and are OK with
initializing) these member variables from the constructor body instead of its
initializer list.

https://codereview.chromium.org/2668813002/diff/1/media/cast/test/utility/udp...
File media/cast/test/utility/udp_proxy_main.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/cast/test/utility/udp...
media/cast/test/utility/udp_proxy_main.cc:77: GlobalCounter* GetGlobalCounter()
{
This should live in the unnamed namespace immediately preceding it.

https://codereview.chromium.org/2668813002/diff/1/media/filters/ffmpeg_glue.cc
File media/filters/ffmpeg_glue.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/filters/ffmpeg_glue.c...
media/filters/ffmpeg_glue.cc:101: static bool initialized =
InitializeInternal();
You can eliminate InitializeInternal() by writing its body here with a lambda:

  static bool initialized = [](){
    /* … */
    return true;
  }();

https://codereview.chromium.org/2668813002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:170: ~AVDAManager() {
NOTREACHED(); }
You can switch this to the “= delete” form too.

https://codereview.chromium.org/2668813002/diff/1/media/gpu/tegra_v4l2_device.cc
File media/gpu/tegra_v4l2_device.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/gpu/tegra_v4l2_device...
media/gpu/tegra_v4l2_device.cc:54: bool InitializeTegraFunctionSymbolFinder() {
This should be marked “static”, in an unnamed namespace, or…

https://codereview.chromium.org/2668813002/diff/1/media/gpu/tegra_v4l2_device...
media/gpu/tegra_v4l2_device.cc:78: #undef TEGRAV4L2_DLSYM
This was never #defined, I think it means TEGRAV4L2_DLSYM_OR_RETURN_ON_ERROR.

https://codereview.chromium.org/2668813002/diff/1/media/gpu/tegra_v4l2_device...
media/gpu/tegra_v4l2_device.cc:130: static bool initialized =
InitializeTegraFunctionSymbolFinder();
…written in-line here in a lambda.

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.h
File media/gpu/vaapi_wrapper.h (right):

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.h#n...
media/gpu/vaapi_wrapper.h:356: VADisplayState* va_display_state_;
I don’t see a reason for this to be a member variable that duplicates a static
variable. Get rid of this and just have the static in the .cc file, accessed via
a getter helper function.

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.h#n...
media/gpu/vaapi_wrapper.h:386: LazyProfileInfos* profile_infos_;
This one too.

https://codereview.chromium.org/2668813002/diff/1/media/midi/midi_manager_win...
File media/midi/midi_manager_winrt.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/midi/midi_manager_win...
media/midi/midi_manager_winrt.cc:136: static CombaseFunctions*
GetCombaseFunctions() {
No need for “static” in an unnamed namespace.

[DaleCurtis, 2017-01-31 22:04:34]

Thanks for review. I think I got them all. Are we waiting on landing these until
clank-apk-sizes looks good? Unfortunately that dashboard hasn't updated yet.

https://codereview.chromium.org/2668813002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2668813002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_peer_connection_handler.cc:1100: CHECK(client_),
GetPeerConnectionHandlers()->insert(this);
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> ; would be more usual than , here.

Whoops, surprised that compiled. Done.

https://codereview.chromium.org/2668813002/diff/1/content/renderer/media/vide...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/2668813002/diff/1/content/renderer/media/vide...
content/renderer/media/video_track_recorder.cc:104: static CodecEnumerator*
GetCodecEnumerator() {
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> No need to write “static” in an unnamed namespace.

Done.

https://codereview.chromium.org/2668813002/diff/1/media/audio/audio_manager.cc
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/audio/audio_manager.c...
media/audio/audio_manager.cc:262: static AudioManagerHelper* GetHelper() {
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> No need to write “static” here in an anonymous namespace.

Done.

https://codereview.chromium.org/2668813002/diff/1/media/audio/simple_sources.cc
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/audio/simple_sources....
media/audio/simple_sources.cc:96: static BeepContext* GetBeepContext() {
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> Here too.

Done.

https://codereview.chromium.org/2668813002/diff/1/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:265: key_systems->UpdateIfNeeded();
On 2017/01/31 at 21:10:34, scottmg wrote:
> Are multiple calls to UpdateIfNeeded() a problem? If so, something like:
> 
> static KeySystemImpl* key_systems = []() {
>   auto ret = new KeySystemsImpl();
>   ret->UpdateIfNeeded();
>   return ret;
> }();
> return key_systems;

No, or it'd have been a problem before too. The IfNeeded part of the name is
true, it will check to see if the update is necessary.

https://codereview.chromium.org/2668813002/diff/1/media/base/media.cc
File media/base/media.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/base/media.cc#newcode58
media/base/media.cc:58: ~MediaInitializer() { NOTREACHED() << "MediaInitializer
should be leaky!"; }
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> Use
> 
>   ~MediaInitializer() = delete;
> 
> instead. That’ll make accidental deletion a build-time error, which is better
than a run-time error.

Good idea. Done.

https://codereview.chromium.org/2668813002/diff/1/media/base/media.cc#newcode60
media/base/media.cc:60: bool has_platform_decoder_support_ = false;
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> I don’t think anything is going to be smart enough to optimize this away on
non-Android, even though it looks like this is only used/usable on Android. It’s
not a huge deal in this case, and doesn’t even have any impact on memory
consumption because even if it were gone, MediaInitializer wouldn’t be allowed
to be zero-sized, but I’m mentioning it in case this pattern is in use
elsewhere.

Thanks for the commentary. When I wrote this I just forgot to include
#OS_ANDROID blocks.

https://codereview.chromium.org/2668813002/diff/1/media/base/win/mf_initializ...
File media/base/win/mf_initializer.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/base/win/mf_initializ...
media/base/win/mf_initializer.cc:12: static bool mf_started =
MFStartup(MF_VERSION, MFSTARTUP_LITE) == S_OK;
On 2017/01/31 at 21:10:34, scottmg wrote:
> Probably going to be a warning here on some compiler.

Ah, win_clang complains about unused variable. Added DCHECK() since that seems
appropriate for a function which always assumes success.

https://codereview.chromium.org/2668813002/diff/1/media/capture/video/linux/v...
File media/capture/video/linux/video_capture_device_chromeos.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/capture/video/linux/v...
media/capture/video/linux/video_capture_device_chromeos.cc:109:
GetCameraConfig()->GetCameraFacing(device_descriptor.device_id,
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> Since GetCameraConfig() is only called from this one constructor, consider
putting the static in-line in the constructor body and getting rid of the helper
function. This will only be palatable if you can initialize (and are OK with
initializing) these member variables from the constructor body instead of its
initializer list.

Class marks those variables as const which is why I went with the external
function approach here.

https://codereview.chromium.org/2668813002/diff/1/media/cast/test/utility/udp...
File media/cast/test/utility/udp_proxy_main.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/cast/test/utility/udp...
media/cast/test/utility/udp_proxy_main.cc:77: GlobalCounter* GetGlobalCounter()
{
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> This should live in the unnamed namespace immediately preceding it.

Done, though this whole class is a bit wonky since it has no namespace.

https://codereview.chromium.org/2668813002/diff/1/media/filters/ffmpeg_glue.cc
File media/filters/ffmpeg_glue.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/filters/ffmpeg_glue.c...
media/filters/ffmpeg_glue.cc:101: static bool initialized =
InitializeInternal();
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> You can eliminate InitializeInternal() by writing its body here with a lambda:
> 
>   static bool initialized = [](){
>     /* … */
>     return true;
>   }();

Thanks, done!

https://codereview.chromium.org/2668813002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:170: ~AVDAManager() {
NOTREACHED(); }
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> You can switch this to the “= delete” form too.

Done.

https://codereview.chromium.org/2668813002/diff/1/media/gpu/tegra_v4l2_device.cc
File media/gpu/tegra_v4l2_device.cc (right):

https://codereview.chromium.org/2668813002/diff/1/media/gpu/tegra_v4l2_device...
media/gpu/tegra_v4l2_device.cc:54: bool InitializeTegraFunctionSymbolFinder() {
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> This should be marked “static”, in an unnamed namespace, or…

Inlined.

https://codereview.chromium.org/2668813002/diff/1/media/gpu/tegra_v4l2_device...
media/gpu/tegra_v4l2_device.cc:78: #undef TEGRAV4L2_DLSYM
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> This was never #defined, I think it means TEGRAV4L2_DLSYM_OR_RETURN_ON_ERROR.

Good catch, fixed.

https://codereview.chromium.org/2668813002/diff/1/media/gpu/tegra_v4l2_device...
media/gpu/tegra_v4l2_device.cc:130: static bool initialized =
InitializeTegraFunctionSymbolFinder();
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> …written in-line here in a lambda.

:)

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.cc
File media/gpu/vaapi_wrapper.cc (left):

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.cc#...
media/gpu/vaapi_wrapper.cc:107: base::LazyInstance<VaapiWrapper::VADisplayState>
On 2017/01/31 at 21:10:34, scottmg wrote:
> Just confirming that you really meant for these to go from non-Leaky
non-static to static leaky.

Nope, definitely did not. Totally fubared this one.

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.h
File media/gpu/vaapi_wrapper.h (right):

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.h#n...
media/gpu/vaapi_wrapper.h:356: VADisplayState* va_display_state_;
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> I don’t see a reason for this to be a member variable that duplicates a static
variable. Get rid of this and just have the static in the .cc file, accessed via
a getter helper function.

Agreed, I messed this conversion up anyways, these were static instances. All
converted to internal getters now.

https://codereview.chromium.org/2668813002/diff/1/media/gpu/vaapi_wrapper.h#n...
media/gpu/vaapi_wrapper.h:386: LazyProfileInfos* profile_infos_;
On 2017/01/31 at 21:33:56, Mark Mentovai wrote:
> This one too.

Done.

[DaleCurtis, 2017-01-31 22:04:42]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 22:05:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark Mentovai, 2017-01-31 22:16:00]

LGTM!

https://codereview.chromium.org/2668813002/diff/20001/media/filters/ffmpeg_gl...
File media/filters/ffmpeg_glue.cc (right):

https://codereview.chromium.org/2668813002/diff/20001/media/filters/ffmpeg_gl...
media/filters/ffmpeg_glue.cc:100: CHECK(initialized);
Can you get away with making this a DCHECK like you did with the other one?

https://codereview.chromium.org/2668813002/diff/20001/media/gpu/vaapi_wrapper.cc
File media/gpu/vaapi_wrapper.cc (right):

https://codereview.chromium.org/2668813002/diff/20001/media/gpu/vaapi_wrapper...
media/gpu/vaapi_wrapper.cc:184: va_lock_ = GetDisplayState()->va_lock();
For consistency, you can deal with this in the initializer list too.

[commit-bot: I haz the power, 2017-01-31 22:33:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 22:33:47]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[miu, 2017-01-31 22:41:49]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2017-01-31 22:41:50]

audio_mirroring_manager.cc and udp_proxy_main.cc lgtm

https://codereview.chromium.org/2668813002/diff/20001/content/browser/media/c...
File content/browser/media/capture/audio_mirroring_manager.cc (right):

https://codereview.chromium.org/2668813002/diff/20001/content/browser/media/c...
content/browser/media/capture/audio_mirroring_manager.cc:16: static
AudioMirroringManager* manager = new AudioMirroringManager();
Dumb question: The memory bots are aware that static locals will leak memory and
won't start failing, right?

[Mark Mentovai, 2017-01-31 23:01:34]

miu wrote:
> audio_mirroring_manager.cc and udp_proxy_main.cc lgtm
> 
>
https://codereview.chromium.org/2668813002/diff/20001/content/browser/media/c...
> File content/browser/media/capture/audio_mirroring_manager.cc (right):
> 
>
https://codereview.chromium.org/2668813002/diff/20001/content/browser/media/c...
> content/browser/media/capture/audio_mirroring_manager.cc:16: static
> AudioMirroringManager* manager = new AudioMirroringManager();
> Dumb question: The memory bots are aware that static locals will leak memory
and
> won't start failing, right?

Based on the discussion in
https://bugs.chromium.org/p/chromium/issues/detail?id=686866#c7 through #c14, we
don’t think that we need to do anything, at least until some bot proves us
wrong. Nothing’s “leaking,” we always maintain a pointer to the object as long
as it’s hanging around (which is forever once it’s created).

[DaleCurtis, 2017-02-01 00:14:18]

Hmm, getting a bunch of leaks detected when unloading the cdm library; for some
reason lsan is not skipping these. ANNOTATE_SCOPED_MEMORY_LEAK is doing
something after all. I'll remove the conversions this affects for now.

[DaleCurtis, 2017-02-01 00:14:32]

On 2017/02/01 at 00:14:18, DaleCurtis wrote:
> Hmm, getting a bunch of leaks detected when unloading the cdm library; for
some reason lsan is not skipping these. ANNOTATE_SCOPED_MEMORY_LEAK is doing
something after all. I'll remove the conversions this affects for now.

(see linux_chromium_asan_rel_ng logs)

[DaleCurtis, 2017-02-01 00:30:36]

On 2017/02/01 at 00:14:32, DaleCurtis wrote:
> On 2017/02/01 at 00:14:18, DaleCurtis wrote:
> > Hmm, getting a bunch of leaks detected when unloading the cdm library; for
some reason lsan is not skipping these. ANNOTATE_SCOPED_MEMORY_LEAK is doing
something after all. I'll remove the conversions this affects for now.
> 
> (see linux_chromium_asan_rel_ng logs)

Hmm, actually I can't reproduce these leaks locally. Bots must have a different
config :/

[DaleCurtis, 2017-02-01 00:38:47]

dalecurtis@chromium.org changed reviewers:
+ jrummell@chromium.org

[DaleCurtis, 2017-02-01 00:38:48]

+jrummell since these look very similar to http://crbug.com/569736 and might
just need some updated suppressions.

[DaleCurtis, 2017-02-01 01:14:31]

dalecurtis@chromium.org changed reviewers:
+ glider@chromium.org

[DaleCurtis, 2017-02-01 01:14:32]

Fixed by adding ANNOTATE_SCOPED_MEMORY_LEAK to ExternalClearKeyTestHelper(). It
looks like lsan is capturing all allocations created through thread safe statics
as leaks if LoadLibrary is used within a static build. +glider for confirmation
on if that's the right fix.

[Mark Mentovai, 2017-02-01 01:15:49]

Oh, you need to be able to unload libraries?

[DaleCurtis, 2017-02-01 01:16:14]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 01:17:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2017-02-01 01:20:29]

On 2017/02/01 at 01:15:49, mark wrote:
> Oh, you need to be able to unload libraries?

It's test code, so I'm unsure about "need." John, do we end up going down an
UnloadLibrary path in production code?

[commit-bot: I haz the power, 2017-02-01 01:34:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 01:34:19]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jrummell, 2017-02-02 02:07:08]

On 2017/02/01 01:20:29, DaleCurtis wrote:
> On 2017/02/01 at 01:15:49, mark wrote:
> > Oh, you need to be able to unload libraries?
> 
> It's test code, so I'm unsure about "need." John, do we end up going down an
> UnloadLibrary path in production code?

Currently the CDM is in the pepper process, so libraries aren't unloaded
(they're preloaded before sandboxing is enabled). It will be the same once we
switch to mojo.

I tried looking at the old bug you referenced. I think I came to the conclusion
that ASAN didn't like the statically allocated memory used by the library
because post-processing can't see the annotations once the library is unloaded.
Adding the macro to the code is much nicer than trying to figure out the exact
signature to exclude.

[DaleCurtis, 2017-02-07 01:14:52]

dalecurtis@chromium.org changed reviewers:
+ kcc@chromium.org
- glider@chromium.org

[DaleCurtis, 2017-02-07 01:14:53]

s/glider/kcc/ for sign-off on the suppression in
media/cdm/external_clear_key_test_helper.cc for the unload library call used in
the test.

[kcc2, 2017-02-10 22:50:37]

https://codereview.chromium.org/2668813002/diff/60001/media/cdm/external_clea...
File media/cdm/external_clear_key_test_helper.cc (right):

https://codereview.chromium.org/2668813002/diff/60001/media/cdm/external_clea...
media/cdm/external_clear_key_test_helper.cc:56: ANNOTATE_SCOPED_MEMORY_LEAK;
This deserves a comment. 
A reader looking at this code has no idea why and what leak we are suppressing.

[DaleCurtis, 2017-02-10 23:28:24]

https://codereview.chromium.org/2668813002/diff/60001/media/cdm/external_clea...
File media/cdm/external_clear_key_test_helper.cc (right):

https://codereview.chromium.org/2668813002/diff/60001/media/cdm/external_clea...
media/cdm/external_clear_key_test_helper.cc:56: ANNOTATE_SCOPED_MEMORY_LEAK;
On 2017/02/10 at 22:50:37, kcc2 wrote:
> This deserves a comment. 
> A reader looking at this code has no idea why and what leak we are
suppressing.

Comment added, bug filed to see if this is a real issue.

[kcc2, 2017-02-10 23:30:29]

lgtm

LGTM for media/cdm/external_clear_key_test_helper.cc

[jrummell, 2017-02-11 00:10:55]

media lgtm

https://codereview.chromium.org/2668813002/diff/100001/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/2668813002/diff/100001/media/PRESUBMIT.py#new...
media/PRESUBMIT.py:171: """Check that base::Time and derived classes are passed
by value, and not by
Comment needs updating?

[DaleCurtis, 2017-02-11 00:13:34]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2017-02-11 00:13:36]

Thanks for review!

https://codereview.chromium.org/2668813002/diff/100001/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/2668813002/diff/100001/media/PRESUBMIT.py#new...
media/PRESUBMIT.py:171: """Check that base::Time and derived classes are passed
by value, and not by
On 2017/02/11 at 00:10:55, jrummell wrote:
> Comment needs updating?

Done.

[DaleCurtis, 2017-02-11 00:13:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
scottmg@chromium.org, mark@chromium.org, miu@chromium.org, kcc@chromium.org,
jrummell@chromium.org
Link to the patchset: https://codereview.chromium.org/2668813002/#ps120001
(title: "Fix presubmit comments.")

[commit-bot: I haz the power, 2017-02-11 00:14:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-11 02:08:23]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486772014820330,
"parent_rev": "c4e83bd305c54ef19e2196b8ece1fd2b632d6d08", "commit_rev":
"c3af50906850b5d9c9439edd80d996e236996c4d"}

[commit-bot: I haz the power, 2017-02-11 02:08:59]

Description was changed from

==========
Remove LazyInstance usage from media/

We have thread safe statics now, there's no reason to use lazy
instance anymore.

BUG=686866
TEST=compiles.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove LazyInstance usage from media/

We have thread safe statics now, there's no reason to use lazy
instance anymore.

BUG=686866
TEST=compiles.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2668813002
Cr-Commit-Position: refs/heads/master@{#449830}
Committed:
https://chromium.googlesource.com/chromium/src/+/c3af50906850b5d9c9439edd80d9...
==========

[commit-bot: I haz the power, 2017-02-11 02:09:01]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/c3af50906850b5d9c9439edd80d9...