Provide DllMain, and hook to support TLS callbacks.

Provide windows notifictaion of thread termination

Provide an automatic fallback scan of the linker
list of notifcation callbacks via DllMain.

This fallback is ONLY used if there are no notifications
coming via the linker list.  This allows all existing
code to continue working as it does today, and provides
thread teardown notifcatino on XP even though we use
the LoadLibrary call.

It is a minimal change, suitable for pushing to other
channels (beta? stable?).

r=cpu
BUG=103209
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113321

[jar (doing other things), 2011-11-20 23:56:55]

Ricardo,

This CL has not yet been compiled, but it should make clear what I was
contemplating.  To try to get this "working," I'll need to compile it, and
specify the function name as the DLL entry point.  Can you provide pointers for
where this should be done in our gyp world?

...or is there a reason that you can foresee that would prevent taking this
approach.

+cc: cpu: You might also have comments are suggestions.

Thanks!

[jar (doing other things), 2011-11-21 09:48:35]

I added a little cleverness in this version.  I try to not provide duplicate
calls on Vista et. al., where we're already getting calls, while I think the
code will provide callbacks on XP.

Please comment on both the overall approach, as well as the "would be
cleverness."

Again caveat: This has not yet been compiled... and I'm looking for a way to
spec the enclosed function as our DllMain routine.

[jar (doing other things), 2011-11-30 01:09:19]

This is the new version, using Dllmain() to get notifications, and pass them
along to the ThreadLocalStorage utility in base (which needs to heard about
thread terminating).

I don't have TCMalloc and nspr hooked in yet... but I thought I'd work on that
CL with "specialists" after getting the profiler properly hooked in (the
profiler has a CHECK that I've enabled to verify that the code is working as
planned, and thread termination notifications are reaching the profiler).

[rvargas (doing something else), 2011-11-30 04:00:23]

I like it overall.

Looking at the IFDEF to use TLS callbacks, it occurred to me that maybe we want
to control that at the gyp level. I mean, unless the plan is to leave the old
code around only for easier debugging (i.e. remove it in the not so distant
future), it would be better just to move it to a separate file in base, just
besides tls_win, and only include the file in the solution if some gyp variable
is set (and also set the definition for the dll).

The advantage would be that from the point of view of base, the library can be
"self contained" as in less code is needed outside of the library to be able to
use TLS.

Another option would be to just leave it at the end of tls_win.h, ifdefout with
the same symbol that you use to enable dllmain... but that's a little bit uglier
because it requires the same symbol to be defined in two totally unrelated
places.

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
File base/threading/thread_local_storage_win.cc (right):

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:24: long windows_native_tls_key =
TLS_OUT_OF_INDEXES;
Shouldn't we move to g_windows_native... ?

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:27: // We intentionally skip 0
(claiming it was used) so that it is not confused
Remove the comment about skipping 0

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:61: &windows_native_tls_key, value,
TLS_OUT_OF_INDEXES)) {
nit: needs two extra spaces here

http://codereview.chromium.org/8550009/diff/14001/chrome/app/chrome_dllmain.cc
File chrome/app/chrome_dllmain.cc (right):

http://codereview.chromium.org/8550009/diff/14001/chrome/app/chrome_dllmain.c...
chrome/app/chrome_dllmain.cc:9: 
nit: remove two lines

http://codereview.chromium.org/8550009/diff/14001/chrome/app/chrome_dllmain.c...
chrome/app/chrome_dllmain.cc:25: }  // namespace  anonymous
nit: just "namespace", and add a line before and after OnThreadTermination

http://codereview.chromium.org/8550009/diff/14001/chrome/app/chrome_dllmain.c...
chrome/app/chrome_dllmain.cc:28: // This mechanism works on XP, and on more
recent versions of Windows.
nit: ...works on all versions of Windows

[rvargas (doing something else), 2011-11-30 04:01:49]

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
File base/threading/thread_local_storage.h (right):

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
base/threading/thread_local_storage.h:78: static void **Initialize();
We could also move this to the cc file.

[jar (doing other things), 2011-11-30 07:31:30]

Changes per comments by rvargas

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
File base/threading/thread_local_storage.h (right):

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
base/threading/thread_local_storage.h:78: static void **Initialize();
On 2011/11/30 04:01:49, rvargas wrote:
> We could also move this to the cc file.

