Oilpan: move Range object to the oilpan heap.

Oilpan: move Range object to the oilpan heap.

R=tkent@chromium.org,haraken@chromium.org,yutak@chromium.org,ager@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170845

[sof, 2014-04-03 20:28:48]

Please take a look.

Some points worth mentioning:

 - Ran into some SpellChecker code that depended upon RefPtr<T> being
zero-initialized, whereas RawPtr<T> is not. Explicitly initialized all (with
nullptr) to address; slightly worrisome.
 - Some editing/ helper classes have been constrained to being stack allocated,
so as to allow their GCed pointers to be kept as members. Please check that I'm
using the right macro for this (DISALLOW_ALLOCATION()) - there has been 
 - WebRange has migrated over to use WebPrivatePtr.
 - Verified to build ok with the clang GC plugin (ToT)


 -

[tkent, 2014-04-04 01:56:55]

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
File Source/core/editing/Editor.cpp (left):

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
Source/core/editing/Editor.cpp:787: RefPtr<Range> range =
selection.toNormalizedRange();
I wonder if we can remove it.

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Text...
File Source/core/editing/TextCheckingHelper.h (right):

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Text...
Source/core/editing/TextCheckingHelper.h:39: DISALLOW_ALLOCATION();
probably STACK_ALLOCATED() is correct, and trace() is unnecessary.

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Text...
Source/core/editing/TextCheckingHelper.h:85: DISALLOW_ALLOCATION();
Ditto.

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Text...
File Source/core/editing/TextIterator.h (right):

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Text...
Source/core/editing/TextIterator.h:347: DISALLOW_ALLOCATION();
Ditto.

[tkent, 2014-04-04 01:58:43]

+yutak FYI

[haraken, 2014-04-04 02:30:18]

LGTM if tkent-san's comments are addressed.

https://codereview.chromium.org/224113002/diff/40001/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/224113002/diff/40001/Source/core/dom/Range.h#...
Source/core/dom/Range.h:174: RefPtr<Document> m_ownerDocument; // Cannot be
null.

Since Range objects reside in an editing undo stack, this RefPtr can be the last
reference to the Document. This means that moving Range objects to oilpan's heap
can change the timing when the Document is destructed. This might cause some
issues, so let's just keep that in mind :)

https://codereview.chromium.org/224113002/diff/40001/Source/web/TextFinder.h
File Source/web/TextFinder.h (right):

https://codereview.chromium.org/224113002/diff/40001/Source/web/TextFinder.h#...
Source/web/TextFinder.h:273: static const bool canInitializeWithMemset = true;

I think you can also specify "canMoveWithMemcpy = true", but I'm OK with not
specifying canMoveWithMemcpy because it's just for performance optimization.

If we add a String to TextFinder::FindMatch but forget to remove
canMoveWithMemcpy in the future, we will produce hard-to-find bugs. Given the
risk, I'm OK with not specifying canMoveWithMemcpy for not-performance-sensitive
objects.

https://codereview.chromium.org/224113002/diff/40001/public/web/WebRange.h
File public/web/WebRange.h (right):

https://codereview.chromium.org/224113002/diff/40001/public/web/WebRange.h#ne...
public/web/WebRange.h:63: bool isNull() const { return m_private.isNull(); }

If we forget to change this, is there any way to notice the error?

[Yuta Kitamura, 2014-04-04 02:37:24]

LGTM but please wait for Kent's OK before landing.

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
File Source/core/editing/Editor.cpp (left):

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
Source/core/editing/Editor.cpp:787: RefPtr<Range> range =
selection.toNormalizedRange();
On 2014/04/04 01:56:56, tkent wrote:
> I wonder if we can remove it.

It's probably OK to completely remove this line.

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
File Source/core/editing/EditorCommand.cpp (right):

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
Source/core/editing/EditorCommand.cpp:221: RefPtrWillBeRawPtr<Range>
ALLOW_UNUSED oldRange = frame.selection().selection().toNormalizedRange();
This line can be removed, as well.

