Oilpan: Prepare to move RenderObject and RenderObjectChildList to Oilpan heap.

Oilpan: Prepare to move RenderObject and RenderObjectChildList to Oilpan heap.

We have a lot of remaining raw pointer members to RenderObject. Follow-up
patches will resolve them.

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

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

[tkent, 2014-07-29 08:25:34]

Please review this.

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:2502: visitor->trace(m_data.m_renderer);
m_data is a union of a rareData and a renderer. We need to trace only one of
them.

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/NodeRareData...
File Source/core/dom/NodeRareData.cpp (right):

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/NodeRareData...
Source/core/dom/NodeRareData.cpp:52: visitor->trace(m_renderer);
trace only in ENABLE(OILPAN) because m_renderer is a raw pointer member.

https://codereview.chromium.org/423093002/diff/1/Source/core/editing/TextIter...
File Source/core/editing/TextIterator.h (right):

https://codereview.chromium.org/423093002/diff/1/Source/core/editing/TextIter...
Source/core/editing/TextIterator.h:179: RawPtrWillBeMember<RenderText>
m_firstLetterText;
TextIterator is STACK_ALLOCATED.  We don't need to add this to trace().

[zerny-chromium, 2014-07-29 08:46:37]

mostly lgtm

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.h#newco...
Source/core/dom/Node.h:110: RenderObject* m_renderer;
Could we alternatively make NodeRareDataBase GCed and dispatch to its
traceAfterDispatch from NodeRareData? Then this can be a Member (which we will
want to enforce in due time).

[haraken, 2014-07-29 11:28:06]

LGTM

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:2502: visitor->trace(m_data.m_renderer);
On 2014/07/29 08:25:33, tkent wrote:
> m_data is a union of a rareData and a renderer. We need to trace only one of
> them.

Shall we add a comment about it?

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.h#newco...
Source/core/dom/Node.h:110: RenderObject* m_renderer;
On 2014/07/29 08:46:37, zerny-chromium wrote:
> Could we alternatively make NodeRareDataBase GCed and dispatch to its
> traceAfterDispatch from NodeRareData? Then this can be a Member (which we will
> want to enforce in due time).

Sounds nicer.

https://codereview.chromium.org/423093002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/423093002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:2832: void RenderObject::postDestroy()

That's great we already have a method to destroy RenderObject explicitly :)

[tkent, 2014-07-29 14:01:44]

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.h#newco...
Source/core/dom/Node.h:110: RenderObject* m_renderer;
On 2014/07/29 08:46:37, zerny-chromium wrote:
> Could we alternatively make NodeRareDataBase GCed and dispatch to its
> traceAfterDispatch from NodeRareData? Then this can be a Member (which we will
> want to enforce in due time).

I think we can do it.  However I have no idea how to implement
NodeRareDataBase::finalizeGarbageCollectedObject. We have no ways to distinguish
NodeRareData and ElementRareData in NodeRareDataBase.

[tkent, 2014-07-30 02:35:14]

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:2502: visitor->trace(m_data.m_renderer);
On 2014/07/29 11:28:06, haraken wrote:
> On 2014/07/29 08:25:33, tkent wrote:
> > m_data is a union of a rareData and a renderer. We need to trace only one of
> > them.
> 
> Shall we add a comment about it?
> 

Done.

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.h#newco...
Source/core/dom/Node.h:110: RenderObject* m_renderer;
On 2014/07/29 14:01:43, tkent wrote:
> On 2014/07/29 08:46:37, zerny-chromium wrote:
> > Could we alternatively make NodeRareDataBase GCed and dispatch to its
> > traceAfterDispatch from NodeRareData? Then this can be a Member (which we
will
> > want to enforce in due time).
> 
> I think we can do it.  However I have no idea how to implement
> NodeRareDataBase::finalizeGarbageCollectedObject. We have no ways to
distinguish
> NodeRareData and ElementRareData in NodeRareDataBase.
> 

I added a FIXME comment.

