Oilpan: Disabling lazy sweeping shouldn't deactivate EAGERLY_FINALIZE.

Oilpan: Disabling lazy sweeping shouldn't deactivate EAGERLY_FINALIZE.

Classes with EAGERLY_FINALIZE macro must be eagerly finalized irrespective of
lazy sweeping enabled or disabled, because otherwise the destructor of such
a class runs in an undefined order and may touch a pointer that's already
freed up.

This was found by Blink GC plugin making an error when built with
ENABLE_OILPAN=1 and ENABLE_LAZY_SWEEPING=0 configuration.

BUG=none
R=oilpan-reviews@chromium.org, haraken@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196842

[haraken, 2015-06-09 10:24:57]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2015-06-09 10:24:57]

LGTM, but Sigbjorn should confirm.

[sof, 2015-06-09 13:58:51]

The combinations are getting out of hand.. but we do need to support running the
destructors for !ENABLE(OILPAN) && ENABLE(LAZY_SWEEPING).

lgtm (but see below.)

https://codereview.chromium.org/1172603002/diff/1/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (left):

https://codereview.chromium.org/1172603002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1222: #define EAGERLY_FINALIZE_WILL_BE_REMOVED()
This strikes me as wrong; we need to eagerly finalize some mixins with
!ENABLE(OILPAN) when lazy sweeping is enabled, but aren't. That's a problem with
trunk, not this CL per se.

[sof, 2015-06-09 13:59:42]

On 2015/06/09 13:58:51, sof wrote:
> The combinations are getting out of hand.. but we do need to support running
the
> destructors for !ENABLE(OILPAN) && ENABLE(LAZY_SWEEPING).
> 

ENABLE(OILPAN) && !ENABLE(LAZY_SWEEPING) :)

[sof, 2015-06-09 14:48:10]

https://codereview.chromium.org/1172603002/diff/1/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (left):

https://codereview.chromium.org/1172603002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1222: #define EAGERLY_FINALIZE_WILL_BE_REMOVED()
On 2015/06/09 13:58:51, sof wrote:
> This strikes me as wrong; we need to eagerly finalize some mixins with
> !ENABLE(OILPAN) when lazy sweeping is enabled, but aren't. That's a problem
with
> trunk, not this CL per se.

I think the analysis from 


https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/u3dax9mf9Go/a1o...

is still valid & not addressed on trunk in other ways, non-Oilpan.

Trying to reproduce the referred-to ASan failure locally; will see where that
goes before following up.

[Yuta Kitamura, 2015-06-10 06:15:31]

Thanks, landing this now.

[Yuta Kitamura, 2015-06-10 06:15:37]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2015-06-10 06:15:43]

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

[commit-bot: I haz the power, 2015-06-10 07:29:48]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196842