Ensure proper finalization of garbage-collected types.

Ensure proper finalization of garbage-collected types.

This CL reflects the finalization requirements of the Blink GC clang plugin. In
particular, the plugin uses clang's notion of when a class has a 'trivial
finalizer' to ensure that classes that do not have trivial finalizers are
derived from finalized GC base classes.

In order for this to work, the collection types must not have user-declared
destructors when instantiated with the heap allocator. In the case of Vector, the
rules are further complicated since a heap-allocated vector can have inlined
elements, and if they need finalization then so does the heap vector.

R=erik.corry@gmail.com, vegorov@chromium.org
BUG=

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

[Mikhail, 2014-03-11 09:58:12]

https://codereview.chromium.org/189543014/diff/40001/Source/wtf/HashTable.h
File Source/wtf/HashTable.h (right):

https://codereview.chromium.org/189543014/diff/40001/Source/wtf/HashTable.h#n...
Source/wtf/HashTable.h:235: class HashTable : public
HashTableDestructorBase<HashTable<Key, Value, Extractor, HashFunctions, Traits,
KeyTraits, Allocator>, Allocator::isGarbageCollected> {
imho this inheritance looks a bit over-complicated, maybe we could follow the
"VectorDestructor" approach, and have for instance:
~HashTable()
{
HashTableDestructor<Allocator::isGarbageCollected>::destruct(m_table,
m_tableSize);
}

[zerny-chromium, 2014-03-11 10:04:47]

Thanks for the review.

https://codereview.chromium.org/189543014/diff/40001/Source/wtf/HashTable.h
File Source/wtf/HashTable.h (right):

https://codereview.chromium.org/189543014/diff/40001/Source/wtf/HashTable.h#n...
Source/wtf/HashTable.h:235: class HashTable : public
HashTableDestructorBase<HashTable<Key, Value, Extractor, HashFunctions, Traits,
KeyTraits, Allocator>, Allocator::isGarbageCollected> {
On 2014/03/11 09:58:13, mikhail.pozdnyakov wrote:
> imho this inheritance looks a bit over-complicated, maybe we could follow the
> "VectorDestructor" approach, and have for instance:
> ~HashTable()
> {
> HashTableDestructor<Allocator::isGarbageCollected>::destruct(m_table,
> m_tableSize);
> }
> 

The issue with doing so is that then HashTable will have a non-trivial
destructor (because it is user-declared) and thus we can not in the clang
compiler plugin use its semantic knowledge of when a class has a trivial
finalizer to determine when a non-finalized garbage collected base class is
correct.

So while I agree that your proposed code is much nicer, it does not solve the
same issue.

[haraken, 2014-03-11 10:26:36]

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSBasic...
File Source/core/css/CSSBasicShapes.h (right):

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSBasic...
Source/core/css/CSSBasicShapes.h:63: #if !ENABLE(OILPAN)

I'm not that happy to have a lot of ENABLE(OILPAN) flags on destructors for the
following reasons:

- It will mess up the code base.
- (If I'm understanding correctly,) if we add !ENABLE(OILPAN), compilers will
implicitly generate the destructor in the header file. This can cause #include
cycles. (That's why Blink writes empty destructors in cpp files.)

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSCalcu...
File Source/core/css/CSSCalculationValue.h (right):

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSCalcu...
Source/core/css/CSSCalculationValue.h:59: class CSSCalcExpressionNode : public
RefCountedWillBeGarbageCollectedFinalized<CSSCalcExpressionNode> {

Why does this class need to be GarbageCollectedFinalized?

[Erik Corry, 2014-03-11 10:34:25]

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSBasic...
File Source/core/css/CSSBasicShapes.h (right):

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSBasic...
Source/core/css/CSSBasicShapes.h:63: #if !ENABLE(OILPAN)
On 2014/03/11 10:26:37, haraken wrote:
> 
> I'm not that happy to have a lot of ENABLE(OILPAN) flags on destructors for
the
> following reasons:
> 
> - It will mess up the code base.

This is unfortunate, but the warning from the clang plugin seems valuable.

> - (If I'm understanding correctly,) if we add !ENABLE(OILPAN), compilers will
> implicitly generate the destructor in the header file. This can cause #include
> cycles. (That's why Blink writes empty destructors in cpp files.)

I think this issue does not arise, because the destructors that are ifdeffed out
are only the truly trivial ones.  The classes that contain a RefPtr or an OwnPtr
in oilpan mode cannot be ifdeffed out (and the plugin will check this), and
those are the ones that would have caused problems when implicitly generated in
the header file.

[zerny-chromium, 2014-03-11 11:05:14]

On 2014/03/11 10:34:25, Erik Corry wrote:
>
https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSBasic...
> File Source/core/css/CSSBasicShapes.h (right):
> 
>
https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSBasic...
> Source/core/css/CSSBasicShapes.h:63: #if !ENABLE(OILPAN)
> On 2014/03/11 10:26:37, haraken wrote:
> > 
> > I'm not that happy to have a lot of ENABLE(OILPAN) flags on destructors for
> the
> > following reasons:
> > 
> > - It will mess up the code base.
> 
> This is unfortunate, but the warning from the clang plugin seems valuable.

I disagree. This is actual dead code, which will be removed once transitioning
is over.

> > - (If I'm understanding correctly,) if we add !ENABLE(OILPAN), compilers
will
> > implicitly generate the destructor in the header file. This can cause
#include
> > cycles. (That's why Blink writes empty destructors in cpp files.)
> 
> I think this issue does not arise, because the destructors that are ifdeffed
out
> are only the truly trivial ones.  The classes that contain a RefPtr or an
OwnPtr
> in oilpan mode cannot be ifdeffed out (and the plugin will check this), and
> those are the ones that would have caused problems when implicitly generated
in
> the header file.

Exactly, if the class has a non-trivial destructor, eg, if it has a RefPtr
field, then the destructor can and will stay.

[zerny-chromium, 2014-03-11 11:05:25]

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSCalcu...
File Source/core/css/CSSCalculationValue.h (right):

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSCalcu...
Source/core/css/CSSCalculationValue.h:59: class CSSCalcExpressionNode : public
RefCountedWillBeGarbageCollectedFinalized<CSSCalcExpressionNode> {
On 2014/03/11 10:26:37, haraken wrote:
> 
> Why does this class need to be GarbageCollectedFinalized?

Because it has a virtual destructor.

[Mikhail, 2014-03-11 12:02:52]

https://codereview.chromium.org/189543014/diff/40001/Source/wtf/HashTable.h
File Source/wtf/HashTable.h (right):

https://codereview.chromium.org/189543014/diff/40001/Source/wtf/HashTable.h#n...
Source/wtf/HashTable.h:235: class HashTable : public
HashTableDestructorBase<HashTable<Key, Value, Extractor, HashFunctions, Traits,
KeyTraits, Allocator>, Allocator::isGarbageCollected> {
On 2014/03/11 10:04:48, zerny-chromium wrote:
> On 2014/03/11 09:58:13, mikhail.pozdnyakov wrote:
> > imho this inheritance looks a bit over-complicated, maybe we could follow
the
> > "VectorDestructor" approach, and have for instance:
> > ~HashTable()
> > {
> > HashTableDestructor<Allocator::isGarbageCollected>::destruct(m_table,
> > m_tableSize);
> > }
> > 
> 
> The issue with doing so is that then HashTable will have a non-trivial
> destructor (because it is user-declared) and thus we can not in the clang
> compiler plugin use its semantic knowledge of when a class has a trivial
> finalizer to determine when a non-finalized garbage collected base class is
> correct.
> 
> So while I agree that your proposed code is much nicer, it does not solve the
> same issue.

Ah I see, thanks for the explanation.

[haraken, 2014-03-11 13:02:58]

>
https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSBasic...
> > Source/core/css/CSSBasicShapes.h:63: #if !ENABLE(OILPAN)
> > On 2014/03/11 10:26:37, haraken wrote:
> > > 
> > > I'm not that happy to have a lot of ENABLE(OILPAN) flags on destructors
for
> > the
> > > following reasons:
> > > 
> > > - It will mess up the code base.
> > 
> > This is unfortunate, but the warning from the clang plugin seems valuable.
> 
> I disagree. This is actual dead code, which will be removed once transitioning
> is over.

Hmm, I agree that the destructor is dead code but we want to avoid adding a ton
of ENABLE(OILPAN) to the code base. It's a mess :(

Would it be hard for the plugin to detect an empty and non-trivial destructor
without having the !ENABLE(OILPAN) flag?

[Mads Ager (chromium), 2014-03-11 13:06:14]

On 2014/03/11 13:02:58, haraken wrote:
> >
>
https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSBasic...
> > > Source/core/css/CSSBasicShapes.h:63: #if !ENABLE(OILPAN)
> > > On 2014/03/11 10:26:37, haraken wrote:
> > > > 
> > > > I'm not that happy to have a lot of ENABLE(OILPAN) flags on destructors
> for
> > > the
> > > > following reasons:
> > > > 
> > > > - It will mess up the code base.
> > > 
> > > This is unfortunate, but the warning from the clang plugin seems valuable.
> > 
> > I disagree. This is actual dead code, which will be removed once
transitioning
> > is over.
> 
> Hmm, I agree that the destructor is dead code but we want to avoid adding a
ton
> of ENABLE(OILPAN) to the code base. It's a mess :(
> 
> Would it be hard for the plugin to detect an empty and non-trivial destructor
> without having the !ENABLE(OILPAN) flag?

Yeah, the answer to this is that clang will report the class as having a
non-trivial destructor if it has a user defined destructor. That is independent
of whether or not there is any code in the destructor. Clang only says that a
class has a trivial destructor if the destructor is compiler generated and
empty.

[haraken, 2014-03-11 13:21:29]

> > > > > I'm not that happy to have a lot of ENABLE(OILPAN) flags on
destructors
> > for
> > > > the
> > > > > following reasons:
> > > > > 
> > > > > - It will mess up the code base.
> > > > 
> > > > This is unfortunate, but the warning from the clang plugin seems
valuable.
> > > 
> > > I disagree. This is actual dead code, which will be removed once
> transitioning
> > > is over.
> > 
> > Hmm, I agree that the destructor is dead code but we want to avoid adding a
> ton
> > of ENABLE(OILPAN) to the code base. It's a mess :(
> > 
> > Would it be hard for the plugin to detect an empty and non-trivial
destructor
> > without having the !ENABLE(OILPAN) flag?
> 
> Yeah, the answer to this is that clang will report the class as having a
> non-trivial destructor if it has a user defined destructor. That is
independent
> of whether or not there is any code in the destructor. Clang only says that a
> class has a trivial destructor if the destructor is compiler generated and
> empty.

Thanks, I understand the situation. Without having the !ENABLE(OILPAN) flags,
the plugin cannot judge whether the class needs a destructor or not. If the
plugin cannot judge that correctly, we will end up producing hard-to-debug bugs.

tkent-san: What do you think? Are you OK with adding !ENABLE(OILPAN) flags to
destructors of all GarbageCollected classes?

[Mads Ager (chromium), 2014-03-11 13:34:40]

LGTM

I think this is worth it in order to get the checking that we are correctly
using GarbageCollectedFinalized. This will make us fail on the right side by
having too many objects be GarbageCollectedFinalized instead of accidentally
using GarbageCollected when we should not which will lead to hard to find leaks.

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSCalcu...
File Source/core/css/CSSCalculationValue.h (right):

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSCalcu...
Source/core/css/CSSCalculationValue.h:59: class CSSCalcExpressionNode : public
RefCountedWillBeGarbageCollectedFinalized<CSSCalcExpressionNode> {
On 2014/03/11 11:05:25, zerny-chromium wrote:
> On 2014/03/11 10:26:37, haraken wrote:
> > 
> > Why does this class need to be GarbageCollectedFinalized?
> 
> Because it has a virtual destructor.

The virtual destructor is definitely needed when !ENABLE(OILPAN), however, maybe
we should ifdef it out for the ENABLE(OILPAN) build and leave this 
GarbageCollected? As far as I can tell no destructor call is needed here with
Oilpan.

https://codereview.chromium.org/189543014/diff/40001/Source/core/plugins/DOMP...
File Source/core/plugins/DOMPluginArray.h (right):

https://codereview.chromium.org/189543014/diff/40001/Source/core/plugins/DOMP...
Source/core/plugins/DOMPluginArray.h:38: class DOMPluginArray FINAL : public
RefCountedWillBeGarbageCollectedFinalized<DOMPluginArray>, public
ScriptWrappable, public DOMWindowProperty {
This is the benefit of this. Here is a script wrappable that was not finalized.

https://codereview.chromium.org/189543014/diff/40001/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/189543014/diff/40001/Source/heap/HeapTest.cpp...
Source/heap/HeapTest.cpp:982: namespace WTF {
Why do you need to move this Ian?

[zerny-chromium, 2014-03-11 13:54:12]

Thanks for the reviews everyone!

I'm not finished testing since I'm observing timeouts that do not appear related
to this CL and also because I need proper testing of both modes.

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSCalcu...
File Source/core/css/CSSCalculationValue.h (right):

https://codereview.chromium.org/189543014/diff/40001/Source/core/css/CSSCalcu...
Source/core/css/CSSCalculationValue.h:59: class CSSCalcExpressionNode : public
RefCountedWillBeGarbageCollectedFinalized<CSSCalcExpressionNode> {
On 2014/03/11 13:34:41, Mads Ager (chromium) wrote:
> On 2014/03/11 11:05:25, zerny-chromium wrote:
> > On 2014/03/11 10:26:37, haraken wrote:
> > > 
> > > Why does this class need to be GarbageCollectedFinalized?
> > 
> > Because it has a virtual destructor.
> 
> The virtual destructor is definitely needed when !ENABLE(OILPAN), however,
maybe
> we should ifdef it out for the ENABLE(OILPAN) build and leave this 
> GarbageCollected? As far as I can tell no destructor call is needed here with
> Oilpan.

Will do.

https://codereview.chromium.org/189543014/diff/40001/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/189543014/diff/40001/Source/heap/HeapTest.cpp...
Source/heap/HeapTest.cpp:982: namespace WTF {
On 2014/03/11 13:34:41, Mads Ager (chromium) wrote:
> Why do you need to move this Ian?

The below code uses inlineCapacity for a vector of VectorObject. Since
VectorTraits<VectorObject>::needsDestruction now determines the
VectorDestructionBase it must be known at that point. This is only required for
heap allocated vectors with inline capacity, which really means that the
VectorObject is to be seen as a part object too. So no circularity should be
able to occur in any case. (This is not always the case and the reason we need
the extra base class to delay our inspection of needsDestruction.)

[Mads Ager (chromium), 2014-03-11 15:09:19]

https://codereview.chromium.org/189543014/diff/40001/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/189543014/diff/40001/Source/heap/HeapTest.cpp...
Source/heap/HeapTest.cpp:982: namespace WTF {
On 2014/03/11 13:54:12, zerny-chromium wrote:
> On 2014/03/11 13:34:41, Mads Ager (chromium) wrote:
> > Why do you need to move this Ian?
> 
> The below code uses inlineCapacity for a vector of VectorObject. Since
> VectorTraits<VectorObject>::needsDestruction now determines the
> VectorDestructionBase it must be known at that point. This is only required
for
> heap allocated vectors with inline capacity, which really means that the
> VectorObject is to be seen as a part object too. So no circularity should be
> able to occur in any case. (This is not always the case and the reason we need
> the extra base class to delay our inspection of needsDestruction.)

Ah, yes, thanks for the explanation. :)

[zerny-chromium, 2014-03-12 07:16:34]

The CQ bit was checked by zerny@chromium.org

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/189543014/60001

[commit-bot: I haz the power, 2014-03-12 07:34:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-12 07:34:31]

Try jobs failed on following builders: blink_presubmit

[tkent, 2014-03-12 07:58:27]

On 2014/03/11 13:21:29, haraken wrote:
> tkent-san: What do you think? Are you OK with adding !ENABLE(OILPAN) flags to
> destructors of all GarbageCollected classes?

My impression is that this issue is a defect of tools, and it will force
developers to avoid good practice of C++.
Ideally we should fix clang and/or the plugin.

Also, why don't we just remove empty destructors regardless of ENABLE(OILPAN)?

[zerny-chromium, 2014-03-12 08:22:35]

On 2014/03/12 07:58:27, tkent wrote:
> On 2014/03/11 13:21:29, haraken wrote:
> > tkent-san: What do you think? Are you OK with adding !ENABLE(OILPAN) flags
to
> > destructors of all GarbageCollected classes?
> 
> My impression is that this issue is a defect of tools, and it will force
> developers to avoid good practice of C++.
> Ideally we should fix clang and/or the plugin.

The definition of trivial destructors is in 12.4 paragraph 3 of the C++ standard
and the clang implementation follows that so far as I can see. We can choose not
to use that definition to check proper use of finalized classes, but it really
seems like a good (semantically sound) and natural choice (it is part of the
language standard, ie, not home-backed)

> Also, why don't we just remove empty destructors regardless of ENABLE(OILPAN)?

We will as soon as the type is not in transition anymore. The destructor is
needed in the non-oilpan build due to the Blink implementation of RefPtr, which
has a non-virtual destructor :-/

[zerny-chromium, 2014-03-12 08:24:13]

On 2014/03/12 07:58:27, tkent wrote:
> On 2014/03/11 13:21:29, haraken wrote:
> > tkent-san: What do you think? Are you OK with adding !ENABLE(OILPAN) flags
to
> > destructors of all GarbageCollected classes?

Also, this is not correct. We are only adding !ENABLE(OILPAN) to classes derived
from RefCountedWillBeGarbageCollected<T>.

[tkent, 2014-03-12 08:56:03]

On 2014/03/12 08:22:35, zerny-chromium wrote:
> On 2014/03/12 07:58:27, tkent wrote:
> > On 2014/03/11 13:21:29, haraken wrote:
> > > tkent-san: What do you think? Are you OK with adding !ENABLE(OILPAN) flags
> to
> > > destructors of all GarbageCollected classes?
> > 
> > My impression is that this issue is a defect of tools, and it will force
> > developers to avoid good practice of C++.
> > Ideally we should fix clang and/or the plugin.
> 
> The definition of trivial destructors is in 12.4 paragraph 3 of the C++
standard
> and the clang implementation follows that so far as I can see. We can choose
not
> to use that definition to check proper use of finalized classes, but it really
> seems like a good (semantically sound) and natural choice (it is part of the
> language standard, ie, not home-backed)
> 
> > Also, why don't we just remove empty destructors regardless of
ENABLE(OILPAN)?
> 
> We will as soon as the type is not in transition anymore. The destructor is
> needed in the non-oilpan build due to the Blink implementation of RefPtr,
which
> has a non-virtual destructor :-/

ok, I understand.
In general, I'm afraid Oilpan introduce a lot of coding rules and they decrease
developers productivity. We should avoid to add new rules if possible.

I don't think the latest patch set is acceptable because non-oilpan developers
can't understand why we should not have empty destructors. It's very easy for a
developer to add code to an empty destructor wrapped with !ENABLE(OILPAN).
Probably we should introduce DECLARE_EMPTY_DESTRUCTOR_WILL_BE_NONE(class) /
DEFINE_EMPTY_DESTRUCTOR_WILL_BE_NONE(class) for fail-safe?

[zerny-chromium, 2014-03-12 09:31:03]

> In general, I'm afraid Oilpan introduce a lot of coding rules and they
decrease
> developers productivity. We should avoid to add new rules if possible.

I agree, that rules can make it harder to get work done, but in the case of
memory management, not following them leads to hard-to-debug issues. That is why
we would like the plugin to warn of semantically unsafe issues. 

If a programmer adds a destructor and puts code in there, the code is actually
dead code if the class is not derived from, eg, GarbageCollectedFinalized. Even
harder to find, is the issue of having members that require destruction, eg,
String, RefPtr, OwnPtr, etc. With the current change, the clang compiler will
issue an exact warning that a non-finalized class needs to be finalized, and the
compiler will output a note about why the class must be finalized (user-declared
dtor, base-needs-finalization, or field-needs-finalization).

You can see an example error in this not-yet-finished CL for the plugin:

https://codereview.chromium.org/192933002
https://codereview.chromium.org/192933002/diff/20001/tools/clang/blink_gc_plu...

> I don't think the latest patch set is acceptable because non-oilpan developers
> can't understand why we should not have empty destructors.

But if a developer adds an empty destructor there must be a reason for doing so.
If it is to add a virtual base destructor because sub-classes might need
destruction, then the class *must be* derived from a finalized base class. If it
is, then there is no longer any need to use #if !ENABLE(OILPAN).

Again, the only places we need to #if out a destructor is when using
RefCountedWillBeGarbageCollected. We don't need to do so for
RefCountedWillBeGarbageCollectedFinalized because that base supports
finalization. That is the common case for our transition types. Also, it is
always save to simply add Finalized to RefCountedWillBeGarbageCollected. It is
only if we are absolutely sure we don't need finalization that the other base
type should be used.

> It's very easy for a
> developer to add code to an empty destructor wrapped with !ENABLE(OILPAN).
> Probably we should introduce DECLARE_EMPTY_DESTRUCTOR_WILL_BE_NONE(class) /
> DEFINE_EMPTY_DESTRUCTOR_WILL_BE_NONE(class) for fail-safe?

Apparently the destructor definition has to be in cpp files to avoid some kind
of include cycles. I can add:

DECLARE_EMPTY_DESTRUCTOR_WILL_BE_NONE(class) and
DEFINE_EMPTY_DESTRUCTOR_WILL_BY_NONE(class)

if you like?

[tkent, 2014-03-12 14:01:57]

On 2014/03/12 09:31:03, zerny-chromium wrote:
> I agree, that rules can make it harder to get work done, but in the case of
> memory management, not following them leads to hard-to-debug issues. That is
why
> we would like the plugin to warn of semantically unsafe issues. 
> 
> If a programmer adds a destructor and puts code in there, the code is actually
> dead code if the class is not derived from, eg, GarbageCollectedFinalized.
Even
> harder to find, is the issue of having members that require destruction, eg,
> String, RefPtr, OwnPtr, etc. With the current change, the clang compiler will
> issue an exact warning that a non-finalized class needs to be finalized, and
the
> compiler will output a note about why the class must be finalized
(user-declared
> dtor, base-needs-finalization, or field-needs-finalization).

I understand the importance of the finalization check by the plugin. I wonder if
we can avoid to remove empty destructors by changing clang plugin interfaces so
that it provides what we need.

> Apparently the destructor definition has to be in cpp files to avoid some kind
> of include cycles. I can add:
> 
> DECLARE_EMPTY_DESTRUCTOR_WILL_BE_NONE(class) and
> DEFINE_EMPTY_DESTRUCTOR_WILL_BY_NONE(class)
> 
> if you like?

Please go ahead.

[zerny-chromium, 2014-03-12 14:09:56]

> I understand the importance of the finalization check by the plugin. I wonder
if
> we can avoid to remove empty destructors by changing clang plugin interfaces
so
> that it provides what we need.

I don't see any direct way to get the same guarantees as the current
implementation does.

> > Apparently the destructor definition has to be in cpp files to avoid some
kind
> > of include cycles. I can add:
> > 
> > DECLARE_EMPTY_DESTRUCTOR_WILL_BE_NONE(class) and
> > DEFINE_EMPTY_DESTRUCTOR_WILL_BY_NONE(class)
> > 
> > if you like?
> 
> Please go ahead.

Done.

[zerny-chromium, 2014-03-12 16:18:33]

Added update for mixin checking.

[tkent, 2014-03-13 01:12:57]

lgtm

https://codereview.chromium.org/189543014/diff/110001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/189543014/diff/110001/Source/heap/Handle.h#ne...
Source/heap/Handle.h:687: #define DEFINE_EMPTY_DESTRUCTOR_WILL_BY_NONE(type) //
do nothing
WILL_BY -> WILL_BE

[haraken, 2014-03-13 01:16:04]

On 2014/03/13 01:12:57, tkent wrote:
> lgtm
> 
> https://codereview.chromium.org/189543014/diff/110001/Source/heap/Handle.h
> File Source/heap/Handle.h (right):
> 
>
https://codereview.chromium.org/189543014/diff/110001/Source/heap/Handle.h#ne...
> Source/heap/Handle.h:687: #define DEFINE_EMPTY_DESTRUCTOR_WILL_BY_NONE(type)
//
> do nothing
> WILL_BY -> WILL_BE

Nit: DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED ? (c.f.,
WTF_FAST_ALLOCATED_WILL_BE_REMOVED)

[haraken, 2014-03-13 01:17:10]

LGTM

[zerny-chromium, 2014-03-13 06:44:38]

On 2014/03/13 01:16:04, haraken wrote:
> On 2014/03/13 01:12:57, tkent wrote:
> > lgtm
> > 
> > https://codereview.chromium.org/189543014/diff/110001/Source/heap/Handle.h
> > File Source/heap/Handle.h (right):
> > 
> >
>
https://codereview.chromium.org/189543014/diff/110001/Source/heap/Handle.h#ne...
> > Source/heap/Handle.h:687: #define DEFINE_EMPTY_DESTRUCTOR_WILL_BY_NONE(type)
> //
> > do nothing
> > WILL_BY -> WILL_BE
> 
> Nit: DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED ? (c.f.,
> WTF_FAST_ALLOCATED_WILL_BE_REMOVED)

Yes. I'll change the names.

[zerny-chromium, 2014-03-13 06:44:54]

https://codereview.chromium.org/189543014/diff/110001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/189543014/diff/110001/Source/heap/Handle.h#ne...
Source/heap/Handle.h:687: #define DEFINE_EMPTY_DESTRUCTOR_WILL_BY_NONE(type) //
do nothing
On 2014/03/13 01:12:58, tkent wrote:
> WILL_BY -> WILL_BE

Oh. Thanks for spotting that!

[zerny-chromium, 2014-03-13 09:33:20]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-13 09:33:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/189543014/110033

[commit-bot: I haz the power, 2014-03-13 10:36:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 10:36:06]

Try jobs failed on following builders:
  tryserver.chromium on linux_blink

[zerny-chromium, 2014-03-13 10:39:08]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-13 10:39:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/189543014/110033

[commit-bot: I haz the power, 2014-03-13 11:45:49]

Change committed as 169117