Remove SVGElementInstance

Remove SVGElementInstance

The SVGElementInstance tree has been removed from SVG2:

http://www.w3.org/TR/SVG2/changes.html#structure

This patch removes it completely. The exact naming of the various trees after removing the instance tree is still under discussion and will be implemented in a follow-up patch.

BUG=313438

[rwlbuis, 2014-06-03 18:15:06]

This is finally showing up green!

I think there are two main areas to fix yet:

- we need better and more consistent naming. So far we had:

<pdr> rwlbuis, adamk and I bikeshedded for a minute. We both like "instance" for
the element that's in the use shadow tree. That will be confusing in the
transition period since there are two kinds of instances. Adam suggested
prototype or template for the mapping going from the instance to the... thing
that makes the instance.
[23:32:52] <pdr> rwlbuis, how does this sound? Call the elements in the
use-created shadow tree instances, and leave the reverse mapping as
"corresponding elements", and doc these all over the place with some comments

 I'll update the patch to reflect these ideas


- @esprehn I am having some problems with the current assert in
SVGElement::correspondingElement. Basically in
SVGUseElement::clearResourceReferences I am removing elements in the user agent
shadow tree that we don't want anymore (error detected or just a reset/rebuild).
Right now we test for correspondingElement and remove the instance element ->
original element mapping in the SVGElement dtor, then we hit in
correspondingElement:

ASSERT(!hasSVGRareData() || !svgRareData()->correspondingElement() ||
containingShadowRoot());

because we are already removed from the user agent shadow tree at that point.
Pdr and myself don't see a mechanism so far to remove the mapping while the
element is still in the user agent shadow tree, and just about to be removed.
Could something be done with detach?

[eseidel, 2014-06-03 18:20:25]

https://codereview.chromium.org/272523002/diff/350001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/272523002/diff/350001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:528: // ASSERT(instance->inUseShadowTree());
Why commit commented out code?

[rwlbuis, 2014-06-03 18:32:31]

On 2014/06/03 18:20:25, eseidel wrote:
>
https://codereview.chromium.org/272523002/diff/350001/Source/core/svg/SVGElem...
> File Source/core/svg/SVGElement.cpp (right):
> 
>
https://codereview.chromium.org/272523002/diff/350001/Source/core/svg/SVGElem...
> Source/core/svg/SVGElement.cpp:528: // ASSERT(instance->inUseShadowTree());
> Why commit commented out code?

Hi Eric, sorry I should have said this is still kind of work in progress (but
close!). The two commented out lines are things we'd probably still like to have
and is related to my second point I mentioned above. I wanted to loop in esprehn
to see if there is some mechanism we can use here.

[pdr., 2014-06-03 18:53:47]

On 2014/06/03 18:15:06, rwlbuis wrote:
> - @esprehn I am having some problems with the current assert in
> SVGElement::correspondingElement. Basically in
> SVGUseElement::clearResourceReferences I am removing elements in the user
agent
> shadow tree that we don't want anymore (error detected or just a
reset/rebuild).
> Right now we test for correspondingElement and remove the instance element ->
> original element mapping in the SVGElement dtor, then we hit in
> correspondingElement:
> 
> ASSERT(!hasSVGRareData() || !svgRareData()->correspondingElement() ||
> containingShadowRoot());
> 
> because we are already removed from the user agent shadow tree at that point.
> Pdr and myself don't see a mechanism so far to remove the mapping while the
> element is still in the user agent shadow tree, and just about to be removed.
> Could something be done with detach?

I talked with esprehn and he pointed out that removedFrom should have all the
info we need. Should we be removing the instance mapping in the SVGElement dtor?
Maybe that should be done in removedFrom as well because the element references
probably shouldn't be around in the element is not in the tree.

[esprehn, 2014-06-03 23:01:24]

On 2014/06/03 at 18:53:47, pdr wrote:
> [...]
> 
> I talked with esprehn and he pointed out that removedFrom should have all the
info we need. Should we be removing the instance mapping in the SVGElement dtor?
Maybe that should be done in removedFrom as well because the element references
probably shouldn't be around in the element is not in the tree.