Done. (and renamed to be readable)

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
File base/threading/thread_local_storage_win.cc (right):

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:24: long windows_native_tls_key =
TLS_OUT_OF_INDEXES;
On 2011/11/30 04:00:23, rvargas wrote:
> Shouldn't we move to g_windows_native... ?

Done.

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:27: // We intentionally skip 0
(claiming it was used) so that it is not confused
On 2011/11/30 04:00:23, rvargas wrote:
> Remove the comment about skipping 0

This is a somewhat significant comment, even though it is asserted throughout. 
The posix implementation doesn't support this "never used" value, and needs to. 
Once they both have that "feature," then we can get rid of the separate
"initialized" boolean (as we can just check for a non-zero key.

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:61: &windows_native_tls_key, value,
TLS_OUT_OF_INDEXES)) {
On 2011/11/30 04:00:23, rvargas wrote:
> nit: needs two extra spaces here

Done.

http://codereview.chromium.org/8550009/diff/14001/chrome/app/chrome_dllmain.cc
File chrome/app/chrome_dllmain.cc (right):

http://codereview.chromium.org/8550009/diff/14001/chrome/app/chrome_dllmain.c...
chrome/app/chrome_dllmain.cc:9: 
On 2011/11/30 04:00:23, rvargas wrote:
> nit: remove two lines

Done.

http://codereview.chromium.org/8550009/diff/14001/chrome/app/chrome_dllmain.c...
chrome/app/chrome_dllmain.cc:25: }  // namespace  anonymous
On 2011/11/30 04:00:23, rvargas wrote:
> nit: just "namespace", and add a line before and after OnThreadTermination

Done.

http://codereview.chromium.org/8550009/diff/14001/chrome/app/chrome_dllmain.c...
chrome/app/chrome_dllmain.cc:28: // This mechanism works on XP, and on more
recent versions of Windows.
On 2011/11/30 04:00:23, rvargas wrote:
> nit: ...works on all versions of Windows

Done.

[rvargas (doing something else), 2011-11-30 18:48:41]

Even though I said the new file should go inside chrome/app, we have a problem
with that... test are not going to work now :(

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
File base/threading/thread_local_storage_win.cc (right):

http://codereview.chromium.org/8550009/diff/14001/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:27: // We intentionally skip 0
(claiming it was used) so that it is not confused
On 2011/11/30 07:31:30, jar wrote:
> On 2011/11/30 04:00:23, rvargas wrote:
> > Remove the comment about skipping 0
> 
> This is a somewhat significant comment, even though it is asserted throughout.

> The posix implementation doesn't support this "never used" value, and needs
to. 
> Once they both have that "feature," then we can get rid of the separate
> "initialized" boolean (as we can just check for a non-zero key.

Right... I got confused by the change to 0 here (even though I noticed the
removal of -1)

http://codereview.chromium.org/8550009/diff/17002/base/threading/thread_local...
File base/threading/thread_local_storage_win.cc (right):

http://codereview.chromium.org/8550009/diff/17002/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:88: }  // namespace anonymous
nit: drop "anonymous"

http://codereview.chromium.org/8550009/diff/17002/chrome/app/chrome_dllmain.cc
File chrome/app/chrome_dllmain.cc (right):

http://codereview.chromium.org/8550009/diff/17002/chrome/app/chrome_dllmain.c...
chrome/app/chrome_dllmain.cc:25: }  // namespace  anonymous
nit: drop "anonymous"

[jar (doing other things), 2011-11-30 19:22:00]

Changes per comments by rvargas.

http://codereview.chromium.org/8550009/diff/17002/base/threading/thread_local...
File base/threading/thread_local_storage_win.cc (right):

http://codereview.chromium.org/8550009/diff/17002/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:88: }  // namespace anonymous
On 2011/11/30 18:48:41, rvargas wrote:
> nit: drop "anonymous"

Done.

http://codereview.chromium.org/8550009/diff/17002/chrome/app/chrome_dllmain.cc
File chrome/app/chrome_dllmain.cc (right):

http://codereview.chromium.org/8550009/diff/17002/chrome/app/chrome_dllmain.c...
chrome/app/chrome_dllmain.cc:25: }  // namespace  anonymous
On 2011/11/30 18:48:41, rvargas wrote:
> nit: drop "anonymous"

Done.

[jar (doing other things), 2011-11-30 23:36:50]

