Add Static Initializer for leak detector TLS

Add Static Initializer for leak detector TLS

This is optional for running leak detector on browser process, but later
it will be necessary to run on renderer process. By initializing the
pthread TLS system early, we avoid initializing it from within an alloc
hook function, in which case it would go into a recursive call loop.

BUG=chromium:615223
Committed: https://crrev.com/386a3c02dede8c906afd6dbda83e28da48c93f8f
Cr-Commit-Position: refs/heads/master@{#397834}

[Simon Que, 2016-06-01 02:50:18]

sque@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-06-01 15:11:28]

jochen@chromium.org changed reviewers:
+ jam@chromium.org

[jochen (gone - plz use gerrit), 2016-06-01 15:11:29]

is there a specific reason to put this into content/app as opposed to
chrome/app?

https://codereview.chromium.org/2023133003/diff/1/components/metrics/leak_det...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/2023133003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.cc:163: // static
nit. empty line above this line

[Simon Que, 2016-06-01 15:40:42]

On 2016/06/01 15:11:29, jochen wrote:
> is there a specific reason to put this into content/app as opposed to
> chrome/app?

No. This could be in chrome/app. How about in
ChromeMainDelegate::BasicStartupComplete?

[Primiano Tucci (use gerrit), 2016-06-01 21:26:29]

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

[Primiano Tucci (use gerrit), 2016-06-01 21:26:30]

not an owner of anything here but the general approach LGTM % putting in the
right shell like Jochen suggested.

https://codereview.chromium.org/2023133003/diff/1/components/metrics/leak_det...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/2023133003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.cc:165: g_hook_data_tls.Get();
ignore_result(g_hook_data_tls.Pointer()); just to document a bit more the intent

https://codereview.chromium.org/2023133003/diff/1/components/metrics/leak_det...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/2023133003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.h:79: static void StaticInit();
I'd call it InitTLSSlot() and not static init.
Technically static initializers are a different (and very evil) thing that we
forbid in chrome.
Here, yes, you are initializing a static thing, but that is not technically a
"static initializer" in the strict sense, as you are doing that in a proper
order and not depending on the loader.
It's just essentially a naming collision with something that you are not doing
:)

https://codereview.chromium.org/2023133003/diff/1/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2023133003/diff/1/content/app/content_main_ru...
content/app/content_main_runner.cc:764: metrics::LeakDetector::StaticInit();
maybe add a comment explaining that the TLS slot needs to be reserved early to
ensure it gets a low slow number and falls in the glibc fast-path which doesn't
require malloc()s.
Otherwise this will be super mysterious and somebody will be tempted to move it.

[Simon Que, 2016-06-02 17:55:17]

https://codereview.chromium.org/2023133003/diff/1/components/metrics/leak_det...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/2023133003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.cc:163: // static
On 2016/06/01 15:11:28, jochen wrote:
> nit. empty line above this line

Done.

https://codereview.chromium.org/2023133003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.cc:165: g_hook_data_tls.Get();
On 2016/06/01 21:26:30, Primiano Tucci wrote:
> ignore_result(g_hook_data_tls.Pointer()); just to document a bit more the
intent

Done.

https://codereview.chromium.org/2023133003/diff/1/components/metrics/leak_det...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/2023133003/diff/1/components/metrics/leak_det...
components/metrics/leak_detector/leak_detector.h:79: static void StaticInit();
On 2016/06/01 21:26:30, Primiano Tucci wrote:
> I'd call it InitTLSSlot() and not static init.
> Technically static initializers are a different (and very evil) thing that we
> forbid in chrome.
> Here, yes, you are initializing a static thing, but that is not technically a
> "static initializer" in the strict sense, as you are doing that in a proper
> order and not depending on the loader.
> It's just essentially a naming collision with something that you are not doing
> :)

Done.

https://codereview.chromium.org/2023133003/diff/1/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2023133003/diff/1/content/app/content_main_ru...
content/app/content_main_runner.cc:764: metrics::LeakDetector::StaticInit();
On 2016/06/01 21:26:30, Primiano Tucci wrote:
> maybe add a comment explaining that the TLS slot needs to be reserved early to
> ensure it gets a low slow number and falls in the glibc fast-path which
doesn't
> require malloc()s.
> Otherwise this will be super mysterious and somebody will be tempted to move
it.
> 

Done.

[jochen (gone - plz use gerrit), 2016-06-03 12:43:19]

lgtm

[Simon Que, 2016-06-03 16:39:46]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-06-03 16:39:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2023133003/#ps20001
(title: "Renamed Init function; added comments; ignore_result; call from
chrome/app")

[commit-bot: I haz the power, 2016-06-03 16:40:17]

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

[commit-bot: I haz the power, 2016-06-03 16:45:16]

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

[commit-bot: I haz the power, 2016-06-03 16:45:17]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Simon Que, 2016-06-03 21:30:46]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-06-03 21:30:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2023133003/#ps40001
(title: "Include protobuf header in unittest; Browser publicly depends on
leak_detector")

[commit-bot: I haz the power, 2016-06-03 21:31:22]

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

[commit-bot: I haz the power, 2016-06-03 23:11:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-03 23:13:36]

Description was changed from

==========
Add Static Initializer for leak detector TLS

This is optional for running leak detector on browser process, but later
it will be necessary to run on renderer process. By initializing the
pthread TLS system early, we avoid initializing it from within an alloc
hook function, in which case it would go into a recursive call loop.

BUG=chromium:615223
==========

to

==========
Add Static Initializer for leak detector TLS

This is optional for running leak detector on browser process, but later
it will be necessary to run on renderer process. By initializing the
pthread TLS system early, we avoid initializing it from within an alloc
hook function, in which case it would go into a recursive call loop.

BUG=chromium:615223

Committed: https://crrev.com/386a3c02dede8c906afd6dbda83e28da48c93f8f
Cr-Commit-Position: refs/heads/master@{#397834}
==========

[commit-bot: I haz the power, 2016-06-03 23:13:37]

Patchset 3 (id:??) landed as
https://crrev.com/386a3c02dede8c906afd6dbda83e28da48c93f8f
Cr-Commit-Position: refs/heads/master@{#397834}