[oilpan] The Node hierarchy should have correct accept method chains

[oilpan] The Node hierarchy should have correct accept call chains

This CL guarantees the following points:

- All classes under Node (except SVG classes) have acceptHeapVisitor.
- Members in each class are visited in the acceptHeapVisitor.
- The acceptHeapVisitor is redirected to acceptHeapVisitor of the super class.

I'll rename acceptHeapVisitor to accept in a separate CL.

R=ager@chromium.org

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

[haraken, 2013-07-25 06:14:37]

Several tests are crashing probably due to Member mis-visiting. Not yet for
review. (I'd hope we have some verification tool here...)

[haraken, 2013-07-25 10:43:56]

Looks like the remaining issue is HandleScope overflow.
http://haraken-pcz2.tok.corp.google.com/oilpan-bot/try-results/296950/layout-...

532 tests are crashing.

[haraken, 2013-07-25 12:31:36]

I begin to think that maybe we shouldn't land this CL.

This CL handlifies all raw pointers on heap objects in the Node hierarchy. For
example, this CL replaces "Node* m_parentNode" with "Member<Node> m_parentNode".
On the other hand, we haven't yet handlified all Node* pointers. Consequently,
we end up having the following code:

  Node* Node::parentNode() {
    return Handle<Node>(m_parentNode).raw();
  }

Remember that parentNode() is a very hot method. Thus, this causes the
HandleScope overflow very easily in a lot of places. To address the overflow, we
have to insert a lot of HandleScopes.

The problem here is that the Handle created in parentNode() is just a temporary
one and will be removed once we handlify all Node* pointers. In other words,
landing the CL requires us to insert a lot of HandleScopes that won't be needed
in the future. For example, it would be sad that we have to insert a HandleScope
to the following code:

  [before]
  Position previousCandidate(PositionIterator p) {
    while (!p.atStart()) { // This calls parentNode().
      HandleScope scope;
      ...;
    }
  }

  [after]
  Position previousCandidate(PositionIterator p) {
    while (1) {
      HandleScope scope;
      if (p.atStart()) break;
      ...;
    }
  }

In summary, I don't think it's a good idea to insert HandleScopes just for this
CL.


I think there are two options:

(a) In this CL, we just add accept call chains and don't introduce Members (i.e.
we keep raw pointers on heap objects as is). In other words, we don't introduce
Member<Node> until we handlify all Node* pointers.

(b) Make Member::raw() public. Then we don't need to create unnecessary Handles
which will be removed in the future:

  Node* Node::parentNode() {
    return m_parentNode.raw();
  }


What do you think?

[Mads Ager (chromium), 2013-07-25 12:38:30]

On 2013/07/25 12:31:36, haraken wrote:
> I begin to think that maybe we shouldn't land this CL.
> 
> This CL handlifies all raw pointers on heap objects in the Node hierarchy. For
> example, this CL replaces "Node* m_parentNode" with "Member<Node>
m_parentNode".
> On the other hand, we haven't yet handlified all Node* pointers. Consequently,
> we end up having the following code:
> 
>   Node* Node::parentNode() {
>     return Handle<Node>(m_parentNode).raw();
>   }
> 
> Remember that parentNode() is a very hot method. Thus, this causes the
> HandleScope overflow very easily in a lot of places. To address the overflow,
we
> have to insert a lot of HandleScopes.
> 
> The problem here is that the Handle created in parentNode() is just a
temporary
> one and will be removed once we handlify all Node* pointers. In other words,
> landing the CL requires us to insert a lot of HandleScopes that won't be
needed
> in the future. For example, it would be sad that we have to insert a
HandleScope
> to the following code:
> 
>   [before]
>   Position previousCandidate(PositionIterator p) {
>     while (!p.atStart()) { // This calls parentNode().
>       HandleScope scope;
>       ...;
>     }
>   }
> 
>   [after]
>   Position previousCandidate(PositionIterator p) {
>     while (1) {
>       HandleScope scope;
>       if (p.atStart()) break;
>       ...;
>     }
>   }
> 
> In summary, I don't think it's a good idea to insert HandleScopes just for
this
> CL.
> 
> 
> I think there are two options:
> 
> (a) In this CL, we just add accept call chains and don't introduce Members
(i.e.
> we keep raw pointers on heap objects as is). In other words, we don't
introduce
> Member<Node> until we handlify all Node* pointers.
> 
> (b) Make Member::raw() public. Then we don't need to create unnecessary
Handles
> which will be removed in the future:
> 
>   Node* Node::parentNode() {
>     return m_parentNode.raw();
>   }
> 
> 
> What do you think?

I would like the full accept call chain to land soon, so if we can split that
out and land that first I would be in favor of that.

Then we can slowly move the raw pointers to Members in smaller bits finding the
correct places to put in handle scopes as we go.

[Mads Ager (chromium), 2013-07-25 12:39:49]