[sof, 2014-04-04 05:32:02]

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
File Source/core/editing/Editor.cpp (left):

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
Source/core/editing/Editor.cpp:787: RefPtr<Range> range =
selection.toNormalizedRange();
On 2014/04/04 02:37:25, Yuta Kitamura wrote:
> On 2014/04/04 01:56:56, tkent wrote:
> > I wonder if we can remove it.
> 
> It's probably OK to completely remove this line.

Great, will do that - it wasn't clear if it had a required effect on editing
state or not.

[Mads Ager (chromium), 2014-04-04 06:15:06]

Drive-by comments inline.

On 2014/04/03 20:28:48, sof wrote:
> Please take a look.
> 
> Some points worth mentioning:
> 
>  - Ran into some SpellChecker code that depended upon RefPtr<T> being
> zero-initialized, whereas RawPtr<T> is not. Explicitly initialized all (with
> nullptr) to address; slightly worrisome.

This was a change we made on purpose. The end result will need explicit
initialization and therefore we would like to catch it as we go instead of
having to fight this whenever we want to ship something without transition
types.

RefPtrWillBeRawPtr<T> a;

Will turn into:

T* a;

Which is uninitialized. Whenever we have a RawPtr we need to really think of
them as raw pointers (including when it comes to initialization). Therefore, the
requirement to write:

RefPtrWillBeRawPtr<T> a = nullptr;


>  - Some editing/ helper classes have been constrained to being stack
allocated,
> so as to allow their GCed pointers to be kept as members. Please check that
I'm
> using the right macro for this (DISALLOW_ALLOCATION()) - there has been 

For that, please use STACK_ALLOCATED() and just have the members in there.

STACK_ALLOCATED: The object is only stack allocated. Heap objects should be in
Members but you do not need the trace method as they are on the stack. (Down the
line these might turn in to raw pointers, but for now Members indicates that we
have thought about them and explicitly taken care of them.)

DISALLOW_ALLOCATION: Cannot be allocated with new operators but can be a part
object. If it has Members you need a trace method and the containing object
needs to call that trace method.

ALLOW_ONLY_INLINE_ALLOCATION: Allows only placement new operator. This disallows
general allocation of this object but allows to put the object as a value object
in collections. If these have Members you need to have a trace method. That
trace method will be called automatically by the Heap collections.

>  - WebRange has migrated over to use WebPrivatePtr.
>  - Verified to build ok with the clang GC plugin (ToT)
> 
> 
>  -

[haraken, 2014-04-04 06:23:20]

> STACK_ALLOCATED: The object is only stack allocated. Heap objects should be in
> Members but you do not need the trace method as they are on the stack. (Down
the
> line these might turn in to raw pointers, but for now Members indicates that
we
> have thought about them and explicitly taken care of them.)
> 
> DISALLOW_ALLOCATION: Cannot be allocated with new operators but can be a part
> object. If it has Members you need a trace method and the containing object
> needs to call that trace method.
> 
> ALLOW_ONLY_INLINE_ALLOCATION: Allows only placement new operator. This
disallows
> general allocation of this object but allows to put the object as a value
object
> in collections. If these have Members you need to have a trace method. That
> trace method will be called automatically by the Heap collections.

- STACK_ALLOCATED
- DISALLOW_ALLOCATION
- DISALLOW_ALLOCATION_OUTSIDE_COLLECTIONS

or

- STACK_ALLOCATED
- DISALLOW_NEW
- DISALLOW_NEW_OUTSIDE_COLLECTIONS

might be a slightly better name?

[Mads Ager (chromium), 2014-04-04 06:24:55]

On Fri, Apr 4, 2014 at 8:23 AM, <haraken@chromium.org> wrote:

> STACK_ALLOCATED: The object is only stack allocated. Heap objects should
>> be in
>> Members but you do not need the trace method as they are on the stack.
>> (Down
>>
> the
>
>> line these might turn in to raw pointers, but for now Members indicates
>> that
>>
> we
>
>> have thought about them and explicitly taken care of them.)
>>
>
>  DISALLOW_ALLOCATION: Cannot be allocated with new operators but can be a
>> part
>> object. If it has Members you need a trace method and the containing
>> object
>> needs to call that trace method.
>>
>
>  ALLOW_ONLY_INLINE_ALLOCATION: Allows only placement new operator. This
>>
> disallows
>
>> general allocation of this object but allows to put the object as a value
>>
> object
>
>> in collections. If these have Members you need to have a trace method.
>> That
>> trace method will be called automatically by the Heap collections.
>>
>
> - STACK_ALLOCATED
> - DISALLOW_ALLOCATION
> - DISALLOW_ALLOCATION_OUTSIDE_COLLECTIONS
>
> or
>
> - STACK_ALLOCATED
> - DISALLOW_NEW
> - DISALLOW_NEW_OUTSIDE_COLLECTIONS
>
> might be a slightly better name?
>