Moved DllMain into content.

jam@ please approve as owner for content

[rvargas (doing something else), 2011-12-01 00:58:51]

We still have a problem with the unit tests (they will leak, and base_unittests
will fail).

We know that:
1. DllMain cannot be compiled as part of any static library (base or anything
else). It has to be part of the actual project that builds a DLL.

2. We need TLS callbacks for regular executables.

---

We could:
1. Move the TLS callbacks part to test_support_base (base/test)... that should
probably fix all tests (or executables from content?), while leaving DllMain at
the application layer, of whoever builds a DLL (it is 4 lines of code!).

2. Leave the two parts together, and move the file somewhere  in base. The file
still cannot be part of base.prj, and it would have to be added to the projects
that need it (again, maybe test_support_base, and chrome_dll)

3. Add runtime corrections to be able to ignore double callbacks and leave the
two parts active (TLS callbacks in base, and DllMain somewhere else).

-----

I would prefer to avoid 3 given that this is something fixable at build time.

I consider DllMain something that should be provided (and dealt with) by the app
layer, not by base, so I prefer 1 over 2, but I do see the benefit of providing
a "default" implementation in base that can be used by higher level clients (at
the cost of more "dumping ground for base").

[jar (doing other things), 2011-12-01 02:23:32]

Given that we need (at least half of) the dllmain file in base to run the base
unit tests, I ended up moving it back to base.  I then edited the gypi file for
chrome.dll to reference it, and added it (explicitly) to the base_unittest.exe
(with a macro define to get it to use the non-dll trick to get thread
termination notifications).

jam: I *think* I won't need the OWNERS approval this way... and I *think* it
makes more sense (as making base function requires this file, or at least its
notification functionality, in some form).

[jam, 2011-12-01 04:03:50]

I haven't read all the messages above (sorry, overloaded). but looking at the
latest code, USE_MS_LINKER_HACK_FOR_THREAD_TERMINATION_NOTIFICATIONS would have
to be set by every embedder of content. it seems bad that by default, they will
leak unless they know to set this magic ifdef. that seems fragile and hard to
track down, and adds more work for every embedder.

[rvargas (doing something else), 2011-12-01 19:36:42]

John: It makes sense to invert the default value to match the common case. Just
keep in mind that as it is, every embedder of content, and every other target
(unit tests that are not embedding content, but use TLS) would have to be
modified anyway to add this file to the target.

