Oilpan: Change Persistent<> data members to Member<> in HitTestResult.

Oilpan: Change Persistent<> data members to Member<> in HitTestResult.

- Disallow new/delete operations for HitTestResult.
  HitTestResult was used
  -- with new/delete operators
  -- as part objects of ContextMenuController and MouseEventWithHitTestResult.
  -- as stack allocated objects

  We'd like to avoid to make such classes in Oilpan.
  So, this CL disallows new/delete, and WebHitTestResult owns another
  class to hold only required data copied from a HitTestResult.

- Move ContextMenuController to Oilpan heap, and trace HitTestResult member.

- Mark MouseEventWithHitTestResults STACK_ALLOCATED.

- We can't get HitTestResult from a WebHitTestResult. So we need to
  introduce new function to WebViewImpl for WebSubstringUtil.mm.


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

[tkent, 2014-06-17 23:40:15]

Please review this.

[haraken, 2014-06-18 01:27:25]

LGTM

Does the plugin detect the error if we use "an on-heap object that does not have
DISALLOW_ALLOCATION" as a part of object?

https://codereview.chromium.org/326393003/diff/1/Source/core/page/ContextMenu...
File Source/core/page/ContextMenuController.h (right):

https://codereview.chromium.org/326393003/diff/1/Source/core/page/ContextMenu...
Source/core/page/ContextMenuController.h:50: void trace(Visitor* visitor) {
visitor->trace(m_hitTestResult); }

We normally write a non-empty trace method in a cpp file.

https://codereview.chromium.org/326393003/diff/1/Source/web/WebHitTestResult.cpp
File Source/web/WebHitTestResult.cpp (right):

https://codereview.chromium.org/326393003/diff/1/Source/web/WebHitTestResult....
Source/web/WebHitTestResult.cpp:125: if (info.m_private.get())

You can use info.isNull().

[tkent, 2014-06-18 03:10:40]

Thank you for reviewing.

> Does the plugin detect the error if we use "an on-heap object that does not
have
> DISALLOW_ALLOCATION" as a part of object?

No.

https://codereview.chromium.org/326393003/diff/1/Source/core/page/ContextMenu...
File Source/core/page/ContextMenuController.h (right):

https://codereview.chromium.org/326393003/diff/1/Source/core/page/ContextMenu...
Source/core/page/ContextMenuController.h:50: void trace(Visitor* visitor) {
visitor->trace(m_hitTestResult); }
On 2014/06/18 01:27:24, haraken wrote:
> 
> We normally write a non-empty trace method in a cpp file.

Done.

https://codereview.chromium.org/326393003/diff/1/Source/web/WebHitTestResult.cpp
File Source/web/WebHitTestResult.cpp (right):

https://codereview.chromium.org/326393003/diff/1/Source/web/WebHitTestResult....
Source/web/WebHitTestResult.cpp:125: if (info.m_private.get())
On 2014/06/18 01:27:25, haraken wrote:
> 
> You can use info.isNull().

Done.

[tkent, 2014-06-18 03:10:43]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-18 03:10:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/326393003/20001

[zerny-chromium, 2014-06-18 05:24:39]

On 2014/06/18 03:10:40, tkent wrote:
> Thank you for reviewing.
> 
> > Does the plugin detect the error if we use "an on-heap object that does not
> have
> > DISALLOW_ALLOCATION" as a part of object?
> 
> No.

The plugin does not do any validation wrt DISALLOW_ALLOCATION in particular.
However, if the object is GC-allocated (ie, it has a non-mixin GC base) and used
as a part object then the plugin will issue an error.

If it is not GC allocated then the plugin will require it traces its fields
except if it is marked as STACK_ALLOCATED.

Do you see a way to strengthen this? We could consider adding a check that
DISALLOW_ALLOCATION is used in the latter case. However, we have previously used
an off-heap non-part object (typically held via an OwnPtr) which had traced
members. That would be an exception to such a check.

[haraken, 2014-06-18 05:29:14]

On 2014/06/18 05:24:39, zerny-chromium wrote:
> On 2014/06/18 03:10:40, tkent wrote:
> > Thank you for reviewing.
> > 
> > > Does the plugin detect the error if we use "an on-heap object that does
not
> > have
> > > DISALLOW_ALLOCATION" as a part of object?
> > 
> > No.
> 
> The plugin does not do any validation wrt DISALLOW_ALLOCATION in particular.
> However, if the object is GC-allocated (ie, it has a non-mixin GC base) and
used
> as a part object then the plugin will issue an error.