I personally like the existing names better. But let's not derail
Sigbjørn's code review with naming discussions! ;-)

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Yuta Kitamura, 2014-04-04 06:29:02]

On 2014/04/04 06:15:06, Mads Ager (chromium) wrote:
> STACK_ALLOCATED: The object is only stack allocated. Heap objects should be in
> Members but you do not need the trace method as they are on the stack. (Down
the
> line these might turn in to raw pointers, but for now Members indicates that
we
> have thought about them and explicitly taken care of them.)
> 
> DISALLOW_ALLOCATION: Cannot be allocated with new operators but can be a part
> object. If it has Members you need a trace method and the containing object
> needs to call that trace method.
> 
> ALLOW_ONLY_INLINE_ALLOCATION: Allows only placement new operator. This
disallows
> general allocation of this object but allows to put the object as a value
object
> in collections. If these have Members you need to have a trace method. That
> trace method will be called automatically by the Heap collections.
> 

Could you kindly document this kind of information at
<http://dev.chromium.org/blink/blink-gc>? I find the current documentation is
not really helpful and I'm not sure I can use these macros correctly in
the future changes.

[tkent, 2014-04-04 06:34:01]

On 2014/04/04 06:29:02, Yuta Kitamura wrote:
> Could you kindly document this kind of information at
> <http://dev.chromium.org/blink/blink-gc%3E I find the current documentation is
> not really helpful and I'm not sure I can use these macros correctly in
> the future changes.

I'll do.

[sof, 2014-04-04 06:35:40]

On 2014/04/04 06:34:01, tkent wrote:
> On 2014/04/04 06:29:02, Yuta Kitamura wrote:
> > Could you kindly document this kind of information at
> > <http://dev.chromium.org/blink/blink-gc%3E I find the current documentation
is
> > not really helpful and I'm not sure I can use these macros correctly in
> > the future changes.
> 
> I'll do.

It's a very clear overview - how about I (also) add it to Heap.h in this CL?

[haraken, 2014-04-04 06:41:20]

On 2014/04/04 06:35:40, sof wrote:
> On 2014/04/04 06:34:01, tkent wrote:
> > On 2014/04/04 06:29:02, Yuta Kitamura wrote:
> > > Could you kindly document this kind of information at
> > > <http://dev.chromium.org/blink/blink-gc%3E I find the current
documentation
> is
> > > not really helpful and I'm not sure I can use these macros correctly in
> > > the future changes.
> > 
> > I'll do.
> 
> It's a very clear overview - how about I (also) add it to Heap.h in this CL?

Sounds great.

[sof, 2014-04-04 07:33:22]

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
File Source/core/editing/Editor.cpp (left):

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
Source/core/editing/Editor.cpp:787: RefPtr<Range> range =
selection.toNormalizedRange();
On 2014/04/04 05:32:02, sof wrote:
> On 2014/04/04 02:37:25, Yuta Kitamura wrote:
> > On 2014/04/04 01:56:56, tkent wrote:
> > > I wonder if we can remove it.
> > 
> > It's probably OK to completely remove this line.
> 
> Great, will do that - it wasn't clear if it had a required effect on editing
> state or not.

Now done.

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
File Source/core/editing/EditorCommand.cpp (right):

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Edit...
Source/core/editing/EditorCommand.cpp:221: RefPtrWillBeRawPtr<Range>
ALLOW_UNUSED oldRange = frame.selection().selection().toNormalizedRange();
On 2014/04/04 02:37:25, Yuta Kitamura wrote:
> This line can be removed, as well.

