Oilpan: IDL union objects should be put in HeapVector

Oilpan: IDL union objects should be put in HeapVector

Since IDL union objects are ALLOW_ONLY_INLINE_ALLOCATION,
they need to be put in HeapVector, not Vector.

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

[haraken, 2015-05-01 14:55:35]

haraken@chromium.org changed reviewers:
+ bashi@chromium.org, oilpan-reviews@chromium.org, yutak@chromium.org

[haraken, 2015-05-01 14:55:36]

PTAL

yutak@: We might want to improve the plugin so that it can catch this kind of
error -- it is wrong to put ALLOW_ONLY_INLINE_ALLOCATION objects that need
tracing in (off-heap) Vectors.

[haraken, 2015-05-01 14:58:13]

https://codereview.chromium.org/1118993002/diff/1/Source/bindings/scripts/v8_...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/1118993002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_types.py:313: if idl_type.is_garbage_collected or
idl_type.is_dictionary or idl_type.is_union_type:

bashi-san@: Maybe would it make more sense to set is_garbage_collected for
dictionaries and union types?

https://codereview.chromium.org/1118993002/diff/1/Source/bindings/templates/u...
File Source/bindings/templates/union.h (right):

https://codereview.chromium.org/1118993002/diff/1/Source/bindings/templates/u...
Source/bindings/templates/union.h:93:
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(blink::{{container.cpp_class}});

bashi-san@: Is it correct to set canInitializeWithMemset=true for IDL union
objects?

[Jens Widell, 2015-05-04 14:02:18]

jl@opera.com changed reviewers:
+ jl@opera.com

[Jens Widell, 2015-05-04 14:02:18]

https://codereview.chromium.org/1118993002/diff/1/Source/bindings/templates/u...
File Source/bindings/templates/union.h (right):

https://codereview.chromium.org/1118993002/diff/1/Source/bindings/templates/u...
Source/bindings/templates/union.h:93:
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(blink::{{container.cpp_class}});
On 2015/05/01 14:58:13, haraken wrote:
> 
> bashi-san@: Is it correct to set canInitializeWithMemset=true for IDL union
> objects?

I'm guessing it would work in practice, but it's technically wrong in some
cases: Dictionary, generated dictionary types, and Nullable<>.

Dictionary has a default constructor that sets m_exceptionState to a non-null
pointer value. While we do assert that m_exceptionState isn't null in some
member functions, those asserts can only be reached if the dictionary wraps an
object value, which it wouldn't (the m_options handle would be empty). And
besides, the Dictionary member in the union wouldn't be used either, since the
union would have no value set, so none of the member functions of the Dictionary
object should be called (except the destructor and assignment operator, which
are both fine).

Generated dictionary types have default constructors that can set various
default member values, but again, as a member in an empty union, such an
incorrectly initialized object shouldn't be used. The member types are the same
as in unions, so this discussion applies recursively.

Nullable<> has a default constructor that sets m_isNull to true. Same here
though, this shouldn't matter since such a union member shouldn't be used.

Other typical union members are primitives, various pointer types (RefPtr,
Member, String) that are definitely safe, and  Vector<>/HeapVector<>. Vectors
look safe to me, but I don't know if they're actually declared to have
canInitializeWithMemset=true.

So, in summary: since memset()-initialized union members shouldn't be accessed,
only their destructors and assignment operators really matter, and those all
look safe to me.

[haraken, 2015-05-04 22:27:26]

