Startup race fixes to tracked objects

Misc fixes to tracked objects

PendingTask::post_births could sometimes be left unitialized
No way for user to know if Tracked::GetBirthPlace will hit tracked_births_ NULL pointer exception.
nit: MissingBirthplace has inconsistent capitalization


BUG=None
TEST=base_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92663

[joth, 2011-07-14 20:12:16]

I've been using tracked objects in a private project to profile application
startup, and came across some race-condition bad pointer crashes that this patch
fixes.
Let me know if someone else should review or if you'd like these handled a
different way.

[awong, 2011-07-14 20:22:20]

LGTM

Good catch! jar@ and darin@ might want to also give this a sanity check.

[darin (slow to review), 2011-07-14 20:37:33]

http://codereview.chromium.org/7375006/diff/2001/base/tracked.cc
File base/tracked.cc (right):

http://codereview.chromium.org/7375006/diff/2001/base/tracked.cc#newcode133
base/tracked.cc:133: static Location kNone("UnknownFunctionName", "UnknownFile",
-1, NULL);
are you certain we don't have to worry about multiple threads calling
GetBirthPlace() at the same time?  function level static variables are lazily
initialized without any sort of thread-safety.  this could create an additional
kNone object in some cases.

[awong, 2011-07-14 20:40:39]

http://codereview.chromium.org/7375006/diff/2001/base/tracked.cc
File base/tracked.cc (right):

http://codereview.chromium.org/7375006/diff/2001/base/tracked.cc#newcode89
base/tracked.cc:89: #if defined(TRACK_ALL_TASK_OBJECTS)
This condition got inverted...

http://codereview.chromium.org/7375006/diff/2001/base/tracked.cc#newcode133
base/tracked.cc:133: static Location kNone("UnknownFunctionName", "UnknownFile",
-1, NULL);
On 2011/07/14 20:37:34, darin wrote:
> are you certain we don't have to worry about multiple threads calling
> GetBirthPlace() at the same time?  function level static variables are lazily
> initialized without any sort of thread-safety.  this could create an
additional
> kNone object in some cases.

If that's an issue, the Tracked::GetBirthPlace() function in the "compiled-out"
side has had the race for a very long time.

[darin (slow to review), 2011-07-14 20:49:04]

On Thu, Jul 14, 2011 at 1:40 PM, <ajwong@chromium.org> wrote:

>
>
http://codereview.chromium.**org/7375006/diff/2001/base/**tracked.cc<http://c...
> File base/tracked.cc (right):
>
> http://codereview.chromium.**org/7375006/diff/2001/base/**
>
tracked.cc#newcode89<http://codereview.chromium.org/7375006/diff/2001/base/tracked.cc#newcode89>
> base/tracked.cc:89: #if defined(TRACK_ALL_TASK_**OBJECTS)
> This condition got inverted...
>
>
> http://codereview.chromium.**org/7375006/diff/2001/base/**
>
tracked.cc#newcode133<http://codereview.chromium.org/7375006/diff/2001/base/tracked.cc#newcode133>
> base/tracked.cc:133: static Location kNone("UnknownFunctionName",
> "UnknownFile", -1, NULL);
> On 2011/07/14 20:37:34, darin wrote:
>
>> are you certain we don't have to worry about multiple threads calling
>> GetBirthPlace() at the same time?  function level static variables are
>>
> lazily
>
>> initialized without any sort of thread-safety.  this could create an
>>
> additional
>
>> kNone object in some cases.
>>
>
> If that's an issue, the Tracked::GetBirthPlace() function in the
> "compiled-out" side has had the race for a very long time.


since kNone is just readonly data, it probably doesn't matter.
-darin


>
>
>
http://codereview.chromium.**org/7375006/<http://codereview.chromium.org/7375...
>

[darin (slow to review), 2011-07-14 20:50:27]

> since kNone is just readonly data, it probably doesn't matter.

actually, because it is essentially just plain old data, initializing those
fields more than once doesn't matter.  there's no heap allocations that would
get lost, etc.

[darin (slow to review), 2011-07-14 20:50:32]

LGTM

[joth, 2011-07-14 20:53:51]

On 14 July 2011 21:37, <darin@chromium.org> wrote:

>
>
http://codereview.chromium.**org/7375006/diff/2001/base/**tracked.cc<http://c...
> File base/tracked.cc (right):
>
> http://codereview.chromium.**org/7375006/diff/2001/base/**
>
tracked.cc#newcode133<http://codereview.chromium.org/7375006/diff/2001/base/tracked.cc#newcode133>
> base/tracked.cc:133: static Location kNone("UnknownFunctionName",
> "UnknownFile", -1, NULL);
> are you certain we don't have to worry about multiple threads calling
> GetBirthPlace() at the same time?  function level static variables are
> lazily initialized without any sort of thread-safety.  this could create
> an additional kNone object in some cases.
>
>
> Good point. I copied this directly from the other (!TRACK_ALL_TASK_OBJECTS)
version of  GetBirthPlace(), so probably should fix there too. Just avoid
the static a return Location(...) should be fine. (That's no more expensive
any other FROM_HERE invocation)

Actually as I now fixed MissingBirthPlace() my client code now avoid calling
in here if there is no tracked_births_ is NULL. So for me, a
CHECK(tracked_births_) would be fine. I'd still need to fix the other
version though. (unless I switch the !TRACK_ALL_TASK_OBJECTS version
of  MissingBirthPlace()  to always return true instead?)

Cheers

[joth, 2011-07-14 20:56:41]

Mails crossed in the air...
sounds like this is probably OK as is then (with the typo fix -- thanks!)

http://codereview.chromium.org/7375006/diff/2001/base/tracked.cc
File base/tracked.cc (right):

http://codereview.chromium.org/7375006/diff/2001/base/tracked.cc#newcode89
base/tracked.cc:89: #if defined(TRACK_ALL_TASK_OBJECTS)
On 2011/07/14 20:40:39, awong wrote:
> This condition got inverted...

Done.

[awong, 2011-07-14 21:02:04]

LGTM

(still) :)

http://codereview.chromium.org/7375006/diff/7001/base/tracked.cc
File base/tracked.cc (right):

http://codereview.chromium.org/7375006/diff/7001/base/tracked.cc#newcode145
base/tracked.cc:145: #endif  // defined(TRACK_ALL_TASK_OBJECTS)
!

[joth, 2011-07-15 07:57:24]

Thanks! Will commit now...

http://codereview.chromium.org/7375006/diff/7001/base/tracked.cc
File base/tracked.cc (right):

http://codereview.chromium.org/7375006/diff/7001/base/tracked.cc#newcode145
base/tracked.cc:145: #endif  // defined(TRACK_ALL_TASK_OBJECTS)
On 2011/07/14 21:02:04, awong wrote:
> !

Done.

[jar (doing other things), 2011-07-18 17:01:48]

(vacation) belated LGTM