Make BootTimesLoader marker usage thread safe

Make BootTimesLoader marker usage thread safe

Manipulate the login/logout marker vectors only on the UI thread.  Fixes crashes
in ~BootTimesLoader.

BUG=chromium-os:24935
TEST=


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

[Rick Byers, 2012-02-27 21:05:23]

Hey Dave,
Can you review my fix for the BootTimesLoader race we discussed?

Thanks,
   Rick

[DaveMoore, 2012-02-27 21:52:19]

http://codereview.chromium.org/9453026/diff/6001/chrome/browser/chromeos/boot...
File chrome/browser/chromeos/boot_times_loader.cc (right):

http://codereview.chromium.org/9453026/diff/6001/chrome/browser/chromeos/boot...
chrome/browser/chromeos/boot_times_loader.cc:445: {
Why can't this all be done in AddMarker (with no AddMarkerImpl)? Is this a const
problem?

[Rick Byers, 2012-02-27 21:58:22]

http://codereview.chromium.org/9453026/diff/6001/chrome/browser/chromeos/boot...
File chrome/browser/chromeos/boot_times_loader.cc (right):

http://codereview.chromium.org/9453026/diff/6001/chrome/browser/chromeos/boot...
chrome/browser/chromeos/boot_times_loader.cc:445: {
On 2012/02/27 21:52:19, DaveMoore wrote:
> Why can't this all be done in AddMarker (with no AddMarkerImpl)? Is this a
const
> problem?

No, AddMarkerImpl is the function that runs on the UI thread (see PostTask
below).  In AddMarker we may be on any thread.   I'm happy to restructure this
to be clearer if you have any suggestions.

[DaveMoore, 2012-02-27 22:00:26]

I mean that AddMarker() which already checks if it's on the UI thread could
simply do the logic in AddMarkerImpl() when it is, otherwise post back to the
proper thread where it will check again, and determine the thread is correct.
On 2012/02/27 21:58:22, Rick Byers wrote:
>
http://codereview.chromium.org/9453026/diff/6001/chrome/browser/chromeos/boot...
> File chrome/browser/chromeos/boot_times_loader.cc (right):
> 
>
http://codereview.chromium.org/9453026/diff/6001/chrome/browser/chromeos/boot...
> chrome/browser/chromeos/boot_times_loader.cc:445: {
> On 2012/02/27 21:52:19, DaveMoore wrote:
> > Why can't this all be done in AddMarker (with no AddMarkerImpl)? Is this a
> const
> > problem?
> 
> No, AddMarkerImpl is the function that runs on the UI thread (see PostTask
> below).  In AddMarker we may be on any thread.   I'm happy to restructure this
> to be clearer if you have any suggestions.

[Rick Byers, 2012-02-27 22:31:16]

On 2012/02/27 22:00:26, DaveMoore wrote:
> I mean that AddMarker() which already checks if it's on the UI thread could
> simply do the logic in AddMarkerImpl() when it is, otherwise post back to the
> proper thread where it will check again, and determine the thread is correct.

Right - that was my initial approach, but I changed it to avoid an unnecessary
additional copy of the TimeMarker (12 bytes).  We need to copy it when
marshaling to the UI thread, but not in AddMarker itself.  Perhaps trying to
save a 12 byte copy is silly though - should I change it to just do the extra
copy?

> On 2012/02/27 21:58:22, Rick Byers wrote:
> >
>
http://codereview.chromium.org/9453026/diff/6001/chrome/browser/chromeos/boot...
> > File chrome/browser/chromeos/boot_times_loader.cc (right):
> > 
> >
>
http://codereview.chromium.org/9453026/diff/6001/chrome/browser/chromeos/boot...
> > chrome/browser/chromeos/boot_times_loader.cc:445: {
> > On 2012/02/27 21:52:19, DaveMoore wrote:
> > > Why can't this all be done in AddMarker (with no AddMarkerImpl)? Is this a
> > const
> > > problem?
> > 
> > No, AddMarkerImpl is the function that runs on the UI thread (see PostTask
> > below).  In AddMarker we may be on any thread.   I'm happy to restructure
this
> > to be clearer if you have any suggestions.

[Rick Byers, 2012-02-28 15:10:34]

Here's the simpler version with the extra copy (actually 20 bytes on 64-bit).  I
like this better.

If we actually cared about the copy, then I should also avoid doing any copy in
the same-thread case (making it more complicated still).

[Rick Byers, 2012-02-29 14:41:42]

Ping - any further comments Dave?

[DaveMoore, 2012-02-29 21:56:11]

lgtm

[commit-bot: I haz the power, 2012-03-01 14:18:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/9453026/19001

[commit-bot: I haz the power, 2012-03-01 15:44:20]

Change committed as 124412