What I'm asking is whether the plugin are doing the verification you mentioned
above ^^^. Specifically, if we make the HitTestResult a GarbageCollected object,
can the plugin complain about it (because HitTestResult is used as a part of
object)? tkent-san is saying no (probably because my question was not clear).

[zerny-chromium, 2014-06-18 05:32:57]

On 2014/06/18 05:29:14, haraken wrote:
> On 2014/06/18 05:24:39, zerny-chromium wrote:
> > On 2014/06/18 03:10:40, tkent wrote:
> > > Thank you for reviewing.
> > > 
> > > > Does the plugin detect the error if we use "an on-heap object that does
> not
> > > have
> > > > DISALLOW_ALLOCATION" as a part of object?
> > > 
> > > No.
> > 
> > The plugin does not do any validation wrt DISALLOW_ALLOCATION in particular.
> > However, if the object is GC-allocated (ie, it has a non-mixin GC base) and
> used
> > as a part object then the plugin will issue an error.
> 
> What I'm asking is whether the plugin are doing the verification you mentioned
> above ^^^. Specifically, if we make the HitTestResult a GarbageCollected
object,
> can the plugin complain about it (because HitTestResult is used as a part of
> object)? tkent-san is saying no (probably because my question was not clear).

For the case I described the plugin will issue an error. If not, then that is a
fault of the plugin. Was this the situation you had tkent? Or was it the case
that HitTestResult was still off-heap allocated but did not have a GC base?

[zerny-chromium, 2014-06-18 05:33:05]

lgtm

https://codereview.chromium.org/326393003/diff/20001/Source/core/page/Context...
File Source/core/page/ContextMenuController.h (right):

https://codereview.chromium.org/326393003/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuController.h:45: class ContextMenuController :
public NoBaseWillBeGarbageCollectedFinalized<ContextMenuController> {
Nit: FINAL

https://codereview.chromium.org/326393003/diff/20001/Source/core/rendering/Hi...
File Source/core/rendering/HitTestResult.h (right):

https://codereview.chromium.org/326393003/diff/20001/Source/core/rendering/Hi...
Source/core/rendering/HitTestResult.h:48: class HitTestResult {
Nit: FINAL

[tkent, 2014-06-18 06:02:44]

On 2014/06/18 05:32:57, zerny-chromium wrote:
> On 2014/06/18 05:29:14, haraken wrote:
> > On 2014/06/18 05:24:39, zerny-chromium wrote:
> > > On 2014/06/18 03:10:40, tkent wrote:
> > > > Thank you for reviewing.
> > > > 
> > > > > Does the plugin detect the error if we use "an on-heap object that
does
> > not
> > > > have
> > > > > DISALLOW_ALLOCATION" as a part of object?
> > > > 
> > > > No.
> > > 
> > > The plugin does not do any validation wrt DISALLOW_ALLOCATION in
particular.
> > > However, if the object is GC-allocated (ie, it has a non-mixin GC base)
and
> > used
> > > as a part object then the plugin will issue an error.
> > 
> > What I'm asking is whether the plugin are doing the verification you
mentioned
> > above ^^^. Specifically, if we make the HitTestResult a GarbageCollected
> object,
> > can the plugin complain about it (because HitTestResult is used as a part of
> > object)? tkent-san is saying no (probably because my question was not
clear).
> 
> For the case I described the plugin will issue an error. If not, then that is
a
> fault of the plugin. Was this the situation you had tkent? Or was it the case
> that HitTestResult was still off-heap allocated but did not have a GC base?

What we want is that the plugin complains about the following code.  This is
compilable now though this generates an assertion failure at runtime.

diff --git a/Source/platform/heap/HeapTest.cpp
b/Source/platform/heap/HeapTest.cpp
index 567c048..2e7ff41 100644
--- a/Source/platform/heap/HeapTest.cpp
+++ b/Source/platform/heap/HeapTest.cpp
@@ -4344,6 +4344,21 @@ TEST(HeapTest, RemoveEmptySets)

     Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
     EXPECT_EQ(0u, map->size());
+
+}
+
+class HasHeapAllocatedArray {
+public:
+    HeapAllocatedArray* array() { return &m_array; }
+
+private:
+    HeapAllocatedArray m_array;
+};
+
+TEST(HeapTest, PartObjectPointer)
+{
+    HasHeapAllocatedArray hhaa;
+    Persistent<HeapAllocatedArray> persistent = hhaa.array();
 }

 } // WebCore namespace

[commit-bot: I haz the power, 2014-06-18 06:16:13]

Change committed as 176393

[zerny-chromium, 2014-06-18 06:26:24]