[tkent, 2014-07-30 03:58:37]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-07-30 03:59:24]

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

[commit-bot: I haz the power, 2014-07-30 04:03:22]

Change committed as 179196

[zerny-chromium, 2014-07-30 04:36:45]

>
https://codereview.chromium.org/423093002/diff/1/Source/core/dom/Node.h#newco...
> Source/core/dom/Node.h:110: RenderObject* m_renderer;
> On 2014/07/29 14:01:43, tkent wrote:
> > On 2014/07/29 08:46:37, zerny-chromium wrote:
> > > Could we alternatively make NodeRareDataBase GCed and dispatch to its
> > > traceAfterDispatch from NodeRareData? Then this can be a Member (which we
> will
> > > want to enforce in due time).
> > 
> > I think we can do it.  However I have no idea how to implement
> > NodeRareDataBase::finalizeGarbageCollectedObject. We have no ways to
> distinguish
> > NodeRareData and ElementRareData in NodeRareDataBase.
> > 
> 
> I added a FIXME comment.

I see. We will have to think about a solution later. Thanks for adding the
FIXME.

[tkent, 2014-07-30 05:36:08]

A revert of this CL has been created in
https://codereview.chromium.org/425203002/ by tkent@chromium.org.

The reason for reverting is: It seems it caused a lot of crashes in Oilpan bots.
.

[tkent, 2014-07-30 07:39:21]

On 2014/07/30 05:36:08, tkent wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/425203002/ by mailto:tkent@chromium.org.
> 
> The reason for reverting is: It seems it caused a lot of crashes in Oilpan
bots.

I found some renderers had children without RenderObjectChildList.

[tkent, 2014-08-01 08:15:10]

I updated patch.  I needed to add a lot of RawPtrWillBeMember and trace to avoid
unexpected RenderObject destruction.  I think this CL won't add new failures in
Oilpan bots.

[haraken, 2014-08-01 17:48:22]

I have a concern about introducing RawPtrWillBePersistent, since it's very
likely to cause leaks. I think we should refactor code so that we don't need to
introduce them.

As far as I see this CL, we'll be able to avoid introducing
RawPtrWillBePersistent by either:

- Changing the RawPtr to an OwnPtr where an OwnPtr should be used; or
- Moving the holder object to the heap.

This CL introduces a couple of persistent hash maps/sets of
Member<RenderObject>s. This also has a risk of producing leaks.

Although you're saying that the latest CL fixes crashes, but you want to also
double-confirm that the CL doesn't leak RenderObjects.

https://codereview.chromium.org/423093002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:486: RawPtrWillBePersistent<RenderScrollbarPart>
m_scrollCorner;

This doesn't need to be a Persistent. I'd prefer keeping it as a raw pointer
with a comment.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:488: RenderLineBoxList m_lineBoxes;   // All
of the root line boxes created for this block flow.  For example,
<div>Hello<br>world.</div> will have two total lines for the <div>.

Are you sure that raw pointers in RenderLineBoxList are safe? If not, we'll need
to move the InlineBox hierarchy before landing this CL.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:64: typedef
WillBeHeapHashMap<RawPtrWillBeMember<const RenderBoxModelObject>,
RawPtrWillBeMember<RenderBoxModelObject> > ContinuationMap;