Looking at the latest version (#12), I really don't have any more comments, and
the only remaining issue is where to put the file (or split it) and how to deal
with gyp. This version at least doesn't break any test, and fixes the issues on
the released product, but something would have to be done for the leaks (not
detected because we don't have coverage on Windows), so I'd be fine checking
that in (so looks good enough, at least for now).

Given that you need an owner of base, I'm nominating Brett :).

Brett: for context, look at my comment from Yesterday (the last one before this
one).

[rvargas (doing something else), 2011-12-01 22:05:13]

Here's my sympathy lgtm, but I'm afraid I'm not going to see the end of this
review (the missing external variable to force a link error that we talked
about).

[jam, 2011-12-01 23:44:27]

On 2011/12/01 19:36:42, rvargas wrote:
> John: It makes sense to invert the default value to match the common case.

I'm not sure which case you mean is the common case? If you mean using a dll, I
don't think that's a common case. Only Chrome needs that for slick
auto-updating.

> Just
> keep in mind that as it is, every embedder of content, and every other target
> (unit tests that are not embedding content, but use TLS) would have to be
> modified anyway to add this file to the target.

What I'm trying to say is that every embedder should get this free. This change
makes it have to do an ifdef. The alternative you provide is it has to include a
file. My opposition was to make an embedder do anything, so that alternative is
the same IMO. Previously, this just worked because the code was compiled into
base (thread_local_storage_win.cc) , which an embedder will get for free without
including any new files.

> 
> Looking at the latest version (#12), I really don't have any more comments,
and
> the only remaining issue is where to put the file (or split it) and how to
deal
> with gyp. This version at least doesn't break any test, and fixes the issues
on
> the released product, but something would have to be done for the leaks (not
> detected because we don't have coverage on Windows), so I'd be fine checking
> that in (so looks good enough, at least for now).
> 
> Given that you need an owner of base, I'm nominating Brett :).
> 
> Brett: for context, look at my comment from Yesterday (the last one before
this
> one).

[jar (doing other things), 2011-12-02 00:12:52]

I flipped the sense of the define (with a new name).  The default will
provide the exe style notifications.  The #define will be needed for a dll.

That CL was uploaded a little while back (I think)

I'm working to make it identifier... to generate a link-time error when you
don't include either... and that is more of a pain.

Jim

On Thu, Dec 1, 2011 at 3:44 PM, <jam@chromium.org> wrote:

> On 2011/12/01 19:36:42, rvargas wrote:
>
>> John: It makes sense to invert the default value to match the common case.
>>
>
> I'm not sure which case you mean is the common case? If you mean using a
> dll, I
> don't think that's a common case. Only Chrome needs that for slick
> auto-updating.
>
>
>  Just
>> keep in mind that as it is, every embedder of content, and every other
>> target
>> (unit tests that are not embedding content, but use TLS) would have to be
>> modified anyway to add this file to the target.
>>
>
> What I'm trying to say is that every embedder should get this free. This
> change
> makes it have to do an ifdef. The alternative you provide is it has to
> include a
> file. My opposition was to make an embedder do anything, so that
> alternative is
> the same IMO. Previously, this just worked because the code was compiled
> into
> base (thread_local_storage_win.cc) , which an embedder will get for free
> without
> including any new files.
>
>
>
>  Looking at the latest version (#12), I really don't have any more
>> comments,
>>
> and
>
>> the only remaining issue is where to put the file (or split it) and how to
>>
> deal
>
>> with gyp. This version at least doesn't break any test, and fixes the
>> issues
>>
> on
>
>> the released product, but something would have to be done for the leaks
>> (not
>> detected because we don't have coverage on Windows), so I'd be fine
>> checking
>> that in (so looks good enough, at least for now).
>>
>
>  Given that you need an owner of base, I'm nominating Brett :).
>>
>
>  Brett: for context, look at my comment from Yesterday (the last one before
>>
> this
>
>> one).
>>
>
>
>
http://codereview.chromium.**org/8550009/<http://codereview.chromium.org/8550...
>

[jam, 2011-12-02 02:34:32]

On 2011/12/02 00:12:52, jar wrote:
> I flipped the sense of the define (with a new name).  The default will
> provide the exe style notifications.  The #define will be needed for a dll.
> 
> That CL was uploaded a little while back (I think)
> 
> I'm working to make it identifier... to generate a link-time error when you
> don't include either... and that is more of a pain.

what happens if both approaches are used together?

[jar (doing other things), 2011-12-02 06:14:30]

I argued with Ricardo in favor of using both, and automatically
(dynamically) allowing which ever one notifies first to permanently disable
the other.  His reasoned argument was that we could do such, but it is
better to link thing correctly (with no extraneous code).

I think his argument has merit, although it might be a lot easier to do
otherwise.  I'm trying to add an external that will reveal when we forget
to link in the notification system.  I'm also adding a check in the
full-blown-Chrome that will detect that we aren't getting the callbacks (as
can happen if you "choose the wrong approach," but still link in something
:-/ ).

I'll see how it goes.

Jim

On Thu, Dec 1, 2011 at 6:34 PM, <jam@chromium.org> wrote:

> On 2011/12/02 00:12:52, jar wrote:
>
>> I flipped the sense of the define (with a new name).  The default will
>> provide the exe style notifications.  The #define will be needed for a
>> dll.
>>
>
>  That CL was uploaded a little while back (I think)
>>
>
>  I'm working to make it identifier... to generate a link-time error when
>> you
>> don't include either... and that is more of a pain.
>>
>
> what happens if both approaches are used together?
>
>
http://codereview.chromium.**org/8550009/<http://codereview.chromium.org/8550...
>

[jam, 2011-12-02 16:42:52]

On Thu, Dec 1, 2011 at 10:14 PM, Jim Roskind <jar@chromium.org> wrote:

> I argued with Ricardo in favor of using both, and automatically
> (dynamically) allowing which ever one notifies first to permanently disable
> the other.  His reasoned argument was that we could do such, but it is
> better to link thing correctly (with no extraneous code).


I agree doing this is "cleaner". However I think this will make it harder
for embedders. If someone just went through all the steps to get and build
content and is trying to use it, having linker errors that they don't
understand (TLS? pick one? which one?) as the first thing is confusing. We
should just make things work automatically and hide the complexity from
embedders. The extra code is so minimal, in comparison to the total build
output.

>
> I think his argument has merit, although it might be a lot easier to do
> otherwise.  I'm trying to add an external that will reveal when we forget
> to link in the notification system.  I'm also adding a check in the
> full-blown-Chrome that will detect that we aren't getting the callbacks (as
> can happen if you "choose the wrong approach," but still link in something
> :-/ ).
>
> I'll see how it goes.
>
> Jim
>
> On Thu, Dec 1, 2011 at 6:34 PM, <jam@chromium.org> wrote:
>
>> On 2011/12/02 00:12:52, jar wrote:
>>
>>> I flipped the sense of the define (with a new name).  The default will
>>> provide the exe style notifications.  The #define will be needed for a
>>> dll.
>>>
>>
>>  That CL was uploaded a little while back (I think)
>>>
>>
>>  I'm working to make it identifier... to generate a link-time error when
>>> you
>>> don't include either... and that is more of a pain.
>>>
>>
>> what happens if both approaches are used together?
>>
>>
http://codereview.chromium.**org/8550009/<http://codereview.chromium.org/8550...
>>
>
>

[jar (doing other things), 2011-12-03 22:42:47]

I'm in agreement with jam's assertion.  The user of the code is best served by
making this as easy as possible. The current code works well on all but XP, and
then only when a DLL is used via LoadLibrary.

I've reverted back to the simple approach I uploaded 2 weeks ago.  A separate
file (base/win/dllmain.cc) watches to see when the poorly documented callback is
failing, and then it takes over support of that service, based on actual
DllMain() notifications.  

I've also cut the CL down to the bare minimum, so that it has a good chance of
being pushed to our beta or stable channel (where XP users are encountering
significant memory leak (due to lack of TCmalloc cleanup after each worker
thread teardown).

[jar (doing other things), 2011-12-06 00:52:33]

cpu: Please take a look.

[cpu_(ooo_6.6-7.5), 2011-12-06 19:15:50]

lgtm

http://codereview.chromium.org/8550009/diff/50001/base/win/dllmain.cc
File base/win/dllmain.cc (right):

http://codereview.chromium.org/8550009/diff/50001/base/win/dllmain.cc#newcode33
base/win/dllmain.cc:33: static void NTAPI on_callback(HINSTANCE h, DWORD
dwReason, PVOID pv);
use same notation as thread_local_storage_win.cc

http://codereview.chromium.org/8550009/diff/50001/base/win/dllmain.cc#newcode57
base/win/dllmain.cc:57: CallbackPointer p_thread_callback_dllmain_first_entry =
on_callback;
this must be const in x64

http://codereview.chromium.org/8550009/diff/50001/base/win/dllmain.cc#newcode60
base/win/dllmain.cc:60: CallbackPointer p_thread_callback_dllmain_later_entry =
on_callback;
use thread_local_storage_win.cc notation like PIMAGE_TLS_CALLBACK

[jar (doing other things), 2011-12-06 19:56:26]

Changes made per comments by cpu

http://codereview.chromium.org/8550009/diff/50001/base/win/dllmain.cc
File base/win/dllmain.cc (right):

http://codereview.chromium.org/8550009/diff/50001/base/win/dllmain.cc#newcode33
base/win/dllmain.cc:33: static void NTAPI on_callback(HINSTANCE h, DWORD
dwReason, PVOID pv);
On 2011/12/06 19:15:50, cpu wrote:
> use same notation as thread_local_storage_win.cc

Done.

http://codereview.chromium.org/8550009/diff/50001/base/win/dllmain.cc#newcode57
base/win/dllmain.cc:57: CallbackPointer p_thread_callback_dllmain_first_entry =
on_callback;
On 2011/12/06 19:15:50, cpu wrote:
> this must be const in x64

Done.

http://codereview.chromium.org/8550009/diff/50001/base/win/dllmain.cc#newcode60
base/win/dllmain.cc:60: CallbackPointer p_thread_callback_dllmain_later_entry =
on_callback;
On 2011/12/06 19:15:50, cpu wrote:
> use thread_local_storage_win.cc notation like PIMAGE_TLS_CALLBACK

Done.

[commit-bot: I haz the power, 2011-12-06 23:47:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/8550009/56001