Revert "Revert "[oilpan]: Make thread shutdown more robust.""

Revert "Revert "[oilpan]: Make thread shutdown more robust.""

This reverts commit 3f2dc8deb5c710a408700d3b564ce1a23894d3ee.

It also adds a fix to the mac release tests seen on the bots. The issue is described in more details in http://code.google.com/p/chromium/issues/detail?id=393832.

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, zerny@chromium.org
BUG=393832, 389647, 391556, 393744
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178273

[wibling-chromium, 2014-07-15 15:24:22]

PTAL.

[haraken, 2014-07-15 15:25:31]

On 2014/07/15 15:24:22, wibling-chromium wrote:
> PTAL.

Where is the diff from the previous CL? :)

[wibling-chromium, 2014-07-15 15:29:20]

On 2014/07/15 15:25:31, haraken wrote:
> On 2014/07/15 15:24:22, wibling-chromium wrote:
> > PTAL.
> 
> Where is the diff from the previous CL? :)

If you look at the diff between patch 1 and 3 for Heap.h and Heap.cpp you will
see the diff for the fix. It is quite small. The asan fix is to not use memset
and the mac test failures (hangs) were due to us not clearing the page with
zeros before adding it to the free pool meaning we could get it back with the
orphaned zap values instead of zeros causing all kinds of issues. The hang was
due to hashTable code assuming the newly allocated backing to be zero filled and
based on this continue to look for an empty entry, but finding only entries with
the orphaned zap value...

[haraken, 2014-07-15 15:31:48]

On 2014/07/15 15:29:20, wibling-chromium wrote:
> On 2014/07/15 15:25:31, haraken wrote:
> > On 2014/07/15 15:24:22, wibling-chromium wrote:
> > > PTAL.
> > 
> > Where is the diff from the previous CL? :)
> 
> If you look at the diff between patch 1 and 3 for Heap.h and Heap.cpp you will
> see the diff for the fix. It is quite small. The asan fix is to not use memset
> and the mac test failures (hangs) were due to us not clearing the page with
> zeros before adding it to the free pool meaning we could get it back with the
> orphaned zap values instead of zeros causing all kinds of issues. The hang was
> due to hashTable code assuming the newly allocated backing to be zero filled
and
> based on this continue to look for an empty entry, but finding only entries
with
> the orphaned zap value...

Thanks, great detective work! LGTM.

[wibling-chromium, 2014-07-15 15:32:51]

On 2014/07/15 15:31:48, haraken wrote:
> On 2014/07/15 15:29:20, wibling-chromium wrote:
> > On 2014/07/15 15:25:31, haraken wrote:
> > > On 2014/07/15 15:24:22, wibling-chromium wrote:
> > > > PTAL.
> > > 
> > > Where is the diff from the previous CL? :)
> > 
> > If you look at the diff between patch 1 and 3 for Heap.h and Heap.cpp you
will
> > see the diff for the fix. It is quite small. The asan fix is to not use
memset
> > and the mac test failures (hangs) were due to us not clearing the page with
> > zeros before adding it to the free pool meaning we could get it back with
the
> > orphaned zap values instead of zeros causing all kinds of issues. The hang
was
> > due to hashTable code assuming the newly allocated backing to be zero filled
> and
> > based on this continue to look for an empty entry, but finding only entries
> with
> > the orphaned zap value...
> 
> Thanks, great detective work! LGTM.

Thanks. The honors goes to Mads who was the one finding the issue :)

[Mads Ager (chromium), 2014-07-16 05:19:30]

LGTM, let's try that again and keep a close eye on the bots. :)

https://codereview.chromium.org/393823003/diff/30001/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/393823003/diff/30001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:159: // Don't use memset when running with ASan
since this needs to zap
I think we should move the comment to before the method. We should make it very
clear that this method should only be used if you need to memset poisoned
memory. Normally you should just use memset.

[wibling-chromium, 2014-07-16 08:45:33]

Thanks for the reviews! I am still testing this on my mac to make sure all
issues are fixed. Will check the commit bit if so.

https://codereview.chromium.org/393823003/diff/30001/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/393823003/diff/30001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:159: // Don't use memset when running with ASan
since this needs to zap
On 2014/07/16 05:19:29, Mads Ager (chromium) wrote:
> I think we should move the comment to before the method. We should make it
very
> clear that this method should only be used if you need to memset poisoned
> memory. Normally you should just use memset.

Done.

[wibling-chromium, 2014-07-16 12:08:00]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-07-16 12:09:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wibling@chromium.org/393823003/50001

[commit-bot: I haz the power, 2014-07-16 12:54:54]

Change committed as 178273