Just to confirm: Members in this persistent handle don't cause leaks, right?
(Don't they need to be WeakMembers?)

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:69: typedef
WillBeHeapHashMap<RawPtrWillBeMember<const RenderBoxModelObject>,
RawPtrWillBeMember<RenderTextFragment> > FirstLetterRemainingTextMap;

Ditto.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderInline.h (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderInline.h:189: RenderLineBoxList m_lineBoxes;   //
All of the line boxes created for this inline flow.  For example,
<i>Hello<br>world.</i> will have two <i> line boxes.

Ditto. Are these raw pointers in RenderLineBoxList safe?

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerModelObject.cpp (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerModelObject.cpp:44: ASSERT(m_didCallDestroy);

You won't need this because ~RenderObject() already has this ASSERT.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerReflectionInfo.h (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerReflectionInfo.h:57: class
RenderLayerReflectionInfo {

It looks easy to move RenderLayerReflectionInfo to the heap. A separate CL is
fine but in that case let's add a FIXME.

I'm concerned about introducing RawPtrWillBePersistent. It's very likely to
cause leaks.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderScrollbar.h (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderScrollbar.h:87:
WillBePersistentHeapHashMap<unsigned, RawPtrWillBeMember<RenderScrollbarPart> >
m_parts;

Are you sure that these Members don't cause leaks? (Don't they need to be
WeakMembers?)

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableCellTest.cpp (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderTableCellTest.cpp:48:
RawPtrWillBePersistent<RenderTableCell> m_cell;

This should be OwnPtr in non-oilpan builds.

In order to avoid introducing RawPtrWillBePersistent, we might want to first
refactor code to use an OwnPtr where an OwnPtr should be used instead of a raw
pointer.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableRowTest.cpp (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderTableRowTest.cpp:54:
RawPtrWillBePersistent<RenderTableRow> m_row;

Ditto. We want to first make this an OwnPtr in non-oilpan builds.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableSection.h (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.h:301: HashSet<RenderTableCell*>
m_overflowingCells;

You'll need to take care of these raw pointers. I think this should be a hash
set of Members.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.h:308: HashMap<pair<const
RenderTableCell*, int>, CollapsedBorderValue > m_cellsCollapsedBorders;

Ditto. This should be a hash map of Members.

[tkent, 2014-08-05 06:15:32]

On 2014/08/01 17:48:22, haraken wrote:
> I have a concern about introducing RawPtrWillBePersistent, since it's very
> likely to cause leaks. I think we should refactor code so that we don't need
to
> introduce them.

Almost all of RawPtrWillBePersistent is temporary.  They help to make the patch
smaller and we should remove them soon.  RenderObject has a strong reference to
a Node, so the leak detector will detect Node leak if we leak RenderObjects.

However it's hard and worthless to move gtest test fixtures to Oilpan heap, and
RenderObject is not compatible with OwnPtr now.  So introducing
RawPtrWillBePersistent for RenderTableCellTest.cpp and RenderTableRowTest.cpp is
reasonable.  Using OwnPtr for RenderObject is risky because we can't detect
RenderObject leak in non-Oilpan build.

[haraken, 2014-08-05 09:00:18]

On 2014/08/05 06:15:32, tkent wrote:
> On 2014/08/01 17:48:22, haraken wrote:
> > I have a concern about introducing RawPtrWillBePersistent, since it's very
> > likely to cause leaks. I think we should refactor code so that we don't need
> to
> > introduce them.
> 
> Almost all of RawPtrWillBePersistent is temporary.  They help to make the
patch
> smaller and we should remove them soon.  RenderObject has a strong reference
to
> a Node, so the leak detector will detect Node leak if we leak RenderObjects.
> 
> However it's hard and worthless to move gtest test fixtures to Oilpan heap,
and
> RenderObject is not compatible with OwnPtr now.  So introducing
> RawPtrWillBePersistent for RenderTableCellTest.cpp and RenderTableRowTest.cpp
is
> reasonable.  Using OwnPtr for RenderObject is risky because we can't detect
> RenderObject leak in non-Oilpan build.

Makes sense.

However, as commented in #13, this CL is introducing Persistents to places where
raw pointers are enough. I'm a bit concerned that this will produce a situation
where the Persistents keep the RenderObjects alive longer than needed and it
hides real bugs (For example, you mentioned that the CL fixed all crashes, but
I'm not sure whether the crashes are gone because the CL really fixed it or the
crashes are gone because the CL introduced Persistents that keep the objects
longer than needed).

So I'm fine with introducing RawPtrWillBePersistent, but I might want to limit
the usage as much as possible.

[tkent, 2014-08-07 04:43:03]

https://codereview.chromium.org/423093002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:486: RawPtrWillBePersistent<RenderScrollbarPart>
m_scrollCorner;
On 2014/08/01 17:48:21, haraken wrote:
> 
> This doesn't need to be a Persistent. I'd prefer keeping it as a raw pointer
> with a comment.

No, this must not be a raw pointer. FrameView owns the RenderScrollbarPart.
There's no other strong references to this object.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:488: RenderLineBoxList m_lineBoxes;   // All
of the root line boxes created for this block flow.  For example,
<div>Hello<br>world.</div> will have two total lines for the <div>.
On 2014/08/01 17:48:21, haraken wrote:
> 
> Are you sure that raw pointers in RenderLineBoxList are safe? If not, we'll
need
> to move the InlineBox hierarchy before landing this CL.

What do you mean? RenderLineBoxList doesn't have RenderObject raw pointers.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:64: typedef
WillBeHeapHashMap<RawPtrWillBeMember<const RenderBoxModelObject>,
RawPtrWillBeMember<RenderBoxModelObject> > ContinuationMap;
On 2014/08/01 17:48:21, haraken wrote:
> 
> Just to confirm: Members in this persistent handle don't cause leaks, right?
> (Don't they need to be WeakMembers?)

I'm not confident of it.  I'll revisit here if the leak detector detects leaks.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerModelObject.cpp (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerModelObject.cpp:44: ASSERT(m_didCallDestroy);
On 2014/08/01 17:48:21, haraken wrote:
> 
> You won't need this because ~RenderObject() already has this ASSERT.

Right.  This was a local debugging code.

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderScrollbar.h (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderScrollbar.h:87:
WillBePersistentHeapHashMap<unsigned, RawPtrWillBeMember<RenderScrollbarPart> >
m_parts;
On 2014/08/01 17:48:21, haraken wrote:
> 
> Are you sure that these Members don't cause leaks? (Don't they need to be
> WeakMembers?)

I'm sure.  RenderScrollbar owns these RenderScrollbarPart objects, and no other
strong references to them.

[haraken, 2014-08-07 07:45:05]

(I'll take another look later today.)

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/423093002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.h:488: RenderLineBoxList m_lineBoxes;   // All
of the root line boxes created for this block flow.  For example,
<div>Hello<br>world.</div> will have two total lines for the <div>.
On 2014/08/07 04:43:03, tkent wrote:
> On 2014/08/01 17:48:21, haraken wrote:
> > 
> > Are you sure that raw pointers in RenderLineBoxList are safe? If not, we'll
> need
> > to move the InlineBox hierarchy before landing this CL.
> 
> What do you mean? RenderLineBoxList doesn't have RenderObject raw pointers.

In general, raw pointers in on-heap objects can cause use-after-free. In this
case, a RenderBlock (on-heap) has raw pointers to the InlineBox hierarhcy via
the RenderLineBoxList. If the RenderBlock object outlives the InlineBox
hierarhcy (because the RenderBlock object is not collected until the next GC is
triggered), the raw pointers to the InlineBox hierarchy become stale pointers.

[haraken, 2014-08-07 15:18:42]

Sorry about the review delay. LGTM.

As commented below, I think it's easy to replace RawPtrWillBePersistent with
OwnPtrWillBePersistent. Let's avoid introducing RawPtrWillBePersistent.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderBoxModelObject.cpp:2520: continuationMap = new
OwnPtrWillBePersistent<ContinuationMap>(adoptPtrWillBeNoop(new
ContinuationMap));

Shouldn't this be:

*continuationMap = adoptPtrWillBeNoop(new ContinuationMap)

or

continuationMap = new OwnPtrWillBePersistent<ContinuationMap>(new
ContinuationMap)

?

new OwnPtr<X>(adoptPtr(new X)) seems strange.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderBoxModelObject.cpp:2523: if (continuationMap)

else if (continuationMap)

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderBoxModelObject.cpp:2551: firstLetterRemainingTextMap
= new OwnPtrWillBePersistent<FirstLetterRemainingTextMap>(adoptPtrWillBeNoop(new
FirstLetterRemainingTextMap));

Ditto.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderFrameSet.h (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderFrameSet.h:58: RenderFrameSet(HTMLFrameSetElement*);

Add explicit.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerReflectionInfo.cpp (left):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderLayerReflectionInfo.cpp:68:
RenderLayerReflectionInfo::~RenderLayerReflectionInfo()

Shall we keep the destructor and add ASSERT(!m_reflection)?

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerReflectionInfo.cpp (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderLayerReflectionInfo.cpp:68: void
RenderLayerReflectionInfo::dispose()

Can we rename this to "destroy", since the rendering hierarchy uses "destroy"
instead of "dispose"?

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerScrollableArea.h (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderLayerScrollableArea.h:252:
RawPtrWillBePersistent<RenderScrollbarPart> m_scrollCorner;

I still think we should use OwnPtr for this. You can just make
RenderScrollbarPart::createAnonymous return an OwnPtr.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderLayerScrollableArea.h:255:
RawPtrWillBePersistent<RenderScrollbarPart> m_resizer;

Ditto.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderQuote.h (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderQuote.h:36: RenderQuote(Document*, const QuoteType);

Add explicit.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbar.h (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.h:87:
WillBePersistentHeapHashMap<unsigned, RawPtrWillBeMember<RenderScrollbarPart> >
m_parts;

I checked the code base, but I'm not confident that this persistent hash map
doesn't cause leaks. Given that we explicitly add/remove items to/from the hash
map, shall we use an off-heap HashMap<unsigned, RenderScrollbarPart*> for
safety?

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCellTest.cpp (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableCellTest.cpp:48:
RawPtrWillBePersistent<RenderTableCell> m_cell;

This could be an OwnPtr. You just need to make RenderTableCell::createAnonymous
return an OwnPtr.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderTableRowTest.cpp (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableRowTest.cpp:54:
RawPtrWillBePersistent<RenderTableRow> m_row;

This could be an OwnPtr. You can just make RenderTableRow::createAnonymous
return an OwnPtr.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderTableSection.h (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.h:329: };

This would work, but can't we just use the WTF_INIT_AND_MEM_ macro?

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderWidget.cpp:79: // though |this| will be referred in
FrameView.

Just help me understand: This comment (and the fact that non-oilpan calls
deref() in destroy()) implies that destroy() can be called multiple times. This
means that willBeDestroyed() can be called multiple times, but is this behavior
OK?

[tkent, 2014-08-08 02:11:07]

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderBoxModelObject.cpp:2520: continuationMap = new
OwnPtrWillBePersistent<ContinuationMap>(adoptPtrWillBeNoop(new
ContinuationMap));
On 2014/08/07 15:18:41, haraken wrote:
> 
> Shouldn't this be:
> 
> *continuationMap = adoptPtrWillBeNoop(new ContinuationMap)
> 
> or
> 
> continuationMap = new OwnPtrWillBePersistent<ContinuationMap>(new
> ContinuationMap)
> 
> ?
> 
> new OwnPtr<X>(adoptPtr(new X)) seems strange.

No.  OwnPtr<T> doesn't have a constructor taking T*.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderBoxModelObject.cpp:2523: if (continuationMap)
On 2014/08/07 15:18:41, haraken wrote:
> 
> else if (continuationMap)

It's unrelated to this CL.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderFrameSet.h (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderFrameSet.h:58: RenderFrameSet(HTMLFrameSetElement*);
On 2014/08/07 15:18:41, haraken wrote:
> 
> Add explicit.

It's unrelated to this CL.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerReflectionInfo.cpp (left):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderLayerReflectionInfo.cpp:68:
RenderLayerReflectionInfo::~RenderLayerReflectionInfo()
On 2014/08/07 15:18:41, haraken wrote:
> 
> Shall we keep the destructor and add ASSERT(!m_reflection)?

Hmm, it needs to make this GarbageCollected*Finalized*.  I'm not sure if it's
better.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerReflectionInfo.cpp (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderLayerReflectionInfo.cpp:68: void
RenderLayerReflectionInfo::dispose()
On 2014/08/07 15:18:41, haraken wrote:
> 
> Can we rename this to "destroy", since the rendering hierarchy uses "destroy"
> instead of "dispose"?

will do.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerScrollableArea.h (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderLayerScrollableArea.h:252:
RawPtrWillBePersistent<RenderScrollbarPart> m_scrollCorner;
On 2014/08/07 15:18:41, haraken wrote:
> 
> I still think we should use OwnPtr for this. You can just make
> RenderScrollbarPart::createAnonymous return an OwnPtr.

OwnPtr can't have RenderObject.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderQuote.h (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderQuote.h:36: RenderQuote(Document*, const QuoteType);
On 2014/08/07 15:18:41, haraken wrote:
> 
> Add explicit.

No.  This has two arguments, and it's unrelated to this CL.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbar.h (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.h:87:
WillBePersistentHeapHashMap<unsigned, RawPtrWillBeMember<RenderScrollbarPart> >
m_parts;
On 2014/08/07 15:18:41, haraken wrote:
> 
> I checked the code base, but I'm not confident that this persistent hash map
> doesn't cause leaks. Given that we explicitly add/remove items to/from the
hash
> map, shall we use an off-heap HashMap<unsigned, RenderScrollbarPart*> for
> safety?

I'm confident that this won't cause leak.  This is the owner of
RenderScrollbarPart, and we can't use raw pointers.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCellTest.cpp (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableCellTest.cpp:48:
RawPtrWillBePersistent<RenderTableCell> m_cell;
On 2014/08/07 15:18:41, haraken wrote:
> 
> This could be an OwnPtr. You just need to make
RenderTableCell::createAnonymous
> return an OwnPtr.

OwnPtr can't have RenderObject.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderTableRowTest.cpp (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableRowTest.cpp:54:
RawPtrWillBePersistent<RenderTableRow> m_row;
On 2014/08/07 15:18:41, haraken wrote:
> 
> This could be an OwnPtr. You can just make RenderTableRow::createAnonymous
> return an OwnPtr.

OwnPtr can't have RenderObject.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderTableSection.h (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTableSection.h:329: };
On 2014/08/07 15:18:41, haraken wrote:
> 
> This would work, but can't we just use the WTF_INIT_AND_MEM_ macro?

No, we can't.  We can't use mem* functions for these classes because they
contain vectors.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderWidget.cpp:79: // though |this| will be referred in
FrameView.
On 2014/08/07 15:18:41, haraken wrote:
> 
> Just help me understand: This comment (and the fact that non-oilpan calls
> deref() in destroy()) implies that destroy() can be called multiple times.
This
> means that willBeDestroyed() can be called multiple times, but is this
behavior
> OK?

I don't think destroy() can be called multiple times.  Why do you think so?

[haraken, 2014-08-08 02:15:36]

Now I understand that we cannot change the RawPtrWillBePersistent to
OwnPtrWillBePersistent.

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/423093002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderWidget.cpp:79: // though |this| will be referred in
FrameView.
On 2014/08/08 02:11:06, tkent wrote:
> On 2014/08/07 15:18:41, haraken wrote:
> > 
> > Just help me understand: This comment (and the fact that non-oilpan calls
> > deref() in destroy()) implies that destroy() can be called multiple times.
> This
> > means that willBeDestroyed() can be called multiple times, but is this
> behavior
> > OK?
> 
> I don't think destroy() can be called multiple times.  Why do you think so?

Sorry, I was misreading the code. destroy() should be called only once.

[tkent, 2014-08-11 10:26:08]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-08-11 10:27:10]

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

[commit-bot: I haz the power, 2014-08-11 10:48:20]

Change committed as 179933