On 2015/05/04 14:02:18, Jens Widell wrote:
>
https://codereview.chromium.org/1118993002/diff/1/Source/bindings/templates/u...
> File Source/bindings/templates/union.h (right):
> 
>
https://codereview.chromium.org/1118993002/diff/1/Source/bindings/templates/u...
> Source/bindings/templates/union.h:93:
> WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(blink::{{container.cpp_class}});
> On 2015/05/01 14:58:13, haraken wrote:
> > 
> > bashi-san@: Is it correct to set canInitializeWithMemset=true for IDL union
> > objects?
> 
> I'm guessing it would work in practice, but it's technically wrong in some
> cases: Dictionary, generated dictionary types, and Nullable<>.
> 
> Dictionary has a default constructor that sets m_exceptionState to a non-null
> pointer value. While we do assert that m_exceptionState isn't null in some
> member functions, those asserts can only be reached if the dictionary wraps an
> object value, which it wouldn't (the m_options handle would be empty). And
> besides, the Dictionary member in the union wouldn't be used either, since the
> union would have no value set, so none of the member functions of the
Dictionary
> object should be called (except the destructor and assignment operator, which
> are both fine).
> 
> Generated dictionary types have default constructors that can set various
> default member values, but again, as a member in an empty union, such an
> incorrectly initialized object shouldn't be used. The member types are the
same
> as in unions, so this discussion applies recursively.
> 
> Nullable<> has a default constructor that sets m_isNull to true. Same here
> though, this shouldn't matter since such a union member shouldn't be used.
> 
> Other typical union members are primitives, various pointer types (RefPtr,
> Member, String) that are definitely safe, and  Vector<>/HeapVector<>. Vectors
> look safe to me, but I don't know if they're actually declared to have
> canInitializeWithMemset=true.
> 
> So, in summary: since memset()-initialized union members shouldn't be
accessed,
> only their destructors and assignment operators really matter, and those all
> look safe to me.

Thanks for the detailed write-up. Agreed with your point:
canInitializeWithMemset=true will work in practice but is technically wrong
(i.e., fragile). Let me think about a safer way to do this.

(Unfortunately, HeapVector can support items that can be initialized with memset
or have a vtable. Another approach would be to add a vtable to the union type,
but it would be nasty as well...)

[Yuta Kitamura, 2015-05-07 05:20:28]

haraken, re: Blink plugin: sounds plausible, can you file
a bug and assign it to me?

[bashi, 2015-05-07 23:59:24]

https://codereview.chromium.org/1118993002/diff/1/Source/bindings/scripts/v8_...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/1118993002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_types.py:313: if idl_type.is_garbage_collected or
idl_type.is_dictionary or idl_type.is_union_type:
On 2015/05/01 14:58:13, haraken wrote:
> 
> bashi-san@: Maybe would it make more sense to set is_garbage_collected for
> dictionaries and union types?

Probably, but we might want to seek a better naming.

https://codereview.chromium.org/1118993002/diff/1/Source/bindings/templates/u...
File Source/bindings/templates/union.h (right):

https://codereview.chromium.org/1118993002/diff/1/Source/bindings/templates/u...
Source/bindings/templates/union.h:93:
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(blink::{{container.cpp_class}});
On 2015/05/04 14:02:18, Jens Widell wrote:
> On 2015/05/01 14:58:13, haraken wrote:
> > 
> > bashi-san@: Is it correct to set canInitializeWithMemset=true for IDL union
> > objects?
> 
> I'm guessing it would work in practice, but it's technically wrong in some
> cases: Dictionary, generated dictionary types, and Nullable<>.
> 
> Dictionary has a default constructor that sets m_exceptionState to a non-null
> pointer value. While we do assert that m_exceptionState isn't null in some
> member functions, those asserts can only be reached if the dictionary wraps an
> object value, which it wouldn't (the m_options handle would be empty). And
> besides, the Dictionary member in the union wouldn't be used either, since the
> union would have no value set, so none of the member functions of the
Dictionary
> object should be called (except the destructor and assignment operator, which
> are both fine).
> 
> Generated dictionary types have default constructors that can set various
> default member values, but again, as a member in an empty union, such an
> incorrectly initialized object shouldn't be used. The member types are the
same
> as in unions, so this discussion applies recursively.
> 
> Nullable<> has a default constructor that sets m_isNull to true. Same here
> though, this shouldn't matter since such a union member shouldn't be used.
> 
> Other typical union members are primitives, various pointer types (RefPtr,
> Member, String) that are definitely safe, and  Vector<>/HeapVector<>. Vectors
> look safe to me, but I don't know if they're actually declared to have
> canInitializeWithMemset=true.
> 
> So, in summary: since memset()-initialized union members shouldn't be
accessed,
> only their destructors and assignment operators really matter, and those all
> look safe to me.

