> git gs LazyInstance | wc -l
1005

is a bit more than I expected, but I guess it should be mostly pretty straightforward (he said hopefully).

>git gs LazyInstance | grep -v base\/lazy_instance | wc -l
950

Already 55 fewer!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da667f7e960637d2531cf0306fbc2137633e1773

commit da667f7e960637d2531cf0306fbc2137633e1773
Author: scottmg <scottmg@chromium.org>
Date: Tue Jan 31 02:14:08 2017

Make static initialization threadsafe by default

Example of followups required here: https://codereview.chromium.org/2667513003/.

BUG=686866, 587210 ,686730, crashpad:141 

Review-Url: https://codereview.chromium.org/2667723002
Cr-Commit-Position: refs/heads/master@{#447163}

[modify] https://crrev.com/da667f7e960637d2531cf0306fbc2137633e1773/build/config/compiler/BUILD.gn
[modify] https://crrev.com/da667f7e960637d2531cf0306fbc2137633e1773/build/config/win/BUILD.gn

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/mini_chromium/+/57f426502e000c4af96514e58b541580ad294722

commit 57f426502e000c4af96514e58b541580ad294722
Author: Mark Mentovai <mark@chromium.org>
Date: Tue Jan 31 01:21:58 2017

Enable thread-safe statics when building with GCC and clang

This is effective for macOS and Linux (including Android).

This is the same as upstream da667f7e9606
(https://codereview.chromium.org/2667723002/). /Zc:threadSafeInit- was
never added to mini_chromium as part of  bug chromium:587210 , so there’s
no need to remove it. MSVC supports thread-safe static initialization
since VS2015, GCC has supported it since version 4.0 (2005), and clang
has probably supported it since the beginning.

BUG=chromium:686866

Change-Id: I06cc737a9f27891c1bceda7f1251ace9f1d7cb29
Reviewed-on: https://chromium-review.googlesource.com/435078
Reviewed-by: Scott Graham <scottmg@chromium.org>

[modify] https://crrev.com/57f426502e000c4af96514e58b541580ad294722/build/common.gypi

Will pick up the media/ cleanup.

Hmm, the lazy instance has leaky annotations for msan; how does this work with the thread safe statics that are intentionally leaky?

Yeah, I'm not sure.

I guess we can either add those annotations directly where they make sense, or perhaps we should have a CreateAndLeak<T>() function with the annotation.

msan detects uninitialized reads, not leaks. asan has a component that detects leaks (called lsan), and it doesn't report things reachable from globals -- so things should in theory Just Work (?)

I didn't find an annotation in https://cs.chromium.org/chromium/src/base/lazy_instance.h?q=lazyinstance+package:%5Echromium$&dr=CSs&l=5 -- can you point to what you mean?

It'd be good to have them centralized somehow, otherwise it's going to be annoying; +lsan folk for thoughts.

thakis: https://cs.chromium.org/chromium/src/base/lazy_instance.h?l=97

That said, I haven't even tested to see if this is an issue. I just remember it being one and there is still the annotation.

How does LeakSanitizer decide that something’s leaky?

These aren’t “leaking:” although we never destroy these objects, we maintain a pointer to them from the instant they’re constructed until the end of time.

I think it'll Just Work with lsan if you do something like `static Foo* foo = new Foo`; lazy_instance does some punning in https://cs.chromium.org/chromium/src/base/lazy_instance.h?l=158 which is probably why it needs that annotation. If it didn't just work, I'd consider that an lsan bug.

See "root set" description at https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizerDesignDocument

Aha! OK, cool, let's go with "do nothing" for now then, and if we need some annotations or tool fixes we can revisit.

I did a little bit of easy archaeology and found that the ANNOTATE_SCOPED_MEMORY_LEAK thing in LazyInstance showed up here:

https://chromium.googlesource.com/chromium/src/+/b2206da557c9bb6e7758e02fe74153eacab5c677

Excerpting its commit message:

> The two suppressions removed by this CL have not fired for a long time,
> for some unknown reason.

So it sounds to me like nothing ever detected even badly-named “leaky” LazyInstances as leaks. The previously-suppressed leaks probably had nothing to do with LazyInstance per se, but some other bugs.

> It's still a good idea to annotate all
> intentional leaks though.

I disagree! Adding ANNOTATE_SCOPED_MEMORY_LEAK here was (in my opinion) a mistake because it masks actual potential leaks, which could happen if someone accidentally overwrites the pointer*, or because of the “scoped” nature of the disabling. If any code called, even indirectly, from the initializer contained a leak of its own, it’d be hidden from the sanitizer. I believe that the logic that led to this being added misunderstood the difference between “leaky” in LazyInstance’s parlance and a “leak” in LeakSanitizer’s eyes.

* Because of the possibility of pointer overwriting, we might want to use “const” on the pointer itself in cases where it’s at risk as we replace LazyInstances with thread-safe static initializatons.

It looks like lsan is reporting all allocations created through thread safe statics as leaks if LoadLibrary is used within a non-component build. I hit this in https://codereview.chromium.org/2668813002/ -- so needed to add the leak annotation to the LoadLibrary initializer to resolve the problem.

kcc, it looks like lsan reports leaks on code like `static Foo* foo = new Foo;` on function level if that code is in a dynamic library that's loaded at runtime. Is that expected?

Nope, not expected. 
Do you have a minimized test? 
Is that library ever dlclosed? 

The closest to a minimal is to run media_unittests --gtest_filter=CdmAdapter/AesDecryptorTest.RemoveSession/0 with the patch linked in c#16. It is a base::ScopedNativeLibrary so the library is unloaded upon test tear down.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6ece5aec616aa4d8d06359e426ac154904aabe5b

commit 6ece5aec616aa4d8d06359e426ac154904aabe5b
Author: scottmg <scottmg@chromium.org>
Date: Wed Feb 01 18:25:19 2017

Remove some LazyInstance use in base/

R=mark@chromium.org
TBR=jam@chromium.org
BUG=686866,  587210 , 686730

Review-Url: https://codereview.chromium.org/2667513003
Cr-Commit-Position: refs/heads/master@{#447558}

[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/cpu.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/debug/close_handle_hook_win.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/debug/close_handle_hook_win.h
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/memory/memory_pressure_listener.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/message_loop/message_loop.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/path_service.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/threading/thread_local_storage.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/threading/watchdog.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/threading/worker_pool.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/threading/worker_pool_win.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/time/time_posix.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/time/time_win.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/trace_event/trace_log.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/win/scoped_handle.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/base/win/win_util.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/chrome/app/chrome_main_delegate.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/components/guest_view/renderer/guest_view_container.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/content/utility/in_process_utility_thread.cc
[modify] https://crrev.com/6ece5aec616aa4d8d06359e426ac154904aabe5b/storage/browser/quota/quota_manager.cc

I investigated the effect on codegen here a bit:
https://gist.github.com/sgraham/9be2ac97910b396e7846be95de4f3539
for a specific localized conversion of LazyInstance to static auto x = new X(): https://codereview.chromium.org/2667993003/.

In this case (which I chose as a simple but representative example of how we typically use LazyInstance) it's roughly a wash, or maybe a very small win for threadsafe statics. But it clearly depends on the specific callsite (style, count, etc.) and inlining decisions.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d69ee9d7f4329d536d02cda7b00046b102c55fc1

commit d69ee9d7f4329d536d02cda7b00046b102c55fc1
Author: scottmg <scottmg@chromium.org>
Date: Mon Feb 06 20:55:36 2017

Don't use LazyInstance in chrome/browser/image_decoder.*

R=thakis@chromium.org
BUG=686866

Review-Url: https://codereview.chromium.org/2667993003
Cr-Commit-Position: refs/heads/master@{#448384}

[modify] https://crrev.com/d69ee9d7f4329d536d02cda7b00046b102c55fc1/chrome/browser/image_decoder.cc
[modify] https://crrev.com/d69ee9d7f4329d536d02cda7b00046b102c55fc1/chrome/browser/image_decoder.h

FWIW, this seems to have added about ~35kb of APK size to Android:

https://chromeperf.appspot.com/report?sid=2c1f96f9fbbbb96afcc54d5a6ebf2fde7cea69238a9c08fde42ca654db9736d3&num_points=1500

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c3af50906850b5d9c9439edd80d996e236996c4d

commit c3af50906850b5d9c9439edd80d996e236996c4d
Author: dalecurtis <dalecurtis@chromium.org>
Date: Sat Feb 11 02:08:18 2017

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}

[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/browser/media/capture/audio_mirroring_manager.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/browser/media/cdm_registry_impl.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/browser/media/cdm_registry_impl.h
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/browser/media/media_internals.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/browser/media/media_internals.h
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/browser/media/media_web_contents_observer.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/renderer/media/recorder/video_track_recorder.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/renderer/media/render_media_client.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/renderer/media/render_media_client.h
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/renderer/media/render_media_client_unittest.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/renderer/media/rtc_peer_connection_handler.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/content/renderer/media/webrtc/rtc_stats.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/PRESUBMIT.py
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/audio/audio_manager.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/audio/simple_sources.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/base/android/media_drm_bridge.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/base/android/media_service_throttler.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/base/android/media_service_throttler.h
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/base/key_systems.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/base/media.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/base/mime_util.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/base/win/mf_initializer.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/base/win/mf_initializer.h
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/base/yuv_convert.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/capture/video/linux/video_capture_device_chromeos.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/capture/video/linux/video_capture_device_factory_linux.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/cast/test/utility/udp_proxy_main.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/cdm/external_clear_key_test_helper.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/filters/ffmpeg_glue.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/filters/vpx_video_decoder.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/gpu/android_video_decode_accelerator.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/gpu/avda_codec_allocator.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/gpu/avda_codec_allocator.h
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/gpu/tegra_v4l2_device.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/gpu/vaapi_wrapper.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/gpu/vaapi_wrapper.h
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/midi/midi_manager_alsa.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/midi/midi_manager_winrt.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/midi/usb_midi_device_factory_android.cc
[modify] https://crrev.com/c3af50906850b5d9c9439edd80d996e236996c4d/media/mojo/services/mojo_cdm_service.cc

Hi, can we remove DEFINE_THREAD_SAFE_STATIC_LOCAL from blink as well?

yhirano: Yes, that can be replaced by DEFINE_STATIC_LOCAL now.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03165229c01b328598a7ba00bfd2d4ab6fa79f0a

commit 03165229c01b328598a7ba00bfd2d4ab6fa79f0a
Author: sigbjornf <sigbjornf@opera.com>
Date: Fri Feb 17 15:12:33 2017

Tidy DEFINE_(THREAD_SAFE_)STATIC_LOCAL() implementations.

Move the handling of static local singletons into the
WTF::StaticSingleton<T> wrapper class, including the "same thread"
debug verification.

Use it to also implement the "thread safe" variant also, which can
now be done in a straightforward manner after issue 686866 enabled
C++ thread safe statics.

R=kinuko
BUG=686866

Review-Url: https://codereview.chromium.org/2680843006
Cr-Commit-Position: refs/heads/master@{#451305}

[modify] https://crrev.com/03165229c01b328598a7ba00bfd2d4ab6fa79f0a/third_party/WebKit/Source/wtf/StdLibExtras.h
[modify] https://crrev.com/03165229c01b328598a7ba00bfd2d4ab6fa79f0a/third_party/WebKit/Source/wtf/Threading.h
[modify] https://crrev.com/03165229c01b328598a7ba00bfd2d4ab6fa79f0a/third_party/WebKit/Source/wtf/ThreadingPthreads.cpp
[modify] https://crrev.com/03165229c01b328598a7ba00bfd2d4ab6fa79f0a/third_party/WebKit/Source/wtf/ThreadingWin.cpp

...apparently that's not true due to oilpan, see sof's CL :-)

I think it might be possible to drop (by repositioning the same thread check where vital), but too big a step to make in one go.

Upon further thought, I prefer having Blink be explicit about what kind of threaded access is allowed for a declared singleton, so I won't push towards only having DEFINE_STATIC_LOCAL().

(If anyone wants to discuss this further and/or do more work,  issue 692669  is perhaps the better place for that.)

+asvitkine -- this now means we can drop all the atomic histogram pointer stuff in histogram_macros_internal.h. 

Yep, this was recently brought up on uma-users@:

https://groups.google.com/a/google.com/d/msg/uma-users/KWrPifwNbew/P9zYiZgyAAAJ

SGTM if someone wants to drive this. I'm hoping this would be a nice binary size win.

asvitkine@: I have measured the impact, and it actually increases the binary size (will provide number after I return from vacation, it was order of several hundred kilobytes).
I suspect that the compiler doesn't need to generate any code to initialize the static atomic word used currently in macros, because it's initially zero (a constant). But if  we use a static pointer initialized to point to a histogram, it *will* generate some code which is more general-purpose than the one in histogram macros (which doesn't assume the initialization should happen strictly once).

I see - that's certainly a surprising result, but makes sense given your explanation. In that case, we probably shouldn't change the code...

What's the recommended way to replace LazyInstance<T>::DestructorAtExit? Basically I want the dtor of T to be called when the process terminates.

To use statics, I have to do something like:
static auto t = std::unique_ptr<T>(new T());

But non-POD static variables are not allowed by the style guide [1].

[1] https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

In a first approximation, you should never want to run destructors at exit. In prod builds, those won't reliably run, and in tests it's usually not needed. What's this for?

I understand that we should not count on the dtor always being called. My understanding is that it's more of a "best effort" approach. In my particular case, I am simulating plugin module destruction [1]. I thought using DestructorAtExit would be simpler :)

To work around this, I can either tie the destruction to the thread, or to some other object that has longer lifetime, which is how the current code works in [1].

But, it seems there are still 300+ use cases of DestructorAtExit in the code base [2]. Does this mean it's safe to replace all of them with "Leaky", and hence just use static initialization?

[1] https://cs.chromium.org/chromium/src/content/ppapi_plugin/ppapi_thread.cc?rcl=17aced1485e54a55ab1a1473650c5325b2158e45&l=157
[2] https://cs.chromium.org/search/?q=DestructorAtExit

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a613a7ef943b66521d4120184c2b31a91f9866ca

commit a613a7ef943b66521d4120184c2b31a91f9866ca
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Tue Aug 01 20:36:54 2017

Stop using a LazyInstance in TabStatsTracker.

Using LazyInstance seems to be discouraged (crbug.com/686866) now that
Chrome supports Thread-safe static initialization.

Change-Id: I7649ad76ada5bd2292341f46fa1955d2ae51385e
BUG: 686866, 720131 
Reviewed-on: https://chromium-review.googlesource.com/594491
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491100}
[modify] https://crrev.com/a613a7ef943b66521d4120184c2b31a91f9866ca/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/a613a7ef943b66521d4120184c2b31a91f9866ca/chrome/browser/metrics/tab_stats_tracker.h