On 2013/07/25 12:38:30, Mads Ager (chromium) wrote:
> On 2013/07/25 12:31:36, haraken wrote:
> > I begin to think that maybe we shouldn't land this CL.
> > 
> > This CL handlifies all raw pointers on heap objects in the Node hierarchy.
For
> > example, this CL replaces "Node* m_parentNode" with "Member<Node>
> m_parentNode".
> > On the other hand, we haven't yet handlified all Node* pointers.
Consequently,
> > we end up having the following code:
> > 
> >   Node* Node::parentNode() {
> >     return Handle<Node>(m_parentNode).raw();
> >   }
> > 
> > Remember that parentNode() is a very hot method. Thus, this causes the
> > HandleScope overflow very easily in a lot of places. To address the
overflow,
> we
> > have to insert a lot of HandleScopes.
> > 
> > The problem here is that the Handle created in parentNode() is just a
> temporary
> > one and will be removed once we handlify all Node* pointers. In other words,
> > landing the CL requires us to insert a lot of HandleScopes that won't be
> needed
> > in the future. For example, it would be sad that we have to insert a
> HandleScope
> > to the following code:
> > 
> >   [before]
> >   Position previousCandidate(PositionIterator p) {
> >     while (!p.atStart()) { // This calls parentNode().
> >       HandleScope scope;
> >       ...;
> >     }
> >   }
> > 
> >   [after]
> >   Position previousCandidate(PositionIterator p) {
> >     while (1) {
> >       HandleScope scope;
> >       if (p.atStart()) break;
> >       ...;
> >     }
> >   }
> > 
> > In summary, I don't think it's a good idea to insert HandleScopes just for
> this
> > CL.
> > 
> > 
> > I think there are two options:
> > 
> > (a) In this CL, we just add accept call chains and don't introduce Members
> (i.e.
> > we keep raw pointers on heap objects as is). In other words, we don't
> introduce
> > Member<Node> until we handlify all Node* pointers.
> > 
> > (b) Make Member::raw() public. Then we don't need to create unnecessary
> Handles
> > which will be removed in the future:
> > 
> >   Node* Node::parentNode() {
> >     return m_parentNode.raw();
> >   }
> > 
> > 
> > What do you think?
> 
> I would like the full accept call chain to land soon, so if we can split that
> out and land that first I would be in favor of that.

PS: I need to full accept call chain to land because we are now (with my Text
change) at the point where we need ContainerNode::accept to visit all its
children. That means that we need to full call chains to make sure that we get
there.

[haraken, 2013-07-25 12:40:01]

> I would like the full accept call chain to land soon, so if we can split that
> out and land that first I would be in favor of that.
> 
> Then we can slowly move the raw pointers to Members in smaller bits finding
the
> correct places to put in handle scopes as we go.

ok, I'll revert the Member changes and leave the accept call change.

[haraken, 2013-07-25 13:08:31]

OK, the Patch Set 5 implements the perfect accept chain for all classes under
Node (except for SVG). I noticed a couple of existing wrong chains (which I
introduced in previous patches) and fixed them in this CL. I'd be happy if you
could take a careful look about the call chain. PTAL.

In the next CL, I'll move acceptHeapVisitor from 'protected' to 'public' and
rename it to accept.

[Mads Ager (chromium), 2013-07-25 13:35:12]

LGTM!

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLMedi...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLMedi...
Source/core/html/HTMLMediaElement.cpp:3070: for (unsigned i = 0;
!canUseSourceElement && i < potentialSourceNodes.size(); ++i) {
HandleScope?

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLMedi...
Source/core/html/HTMLMediaElement.cpp:3214: if (source !=
Handle<HTMLSourceElement>(m_currentSourceNode).raw() && source !=
Handle<Node>(m_nextChildNodeToConsider).raw())
Do you need the handle wrapping followed by raw() here. I thought comparisons
with raw pointers actually worked, but I might be wrong. In any case source will
become a handle and this will disappear. :)

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLMete...
File Source/core/html/HTMLMeterElement.cpp (right):

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLMete...
Source/core/html/HTMLMeterElement.cpp:252:
LabelableElement::acceptHeapVisitor(visitor);
Good catch. :)

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLScri...
File Source/core/html/HTMLScriptElement.cpp (right):

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLScri...
Source/core/html/HTMLScriptElement.cpp:179: // FIXME(oilpan): There is multiple
inheritance going on here. This is a ScriptElement as
Yup, thanks for adding the comment here. :)

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLSele...
File Source/core/html/HTMLSelectElement.h (right):

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLSele...
Source/core/html/HTMLSelectElement.h:39: class HTMLSelectElement : public
HTMLFormControlElementWithState, public TypeAheadDataSource {
We will probably have TypeAheadDataSource in members somewhere (in TypeAhead?).
It has no members so we don't have to visit it, however, we do need to have a
trait for it to adjust the pointers when we visit them through members because
it will not point to the start of the object.

Nothing needs to be done for this change.

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLTrac...
File Source/core/html/HTMLTrackElement.h (right):

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLTrac...
Source/core/html/HTMLTrackElement.h:37: class HTMLTrackElement : public
HTMLElement, public TextTrackClient {
Another one. When we get TextTrackClients that we need to visit we will have to
have a trait for it to get to the start of the object.

[haraken, 2013-07-25 13:44:49]

Thanks for reviewing!

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLMedi...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLMedi...
Source/core/html/HTMLMediaElement.cpp:3070: for (unsigned i = 0;
!canUseSourceElement && i < potentialSourceNodes.size(); ++i) {
On 2013/07/25 13:35:12, Mads Ager (chromium) wrote:
> HandleScope?

Done.

https://codereview.chromium.org/20123003/diff/15001/Source/core/html/HTMLMedi...
Source/core/html/HTMLMediaElement.cpp:3214: if (source !=
Handle<HTMLSourceElement>(m_currentSourceNode).raw() && source !=
Handle<Node>(m_nextChildNodeToConsider).raw())

I reverted the members to RefPtrs (and remove these Handle wrappings). We should
move them to Members when handlifying HTMLSourceElement and Node.

[haraken, 2013-07-25 13:45:39]

Committed patchset #7 manually as r154915.