Thanks Jens for detailed explanation! Given above, I think it would work in
practice. I'd be happy if there is another workaround, but I think this is an
acceptable approach.

[haraken, 2015-05-08 00:06:09]

On 2015/05/07 23:59:24, bashi1(ooo til May 12) wrote:
>
https://codereview.chromium.org/1118993002/diff/1/Source/bindings/scripts/v8_...
> File Source/bindings/scripts/v8_types.py (right):
> 
>
https://codereview.chromium.org/1118993002/diff/1/Source/bindings/scripts/v8_...
> Source/bindings/scripts/v8_types.py:313: if idl_type.is_garbage_collected or
> idl_type.is_dictionary or idl_type.is_union_type:
> On 2015/05/01 14:58:13, haraken wrote:
> > 
> > bashi-san@: Maybe would it make more sense to set is_garbage_collected for
> > dictionaries and union types?
> 
> Probably, but we might want to seek a better naming.
> 
>
https://codereview.chromium.org/1118993002/diff/1/Source/bindings/templates/u...
> File Source/bindings/templates/union.h (right):
> 
>
https://codereview.chromium.org/1118993002/diff/1/Source/bindings/templates/u...
> Source/bindings/templates/union.h:93:
> WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(blink::{{container.cpp_class}});
> On 2015/05/04 14:02:18, Jens Widell wrote:
> > On 2015/05/01 14:58:13, haraken wrote:
> > > 
> > > bashi-san@: Is it correct to set canInitializeWithMemset=true for IDL
union
> > > objects?
> > 
> > I'm guessing it would work in practice, but it's technically wrong in some
> > cases: Dictionary, generated dictionary types, and Nullable<>.
> > 
> > Dictionary has a default constructor that sets m_exceptionState to a
non-null
> > pointer value. While we do assert that m_exceptionState isn't null in some
> > member functions, those asserts can only be reached if the dictionary wraps
an
> > object value, which it wouldn't (the m_options handle would be empty). And
> > besides, the Dictionary member in the union wouldn't be used either, since
the
> > union would have no value set, so none of the member functions of the
> Dictionary
> > object should be called (except the destructor and assignment operator,
which
> > are both fine).
> > 
> > Generated dictionary types have default constructors that can set various
> > default member values, but again, as a member in an empty union, such an
> > incorrectly initialized object shouldn't be used. The member types are the
> same
> > as in unions, so this discussion applies recursively.
> > 
> > Nullable<> has a default constructor that sets m_isNull to true. Same here
> > though, this shouldn't matter since such a union member shouldn't be used.
> > 
> > Other typical union members are primitives, various pointer types (RefPtr,
> > Member, String) that are definitely safe, and  Vector<>/HeapVector<>.
Vectors
> > look safe to me, but I don't know if they're actually declared to have
> > canInitializeWithMemset=true.
> > 
> > So, in summary: since memset()-initialized union members shouldn't be
> accessed,
> > only their destructors and assignment operators really matter, and those all
> > look safe to me.
> 
> Thanks Jens for detailed explanation! Given above, I think it would work in
> practice. I'd be happy if there is another workaround, but I think this is an
> acceptable approach.

Thanks, then I might want to go with this approach at the moment. PTAL?

[bashi, 2015-05-08 00:11:20]

LGTM.

Could you add a comment to describe why we use
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS() and why it is safe?

[haraken, 2015-05-08 01:02:09]

> Could you add a comment to describe why we use
> WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS() and why it is safe?

Done.

[haraken, 2015-05-08 01:03:17]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-05-08 01:03:18]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/1118993002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-05-08 01:04:07]

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

[commit-bot: I haz the power, 2015-05-08 05:43:19]

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