That sounds right to me. By the time the destructor runs the element should be
all cleaned up, removedFrom is the right place to do that clean up. The
destructor should assert that it's cleaned up, just like we ASSERT(!renderer())
in ~Node. Also pdr is the best. :)

[rwlbuis, 2014-06-04 00:04:42]

On 2014/06/03 23:01:24, esprehn wrote:
> On 2014/06/03 at 18:53:47, pdr wrote:
> > [...]
> > 
> > I talked with esprehn and he pointed out that removedFrom should have all
the
> info we need. Should we be removing the instance mapping in the SVGElement
dtor?
> Maybe that should be done in removedFrom as well because the element
references
> probably shouldn't be around in the element is not in the tree.
> 
> That sounds right to me. By the time the destructor runs the element should be
> all cleaned up, removedFrom is the right place to do that clean up. The
> destructor should assert that it's cleaned up, just like we
ASSERT(!renderer())
> in ~Node. Also pdr is the best. :)

I still think removedFrom is not what I want. At least, AFAICS at that point
inUseShadowTree() will be false and the ASSERTs will be triggered. In my latest
CL I use attach/detach and that shows up green but I wonder how much it
decreases performance for non-use SVG element usage... I guess because
attach/detach should be one-time only outside of <use> it should be okay.

[pdr., 2014-06-04 04:07:52]

Epic patch Rob!

My complaints below are mostly minor. If you don't mind, I'd like to do a couple
of rounds on this one to make sure we get it right.

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:113: if (correspondingElement() &&
correspondingElement()->instancesForElement().contains(this))
Can we make instancesForElement protected and move the contains logic into
removeInstanceMapping?

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.h:52: void attach(const AttachContext&) OVERRIDE;
Should these be virtual?

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.cpp (left):

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:529: // A general description from the SVG
spec, describing what buildInstanceTree() actually does.
I think this comment was useful. Could we update it instead of removing it?

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:346: static void
cloneChildrenAndAssociate(Node* toClone, PassRefPtr<Node> clone);
I'm not sure this is needed (see comments in expandUseElementsInShadowTree).