Done.

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Text...
File Source/core/editing/TextCheckingHelper.h (right):

https://codereview.chromium.org/224113002/diff/40001/Source/core/editing/Text...
Source/core/editing/TextCheckingHelper.h:39: DISALLOW_ALLOCATION();
On 2014/04/04 01:56:56, tkent wrote:
> probably STACK_ALLOCATED() is correct, and trace() is unnecessary.
> 

Thanks, that is how its instances are allocated; switched.

https://codereview.chromium.org/224113002/diff/40001/public/web/WebRange.h
File public/web/WebRange.h (right):

https://codereview.chromium.org/224113002/diff/40001/public/web/WebRange.h#ne...
public/web/WebRange.h:63: bool isNull() const { return m_private.isNull(); }
On 2014/04/04 02:30:19, haraken wrote:
> 
> If we forget to change this, is there any way to notice the error?

If I understand your question correctly, then yes - WebPrivatePtr<> cannot be
converted to a bool.

[sof, 2014-04-04 07:34:44]

On 2014/04/04 06:41:20, haraken wrote:
> On 2014/04/04 06:35:40, sof wrote:
> > On 2014/04/04 06:34:01, tkent wrote:
> > > On 2014/04/04 06:29:02, Yuta Kitamura wrote:
> > > > Could you kindly document this kind of information at
> > > > <http://dev.chromium.org/blink/blink-gc%3E I find the current
> documentation
> > is
> > > > not really helpful and I'm not sure I can use these macros correctly in
> > > > the future changes.
> > > 
> > > I'll do.
> > 
> > It's a very clear overview - how about I (also) add it to Heap.h in this CL?
> 
> Sounds great.

Added it just above the definition of DISALLOW_ALLOCATION().

[tkent, 2014-04-04 07:44:44]

lgtm

[sof, 2014-04-04 07:47:57]

On 2014/04/04 06:15:06, Mads Ager (chromium) wrote:
> Drive-by comments inline.
> 
> On 2014/04/03 20:28:48, sof wrote:
> > Please take a look.
> > 
> > Some points worth mentioning:
> > 
> >  - Ran into some SpellChecker code that depended upon RefPtr<T> being
> > zero-initialized, whereas RawPtr<T> is not. Explicitly initialized all (with
> > nullptr) to address; slightly worrisome.
> 
> This was a change we made on purpose. The end result will need explicit
> initialization and therefore we would like to catch it as we go instead of
> having to fight this whenever we want to ship something without transition
> types.
> 
> RefPtrWillBeRawPtr<T> a;
> 
> Will turn into:
> 
> T* a;
> 
> Which is uninitialized. Whenever we have a RawPtr we need to really think of
> them as raw pointers (including when it comes to initialization). Therefore,
the
> requirement to write:
> 
> RefPtrWillBeRawPtr<T> a = nullptr;
> 

Requiring explicit initializations makes good sense; the worry is introducing
uninitialized references when adding the transition types. Even if we have tools
that will catch that out, it depends on exercising the code path. So, something
to keep an eye out for.

[Mads Ager (chromium), 2014-04-04 07:48:37]

LGTM

Maybe consider the removal of the two RefPtrs to a separate change to make sure
there are no changes in behavior for the non-oilpan build in this change? If the
removal of the RefPtrs turns out to be unsafe it would be nice to just have a
simple removal changelist to revert. :)

https://codereview.chromium.org/224113002/diff/60001/Source/core/editing/Edit...
File Source/core/editing/Editor.cpp (left):

https://codereview.chromium.org/224113002/diff/60001/Source/core/editing/Edit...
Source/core/editing/Editor.cpp:787: RefPtr<Range> range =
selection.toNormalizedRange();
Range is unused, are we sure this is not a protecting RefPtr that needs to be
there in the non-oilpan build to guard against deletion in the below code?

https://codereview.chromium.org/224113002/diff/60001/Source/core/editing/Edit...
File Source/core/editing/EditorCommand.cpp (left):

https://codereview.chromium.org/224113002/diff/60001/Source/core/editing/Edit...
Source/core/editing/EditorCommand.cpp:221: RefPtr<Range> oldRange =
frame.selection().selection().toNormalizedRange();
Ditto?