On 2014/06/18 06:02:44, tkent wrote:
> On 2014/06/18 05:32:57, zerny-chromium wrote:
> > On 2014/06/18 05:29:14, haraken wrote:
> > > On 2014/06/18 05:24:39, zerny-chromium wrote:
> > > > On 2014/06/18 03:10:40, tkent wrote:
> > > > > Thank you for reviewing.
> > > > > 
> > > > > > Does the plugin detect the error if we use "an on-heap object that
> does
> > > not
> > > > > have
> > > > > > DISALLOW_ALLOCATION" as a part of object?
> > > > > 
> > > > > No.
> > > > 
> > > > The plugin does not do any validation wrt DISALLOW_ALLOCATION in
> particular.
> > > > However, if the object is GC-allocated (ie, it has a non-mixin GC base)
> and
> > > used
> > > > as a part object then the plugin will issue an error.
> > > 
> > > What I'm asking is whether the plugin are doing the verification you
> mentioned
> > > above ^^^. Specifically, if we make the HitTestResult a GarbageCollected
> > object,
> > > can the plugin complain about it (because HitTestResult is used as a part
of
> > > object)? tkent-san is saying no (probably because my question was not
> clear).
> > 
> > For the case I described the plugin will issue an error. If not, then that
is
> a
> > fault of the plugin. Was this the situation you had tkent? Or was it the
case
> > that HitTestResult was still off-heap allocated but did not have a GC base?
> 
> What we want is that the plugin complains about the following code.  This is
> compilable now though this generates an assertion failure at runtime.
> 
> diff --git a/Source/platform/heap/HeapTest.cpp
> b/Source/platform/heap/HeapTest.cpp
> index 567c048..2e7ff41 100644
> --- a/Source/platform/heap/HeapTest.cpp
> +++ b/Source/platform/heap/HeapTest.cpp
> @@ -4344,6 +4344,21 @@ TEST(HeapTest, RemoveEmptySets)
> 
>      Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
>      EXPECT_EQ(0u, map->size());
> +
> +}
> +
> +class HasHeapAllocatedArray {
> +public:
> +    HeapAllocatedArray* array() { return &m_array; }
> +
> +private:
> +    HeapAllocatedArray m_array;
> +};
> +
> +TEST(HeapTest, PartObjectPointer)
> +{
> +    HasHeapAllocatedArray hhaa;
> +    Persistent<HeapAllocatedArray> persistent = hhaa.array();
>  }
> 
>  } // WebCore namespace

Yes. The same rule should apply to GC allocated collections too. Unfortunately
the use of part-object allocated collections is something we do allow, and the
plugin can't check that we don't take a pointer to such a part-object allocated
collection (since that is not part of the declaration structure that we
statically check).

We have discussed this locally and are not clear how to fix this for
collections. Once idea would be to distinguish the two allocation kinds by
distinct types and prohibit interpreting a part-allocated collection as a
heap-allocated collection. Another, is to effectively make the part-allocated
collections a "GC Mixin", ie, have an adjustAndMark that would keep the owner
object alive.

(As an aside, your example should give an error about m_array not being traced
in HasHeapAllocatedArray, but that is not the point you are trying to make
here.)

[tkent, 2014-06-18 06:36:08]

On 2014/06/18 06:26:24, zerny-chromium wrote:
> Yes. The same rule should apply to GC allocated collections too. Unfortunately
> the use of part-object allocated collections is something we do allow, and the
> plugin can't check that we don't take a pointer to such a part-object
allocated
> collection (since that is not part of the declaration structure that we
> statically check).
> 
> We have discussed this locally and are not clear how to fix this for
> collections. Once idea would be to distinguish the two allocation kinds by
> distinct types and prohibit interpreting a part-allocated collection as a
> heap-allocated collection. Another, is to effectively make the part-allocated
> collections a "GC Mixin", ie, have an adjustAndMark that would keep the owner
> object alive.

I understand.  Thanks.
This is not a serious issue because we have runtime assertions.

[sof, 2014-06-18 06:36:32]

https://codereview.chromium.org/326393003/diff/20001/Source/web/WebHitTestRes...
File Source/web/WebHitTestResult.cpp (right):

https://codereview.chromium.org/326393003/diff/20001/Source/web/WebHitTestRes...
Source/web/WebHitTestResult.cpp:44: class WebHitTestResultPrivate {
I think it would be better to have a WebPrivateOwnPtr that could hold a
Persistent.

[tkent, 2014-07-23 03:21:13]

I'll revert this change because this caused a performance regression about
WebHitTestResult creation.