If it is needed, can we use recursion (having cloneChildrenAndAssociate call
itself) to avoid needing two functions here? It may be possible to refactor this
a bit to not need the
"cloneParent->setCorrespondingElement(use->correspondingElement());" call in
SVGUseElement::expandUseElementsInShadowTree.

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:351: if (toClone->isSVGElement())
ForeignObject isn't allowed with <use>. Do we even need to clone if isSVGElement
is false? If not, several casts can be cleaned up here and elsewhere.

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:353:
toSVGElement(clone.get())->setCorrespondingElement(svgElement->correspondingElement()
? svgElement->correspondingElement() : svgElement);
I'm afraid I don't understand this line. We're setting the clone's
correspondingElement to the original element's correspondingElement? I expected
that we could ASSERT(!toClone->correspondingElement) here.

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:426:
transferEventListenersToShadowTree(m_targetElementInstance.get());
Is this still needed?

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:501: for (RefPtr<Node> child =
target->firstChild(); child; child = child->nextSibling()) {
Why doesn't the SVGElement traversal work here?

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:587: cloneChildrenAndAssociate(use,
cloneParent.get());
expandUseElementsInShadowTree already handles recursively calling itself on all
of its children. Do we need to call this clone children function at all, or can
that logic (just the clone and setCorrespondingElement) be moved here?

[kouhei (in TOK), 2014-06-04 04:28:45]

Thanks a lot for working on this!

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:509: if (instancePtr->isSVGElement()) {
Why do we need this if condition?

[rwlbuis, 2014-06-04 15:02:25]

On 2014/06/04 04:28:45, kouhei wrote:
> Thanks a lot for working on this!
> 
>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> File Source/core/svg/SVGUseElement.cpp (right):
> 
>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:509: if (instancePtr->isSVGElement()) {
> Why do we need this if condition?

We have to deal with SVG Elements and Text nodes. And only if we are dealing
with an SVG Element do we need to go into cloning the children.

[rwlbuis, 2014-06-04 15:11:14]

On 2014/06/04 04:07:52, pdr wrote:
> Epic patch Rob!
> 
> My complaints below are mostly minor. If you don't mind, I'd like to do a
couple
> of rounds on this one to make sure we get it right.
> 
>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGElem...
> File Source/core/svg/SVGElement.cpp (right):
> 
>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGElem...
> Source/core/svg/SVGElement.cpp:113: if (correspondingElement() &&
> correspondingElement()->instancesForElement().contains(this))
> Can we make instancesForElement protected and move the contains logic into
> removeInstanceMapping?

Unfortunately the animation code needs access so I don't think we can make it
protected (I assume we don't want friend declarations).
I tried to put the contains logic into removeInstanceMapping.

>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGElem...
> File Source/core/svg/SVGElement.h (right):
> 
>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGElem...
> Source/core/svg/SVGElement.h:52: void attach(const AttachContext&) OVERRIDE;
> Should these be virtual?

Yep, fixed.

>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> File Source/core/svg/SVGUseElement.cpp (left):
> 
>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:529: // A general description from the SVG
> spec, describing what buildInstanceTree() actually does.
> I think this comment was useful. Could we update it instead of removing it?

Sure once I can think of something useful to say there :) The quote is not
applicable as it meant for the instance tree from 1.1, that is why I removed it.

>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> File Source/core/svg/SVGUseElement.cpp (right):
> 
>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:346: static void
> cloneChildrenAndAssociate(Node* toClone, PassRefPtr<Node> clone);
> I'm not sure this is needed (see comments in expandUseElementsInShadowTree).
> 
> If it is needed, can we use recursion (having cloneChildrenAndAssociate call
> itself) to avoid needing two functions here? It may be possible to refactor
this
> a bit to not need the
> "cloneParent->setCorrespondingElement(use->correspondingElement());" call in
> SVGUseElement::expandUseElementsInShadowTree.

I removed the helper.

>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:351: if (toClone->isSVGElement())
> ForeignObject isn't allowed with <use>. Do we even need to clone if
isSVGElement
> is false? If not, several casts can be cleaned up here and elsewhere.

We have to deal with either Text nodes or SVG elements. I hope you like the new
code better...

>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:353:
>
toSVGElement(clone.get())->setCorrespondingElement(svgElement->correspondingElement()
> ? svgElement->correspondingElement() : svgElement);
> I'm afraid I don't understand this line. We're setting the clone's
> correspondingElement to the original element's correspondingElement? I
expected
> that we could ASSERT(!toClone->correspondingElement) here.

I think it is needed but will investigate more.

>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:426:
> transferEventListenersToShadowTree(m_targetElementInstance.get());
> Is this still needed?

I think so. Though I suppose it can be done in cloneAndAssociate too.

>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:501: for (RefPtr<Node> child =
> target->firstChild(); child; child = child->nextSibling()) {
> Why doesn't the SVGElement traversal work here?

We have to deal with SVG Elements and Text nodes.

>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:587: cloneChildrenAndAssociate(use,
> cloneParent.get());
> expandUseElementsInShadowTree already handles recursively calling itself on
all
> of its children. Do we need to call this clone children function at all, or
can
> that logic (just the clone and setCorrespondingElement) be moved here?

Fixed.

[rwlbuis, 2014-06-04 18:38:45]

On 2014/06/04 15:11:14, rwlbuis wrote:
> >
>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> > Source/core/svg/SVGUseElement.cpp:353:
> >
>
toSVGElement(clone.get())->setCorrespondingElement(svgElement->correspondingElement()
> > ? svgElement->correspondingElement() : svgElement);
> > I'm afraid I don't understand this line. We're setting the clone's
> > correspondingElement to the original element's correspondingElement? I
> expected
> > that we could ASSERT(!toClone->correspondingElement) here.
> 
> I think it is needed but will investigate more.

The problem was in handling the already cloned elements for use/symbol, ie. its
children. What we did was replace them with <g>/<svg> and then
clone and associate them again. Since these are actually elements that are now
in the cloned tree, the corresponding element should not be lost,
hence the complexity of that line.
I noticed integrating the other CL fixes it (just move instead of clone) fixes
it:

https://codereview.chromium.org/278683003/

> >
>
https://codereview.chromium.org/272523002/diff/370001/Source/core/svg/SVGUseE...
> > Source/core/svg/SVGUseElement.cpp:426:
> > transferEventListenersToShadowTree(m_targetElementInstance.get());
> > Is this still needed?
> 
> I think so. Though I suppose it can be done in cloneAndAssociate too.

The latest CL has that change.

I now traverse over SVGElement instead of Node where possible in latest CL.

[pdr., 2014-06-05 04:40:01]

This is looking fantastic. I think we're pretty close to landing.

@Kouhei, could you please double-check the Oilpan type changes in this patch?

@fmalita, @schenney, could you please review this as well?

https://codereview.chromium.org/272523002/diff/430001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/272523002/diff/430001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1085: crbug.com/313438
svg/custom/use-instanceRoot-with-use-removed.svg [ Skip ]
Lets just remove these instanceRoot tests.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:347: {
Can you assert that toClone is a text or svg node?

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:354:
data->eventListenerMap.copyEventListenersNotCreatedFromMarkupToTarget(svgElement);
What are these events for? I thought that we wouldn't need to do this.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:581: for (Node* child = use->firstChild();
child;) {
This doesn't look right. I think you just want:

for (Node* child = use->firstChild(); child; child = child->nextSibling()
    cloneParent->appendChild(child);

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:616: for (RefPtr<SVGElement> sibling =
Traversal<SVGElement>::nextSibling(*element); sibling; sibling =
Traversal<SVGElement>::nextSibling(*sibling)) {
I think you want:
RefPtr<SVGElement> sibling = Traversal<SVGElement>::firstChild(*element)

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:648: for (Node* child = element->firstChild();
child; ) {
Same issue with this loop as the one above. I don't think the oldChild part is
necessary.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:671: for (RefPtr<SVGElement> sibling =
Traversal<SVGElement>::nextSibling(*element); sibling; sibling =
Traversal<SVGElement>::nextSibling(*sibling))
Should the first part of the foor lop be firstChild and not nextSibling as well?

[kouhei (in TOK), 2014-06-05 05:41:36]

(+cc: oilpan-reviews)

lgtm. I love this patch. The SVGElementInstance tree was a headache in oilpan
transition, and I'm very happy we don't need it anymore.

I addressed some life time issues, but they are all optional, I'm happy to fix
them while migrating to oilpan.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:381: RefPtr<Element> newChild =
target->cloneElementWithoutChildren();
Oilpan nit: RefPtrWillBeRawPtr

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:495: for (RefPtr<Node> child =
target->firstChild(); child; child = child->nextSibling()) {
Can this be Node*? I don't think a strong ref is needed here.
If we do need a strong ref, oilpan nit: RefPtrWillBeRawPtr

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:593: RefPtr<Node> newChild =
cloneNodeAndAssociate(target);
oilpan: RefPtrWillBeRawPtr

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:623: for (RefPtr<SVGElement> child =
Traversal<SVGElement>::firstChild(*element); child; child =
Traversal<SVGElement>::nextSibling(*child)) {
I think you can also use SVGElement* here.
If not, oilpan nit: RefPtrWillBeRawPtr.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:671: for (RefPtr<SVGElement> sibling =
Traversal<SVGElement>::nextSibling(*element); sibling; sibling =
Traversal<SVGElement>::nextSibling(*sibling))
On 2014/06/05 04:40:01, pdr wrote:
> Should the first part of the foor lop be firstChild and not nextSibling as
well?

Ditto: RefPtr<SVGElement> -> SVGElement*

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:675: for (RefPtr<SVGElement> child =
Traversal<SVGElement>::firstChild(*element); child; child =
Traversal<SVGElement>::nextSibling(*child))
Ditto.

[haraken, 2014-06-05 05:48:59]

This CL doesn't have any complexity in terms of oilpan, so I think you can just
check if the CL builds with enable_oilpan=1.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:346: static PassRefPtr<Node>
cloneNodeAndAssociate(Node* toClone)

PassRefPtrWillBeRawPtr

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:348: RefPtr<Node> clone =
toClone->cloneNode(false);

RefPtrWillBeRawPtr

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:607: RefPtr<SVGElement>
replacingElement(cloneParent.get());

RefPtrWillBeRawPtr

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:616: for (RefPtr<SVGElement> sibling =
Traversal<SVGElement>::nextSibling(*element); sibling; sibling =
Traversal<SVGElement>::nextSibling(*sibling)) {

RefPtrWillBeRawPtr

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:662: RefPtr<SVGElement>
replacingElement(svgElement.get());

RefPtrWillBeRawPtr

[esprehn, 2014-06-05 06:29:15]

The copyEventListenersNotCreatedFromMarkupToTarget call looks pretty scary,
doesn't that mean the |this| value in the event handler will leak a reference
into the UA shadow tree of the <use> ?

<g id="x">
  <image xlink:href="..."/>
</g>
rect.addEventListener("load", function() {
  // Isn't this in the UA shadow root now??
  console.log(this.parentNode);
}, true);
<use xlink:href="#x"/>

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:349: if (clone->isSVGElement()) {
Early return.

if (!clone->isSVGElement())
  return clone.release();

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:354:
data->eventListenerMap.copyEventListenersNotCreatedFromMarkupToTarget(svgElement);
This doesn't look safe, I think you leak access to the user agent shadow root
tree with the event listener here.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:356: for (Node* n = toClone->firstChild(); n
&& !exceptionState.hadException(); n = n->nextSibling())
node =, don't abbreviate.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:377: ShadowRoot* shadowTreeRootElement =
userAgentShadowRoot();
ShadowRoot& root = ensureUserAgentShadowRoot();

get rid of the ASSERT.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:407:
expandUseElementsInShadowTree(m_targetElementInstance.get(), foundProblem);
Use a return value? This foundProblem thing is weird.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:474: void
SVGUseElement::buildShadowTree(SVGElement* target, SVGElement* targetInstance,
bool& foundProblem, bool foundUse)
Use a return value, or pass a structure. Don't use the bool& and bool stuff.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:501: Node* instancePtr = newChild.get();
Get rid of this local var, also don't use Ptr.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:503: if (instancePtr->isSVGElement()) {
newChild->isSVGElement()

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:505: buildShadowTree(toSVGElement(child),
toSVGElement(instancePtr), foundProblem, foundUse);
toSVGElement(newChild.get())

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:506: if (foundProblem)
I think you want to pass a Context structure that has the foundProblem and
foundUse bits on it instead.

https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:762: instanceTreeIsLoading(element);
You're missing a return or something here, this does nothing since it ignores
the return value.

I'm pretty sure this was supposed to be:

if (element->hasChildren() && instanceTreeIsLoading(element))
  return true;

[rwlbuis, 2014-06-05 14:38:22]

On 2014/06/05 05:48:59, haraken wrote:
> This CL doesn't have any complexity in terms of oilpan, so I think you can
just
> check if the CL builds with enable_oilpan=1.

Right, I just trigger the oilpan debug build for linux.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> File Source/core/svg/SVGUseElement.cpp (right):
> 
>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:346: static PassRefPtr<Node>
> cloneNodeAndAssociate(Node* toClone)
> 
> PassRefPtrWillBeRawPtr
> 
>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:348: RefPtr<Node> clone =
> toClone->cloneNode(false);
> 
> RefPtrWillBeRawPtr
> 
>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:607: RefPtr<SVGElement>
> replacingElement(cloneParent.get());
> 
> RefPtrWillBeRawPtr
> 
>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:616: for (RefPtr<SVGElement> sibling =
> Traversal<SVGElement>::nextSibling(*element); sibling; sibling =
> Traversal<SVGElement>::nextSibling(*sibling)) {
> 
> RefPtrWillBeRawPtr
> 
>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:662: RefPtr<SVGElement>
> replacingElement(svgElement.get());
> 
> RefPtrWillBeRawPtr

Thanks! I fixed it locally and will put up a new patch.

[rwlbuis, 2014-06-05 16:25:21]

>
https://codereview.chromium.org/272523002/diff/430001/LayoutTests/TestExpecta...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/272523002/diff/430001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:1085: crbug.com/313438
> svg/custom/use-instanceRoot-with-use-removed.svg [ Skip ]
> Lets just remove these instanceRoot tests.

Agreed, done.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> File Source/core/svg/SVGUseElement.cpp (right):
> 
>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:347: {
> Can you assert that toClone is a text or svg node?

Actually I noticed some tests use html elements so it won't work.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:354:
>
data->eventListenerMap.copyEventListenersNotCreatedFromMarkupToTarget(svgElement);
> What are these events for? I thought that we wouldn't need to do this.

Seems legacy and not needed anymore, removed.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:581: for (Node* child = use->firstChild();
> child;) {
> This doesn't look right. I think you just want:
> 
> for (Node* child = use->firstChild(); child; child = child->nextSibling()
>     cloneParent->appendChild(child);

Careful! :) After appending child->nextSibling is set to 0. I changed it a bit
so hopefully it is more clear what it is doing.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:616: for (RefPtr<SVGElement> sibling =
> Traversal<SVGElement>::nextSibling(*element); sibling; sibling =
> Traversal<SVGElement>::nextSibling(*sibling)) {
> I think you want:
> RefPtr<SVGElement> sibling = Traversal<SVGElement>::firstChild(*element)

Still investigating, potentially we are doing too much there, see the next loop.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:648: for (Node* child =
element->firstChild();
> child; ) {
> Same issue with this loop as the one above. I don't think the oldChild part is
> necessary.

See above.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:671: for (RefPtr<SVGElement> sibling =
> Traversal<SVGElement>::nextSibling(*element); sibling; sibling =
> Traversal<SVGElement>::nextSibling(*sibling))
> Should the first part of the foor lop be firstChild and not nextSibling as
well?

See above.

[rwlbuis, 2014-06-05 19:21:04]

On 2014/06/05 06:29:15, esprehn wrote:
> The copyEventListenersNotCreatedFromMarkupToTarget call looks pretty scary,
> doesn't that mean the |this| value in the event handler will leak a reference
> into the UA shadow tree of the <use> ?
> 
> <g id="x">
>   <image xlink:href="..."/>
> </g>
> rect.addEventListener("load", function() {
>   // Isn't this in the UA shadow root now??
>   console.log(this.parentNode);
> }, true);
> <use xlink:href="#x"/>

Good point, I do wonder if we already have this leaking problem without my
patch, likely needs more testing.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> File Source/core/svg/SVGUseElement.cpp (right):
> 
>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:349: if (clone->isSVGElement()) {
> Early return.
> 
> if (!clone->isSVGElement())
>   return clone.release();

Fixed.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:354:
>
data->eventListenerMap.copyEventListenersNotCreatedFromMarkupToTarget(svgElement);
> This doesn't look safe, I think you leak access to the user agent shadow root
> tree with the event listener here.

I am not using it anymore.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:356: for (Node* n = toClone->firstChild(); n
> && !exceptionState.hadException(); n = n->nextSibling())
> node =, don't abbreviate.

Fixed.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:377: ShadowRoot* shadowTreeRootElement =
> userAgentShadowRoot();
> ShadowRoot& root = ensureUserAgentShadowRoot();
> 
> get rid of the ASSERT.

Fixed.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:407:
> expandUseElementsInShadowTree(m_targetElementInstance.get(), foundProblem);
> Use a return value? This foundProblem thing is weird.

Fixed.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:474: void
> SVGUseElement::buildShadowTree(SVGElement* target, SVGElement* targetInstance,
> bool& foundProblem, bool foundUse)
> Use a return value, or pass a structure. Don't use the bool& and bool stuff.

PArtly fixed, I don't mind the foundUse so I kept it in.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:501: Node* instancePtr = newChild.get();
> Get rid of this local var, also don't use Ptr.

Fixed.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:503: if (instancePtr->isSVGElement()) {
> newChild->isSVGElement()
> 
>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:505: buildShadowTree(toSVGElement(child),
> toSVGElement(instancePtr), foundProblem, foundUse);
> toSVGElement(newChild.get())
> 
>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:506: if (foundProblem)
> I think you want to pass a Context structure that has the foundProblem and
> foundUse bits on it instead.

See above.

>
https://codereview.chromium.org/272523002/diff/430001/Source/core/svg/SVGUseE...
> Source/core/svg/SVGUseElement.cpp:762: instanceTreeIsLoading(element);
> You're missing a return or something here, this does nothing since it ignores
> the return value.
> 
> I'm pretty sure this was supposed to be:
> ixe
> if (element->hasChildren() && instanceTreeIsLoading(element))
>   return true;

Excellent find! I wonder how long that has been there.... Fixed.

[esprehn, 2014-06-05 20:57:25]

lgtm, couple nits to fix before landing.

https://codereview.chromium.org/272523002/diff/490001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/272523002/diff/490001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:113: if (correspondingElement())
Save in a local in the if()

https://codereview.chromium.org/272523002/diff/490001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:120: if (correspondingElement())
There's a hidden branch in there, I'd suggest:

if (SVGElement* element = correspondingElement())
  element->mapInstanceToElement(this);

https://codereview.chromium.org/272523002/diff/490001/Source/core/svg/SVGUseE...
File Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/272523002/diff/490001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:346: static PassRefPtrWillBeRawPtr<Node>
cloneNodeAndAssociate(Node* toClone)
toClone should be a reference.

https://codereview.chromium.org/272523002/diff/490001/Source/core/svg/SVGUseE...
Source/core/svg/SVGUseElement.cpp:748: if (element->hasChildren() &&
instanceTreeIsLoading(element))
We should probably write a test for this, but it's fine to land the fix for now.

[rwlbuis, 2014-06-05 23:25:42]

On 2014/06/05 20:57:25, esprehn wrote:
> lgtm, couple nits to fix before landing.

Thanks, CL 21 should fix these.
For the ua shadow tree leakage, I have opened
https://codereview.chromium.org/312423002/. Unfortunately it regressed
somewhere in the series of patches for removing SVGElementInstance, but this
patch does not introduce it, it is already there.

[haraken, 2014-06-05 23:28:12]

LGTM in terms of oilpan.

[esprehn, 2014-06-05 23:31:58]

On 2014/06/05 23:25:42, rwlbuis wrote:
> On 2014/06/05 20:57:25, esprehn wrote:
> > lgtm, couple nits to fix before landing.
> 
> Thanks, CL 21 should fix these.
> For the ua shadow tree leakage, I have opened
> https://codereview.chromium.org/312423002/. Unfortunately it regressed
> somewhere in the series of patches for removing SVGElementInstance, but this
> patch does not introduce it, it is already there.

That's a critical security bug, you need to either roll out the patches or fix
it ASAP.

[pdr., 2014-06-06 03:29:38]

On 2014/06/05 23:31:58, esprehn wrote:
> On 2014/06/05 23:25:42, rwlbuis wrote:
> > On 2014/06/05 20:57:25, esprehn wrote:
> > > lgtm, couple nits to fix before landing.
> > 
> > Thanks, CL 21 should fix these.
> > For the ua shadow tree leakage, I have opened
> > https://codereview.chromium.org/312423002/. Unfortunately it regressed
> > somewhere in the series of patches for removing SVGElementInstance, but this
> > patch does not introduce it, it is already there.
> 
> That's a critical security bug, you need to either roll out the patches or fix
> it ASAP.

I agree that this is incorrect but why is it a security bug to expose the
<use>-generated user agent shadow tree?

[rwlbuis, 2014-06-06 15:10:02]

On 2014/06/06 03:29:38, pdr wrote:
> On 2014/06/05 23:31:58, esprehn wrote:
> > On 2014/06/05 23:25:42, rwlbuis wrote:
> > > On 2014/06/05 20:57:25, esprehn wrote:
> > > > lgtm, couple nits to fix before landing.
> > > 
> > > Thanks, CL 21 should fix these.
> > > For the ua shadow tree leakage, I have opened
> > > https://codereview.chromium.org/312423002/. Unfortunately it regressed
> > > somewhere in the series of patches for removing SVGElementInstance, but
this
> > > patch does not introduce it, it is already there.
> > 
> > That's a critical security bug, you need to either roll out the patches or
fix
> > it ASAP.
> 
> I agree that this is incorrect but why is it a security bug to expose the
> <use>-generated user agent shadow tree?

Philip, can I land this patch? I see 3x okay, but just want to be sure because
of the security bug still under discussion and not sure if I addressed all your
issues.

[pdr., 2014-06-06 15:31:56]

On 2014/06/06 15:10:02, rwlbuis wrote:
> On 2014/06/06 03:29:38, pdr wrote:
> > On 2014/06/05 23:31:58, esprehn wrote:
> > > On 2014/06/05 23:25:42, rwlbuis wrote:
> > > > On 2014/06/05 20:57:25, esprehn wrote:
> > > > > lgtm, couple nits to fix before landing.
> > > > 
> > > > Thanks, CL 21 should fix these.
> > > > For the ua shadow tree leakage, I have opened
> > > > https://codereview.chromium.org/312423002/. Unfortunately it regressed
> > > > somewhere in the series of patches for removing SVGElementInstance, but
> this
> > > > patch does not introduce it, it is already there.
> > > 
> > > That's a critical security bug, you need to either roll out the patches or
> fix
> > > it ASAP.
> > 
> > I agree that this is incorrect but why is it a security bug to expose the
> > <use>-generated user agent shadow tree?
> 
> Philip, can I land this patch? I see 3x okay, but just want to be sure because
> of the security bug still under discussion and not sure if I addressed all
your
> issues.

My understanding is that the other bug is not made worse with this patch. As
long as that's correct...
LGTM!

[rwlbuis, 2014-06-06 15:39:27]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-06-06 15:39:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob.buis@samsung.com/272523002/510001

[commit-bot: I haz the power, 2014-06-06 17:04:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11003)

[commit-bot: I haz the power, 2014-06-06 17:49:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 17:49:18]

Could not make sense out of svn commit message.

[pdr., 2014-06-06 17:50:54]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-06-06 17:51:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob.buis@samsung.com/272523002/510001

[commit-bot: I haz the power, 2014-06-06 17:51:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 17:51:42]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1064.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
6f1e7fa0d1495adc948db48f6ec1daaa9f573eb0..04e3f9cf767e546f84ffc5bcd28ad7c8f30215ed
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1064,3 +1064,5 @@ crbug.com/308768
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag
 crbug.com/308768
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html [ Skip ]
 crbug.com/308768 http/tests/xmlhttprequest/xmlhttprequest-data-url.html [ Skip
]
 crbug.com/308768 inspector/network/network-status-non-http.html [ Skip ]
+
+crbug.com/313438 svg/custom/use-instanceRoot-event-bubbling.xhtml [
NeedsRebaseline ]

[pdr., 2014-06-06 17:54:32]

On 2014/06/06 17:49:18, I haz the power (commit-bot) wrote:
> Could not make sense out of svn commit message.

I filed https://code.google.com/p/chromium/issues/detail?id=381770 due to this
strange error.

[rwlbuis, 2014-06-06 18:37:22]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-06-06 18:38:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob.buis@samsung.com/272523002/510001

[commit-bot: I haz the power, 2014-06-06 18:38:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 18:38:35]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1064.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
6f1e7fa0d1495adc948db48f6ec1daaa9f573eb0..04e3f9cf767e546f84ffc5bcd28ad7c8f30215ed
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1064,3 +1064,5 @@ crbug.com/308768
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag
 crbug.com/308768
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html [ Skip ]
 crbug.com/308768 http/tests/xmlhttprequest/xmlhttprequest-data-url.html [ Skip
]
 crbug.com/308768 inspector/network/network-status-non-http.html [ Skip ]
+
+crbug.com/313438 svg/custom/use-instanceRoot-event-bubbling.xhtml [
NeedsRebaseline ]

[pdr., 2014-06-06 18:43:11]

This actually landed as
http://src.chromium.org/viewvc/blink?view=rev&rev=175681. (There is already a
fix up for the CQ bug)

[rwlbuis, 2014-06-06 19:02:17]

On 2014/06/06 18:43:11, pdr wrote:
> This actually landed as
> http://src.chromium.org/viewvc/blink?view=rev&rev=175681. (There is already a
> fix up for the CQ bug)

Thanks!