[Mads Ager (chromium), 2014-04-04 07:49:46]

On 2014/04/04 07:47:57, sof wrote:
> On 2014/04/04 06:15:06, Mads Ager (chromium) wrote:
> > Drive-by comments inline.
> > 
> > On 2014/04/03 20:28:48, sof wrote:
> > > Please take a look.
> > > 
> > > Some points worth mentioning:
> > > 
> > >  - Ran into some SpellChecker code that depended upon RefPtr<T> being
> > > zero-initialized, whereas RawPtr<T> is not. Explicitly initialized all
(with
> > > nullptr) to address; slightly worrisome.
> > 
> > This was a change we made on purpose. The end result will need explicit
> > initialization and therefore we would like to catch it as we go instead of
> > having to fight this whenever we want to ship something without transition
> > types.
> > 
> > RefPtrWillBeRawPtr<T> a;
> > 
> > Will turn into:
> > 
> > T* a;
> > 
> > Which is uninitialized. Whenever we have a RawPtr we need to really think of
> > them as raw pointers (including when it comes to initialization). Therefore,
> the
> > requirement to write:
> > 
> > RefPtrWillBeRawPtr<T> a = nullptr;
> > 
> 
> Requiring explicit initializations makes good sense; the worry is introducing
> uninitialized references when adding the transition types. Even if we have
tools
> that will catch that out, it depends on exercising the code path. So,
something
> to keep an eye out for.

Yes, definitely. I have put in a zap value. So if you see crashes in debug mode
that access 0x3a3a3a3a you are working with uninitialized raw pointers.

[haraken, 2014-04-04 07:56:09]

LGTM with mad's comment about a protected RefPtr.

[Yuta Kitamura, 2014-04-04 08:12:29]

On 2014/04/04 07:48:37, Mads Ager (chromium) wrote:
> Maybe consider the removal of the two RefPtrs to a separate change to make
sure
> there are no changes in behavior for the non-oilpan build in this change? If
the
> removal of the RefPtrs turns out to be unsafe it would be nice to just have a
> simple removal changelist to revert. :)

I'm mostly sure that these two are just oversight, i.e. they did not get removed
by accident when their uses were gone, according to my git-blame-fu ;)

And also I'm sure that these ranges do not serve as our idiomatic "protect"
variables; VisibleSelection::toNormalizedRange() returns a fresh Range object
every time it is called, and there is no point in keeping an unused Range object
alive AFAIK.

So I think it is safe to land the patch as-is, but if you want to be extra
cautious, I'm fine with separating the patch :)

[sof, 2014-04-04 08:19:20]

On 2014/04/04 08:12:29, Yuta Kitamura wrote:
> On 2014/04/04 07:48:37, Mads Ager (chromium) wrote:
> > Maybe consider the removal of the two RefPtrs to a separate change to make
> sure
> > there are no changes in behavior for the non-oilpan build in this change? If
> the
> > removal of the RefPtrs turns out to be unsafe it would be nice to just have
a
> > simple removal changelist to revert. :)
> 
> I'm mostly sure that these two are just oversight, i.e. they did not get
removed
> by accident when their uses were gone, according to my git-blame-fu ;)
> 
> And also I'm sure that these ranges do not serve as our idiomatic "protect"
> variables; VisibleSelection::toNormalizedRange() returns a fresh Range object
> every time it is called, and there is no point in keeping an unused Range
object
> alive AFAIK.
> 
> So I think it is safe to land the patch as-is, but if you want to be extra
> cautious, I'm fine with separating the patch :)

Thanks for the extra clarification & assurances :) I've been digging into the
history a bit, these range objects appear to have become unused via
https://codereview.chromium.org/26237004

With that known and your explanation, I think it is fine to proceed without
separating out these two removals.

[Mads Ager (chromium), 2014-04-04 08:22:44]

Sounds good. Thanks for the context! :)

[sof, 2014-04-04 08:23:22]

Thanks everyone for the reviews & help!

[sof, 2014-04-04 08:24:04]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-04-04 08:24:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/224113002/60001

[commit-bot: I haz the power, 2014-04-04 08:38:37]

Change committed as 170845