[svg] Update the <use> shadowtree if a referenced element is removed.

[svg] Update the <use> shadowtree if a referenced element is removed.

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

[Erik Dahlström (inactive), 2015-04-16 15:04:00]

ed@opera.com changed reviewers:
+ fs@opera.com, pdr@chromium.org

[pdr., 2015-04-17 04:57:25]

https://codereview.chromium.org/1087653004/diff/1/Source/core/svg/SVGUseEleme...
File Source/core/svg/SVGUseElement.cpp (right):

https://codereview.chromium.org/1087653004/diff/1/Source/core/svg/SVGUseEleme...
Source/core/svg/SVGUseElement.cpp:396: addReferenceTo(target);
It's been a while since I've been in SVGUseElement so please forgive some paging
out that occurred.

IIRC, we have two element "map" concepts:
1) the ingoing/outgoing reference machinery used in addReferenceTo.
2) the instances of use elements (instancesForElement, invalidateInstances, etc)

I would have expected this to have been handled through the existing <use>
element's SVGElement::removedFrom which calls invalidateInstances. Do you know
why it isn't?

[Erik Dahlström (inactive), 2015-04-17 13:11:35]

On 2015/04/17 04:57:25, pdr wrote:
>
https://codereview.chromium.org/1087653004/diff/1/Source/core/svg/SVGUseEleme...
> File Source/core/svg/SVGUseElement.cpp (right):
> 
>
https://codereview.chromium.org/1087653004/diff/1/Source/core/svg/SVGUseEleme...
> Source/core/svg/SVGUseElement.cpp:396: addReferenceTo(target);
> It's been a while since I've been in SVGUseElement so please forgive some
paging
> out that occurred.
> 
> IIRC, we have two element "map" concepts:
> 1) the ingoing/outgoing reference machinery used in addReferenceTo.
> 2) the instances of use elements (instancesForElement, invalidateInstances,
etc)
> 
> I would have expected this to have been handled through the existing <use>
> element's SVGElement::removedFrom which calls invalidateInstances. Do you know
> why it isn't?

The notification in SVGElement::removedFrom comes in after the node has been
removed from the document tree, and there's an early exit in
SVGElement::invalidateInstances for that case.

So an alternative fix might be to check if the removed element had instances
even if it wasn't in the document, and if it did, check if those instances still
exist in the document and invalidate the corresponding shadowtree if so.

[pdr., 2015-04-18 03:38:16]

pdr@chromium.org changed reviewers:
+ rwlbbuis@samsung.com

[pdr., 2015-04-18 03:41:11]

+cc Rob Buis who also worked in this area in case he has thoughts.

On 2015/04/17 at 13:11:35, ed wrote:
> On 2015/04/17 04:57:25, pdr wrote:
> >
https://codereview.chromium.org/1087653004/diff/1/Source/core/svg/SVGUseEleme...
> > File Source/core/svg/SVGUseElement.cpp (right):
> > 
> >
https://codereview.chromium.org/1087653004/diff/1/Source/core/svg/SVGUseEleme...
> > Source/core/svg/SVGUseElement.cpp:396: addReferenceTo(target);
> > It's been a while since I've been in SVGUseElement so please forgive some
paging
> > out that occurred.
> > 
> > IIRC, we have two element "map" concepts:
> > 1) the ingoing/outgoing reference machinery used in addReferenceTo.
> > 2) the instances of use elements (instancesForElement, invalidateInstances,
etc)
> > 
> > I would have expected this to have been handled through the existing <use>
> > element's SVGElement::removedFrom which calls invalidateInstances. Do you
know
> > why it isn't?
> 
> The notification in SVGElement::removedFrom comes in after the node has been
removed from the document tree, and there's an early exit in
SVGElement::invalidateInstances for that case.
> 
> So an alternative fix might be to check if the removed element had instances
even if it wasn't in the document, and if it did, check if those instances still
exist in the document and invalidate the corresponding shadowtree if so.

I think that makes sense too. We may even be able to move the
invalidateInstances call in SVGElement::removedFrom before Element::removedFrom.

[Erik Dahlström (inactive), 2015-04-23 12:10:26]

On 2015/04/18 03:41:11, pdr (OOO April 18th to 22nd) wrote:
> +cc Rob Buis who also worked in this area in case he has thoughts.
> 
> On 2015/04/17 at 13:11:35, ed wrote:
> > On 2015/04/17 04:57:25, pdr wrote:
> > >
>
https://codereview.chromium.org/1087653004/diff/1/Source/core/svg/SVGUseEleme...
> > > File Source/core/svg/SVGUseElement.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/1087653004/diff/1/Source/core/svg/SVGUseEleme...
> > > Source/core/svg/SVGUseElement.cpp:396: addReferenceTo(target);
> > > It's been a while since I've been in SVGUseElement so please forgive some
> paging
> > > out that occurred.
> > > 
> > > IIRC, we have two element "map" concepts:
> > > 1) the ingoing/outgoing reference machinery used in addReferenceTo.
> > > 2) the instances of use elements (instancesForElement,
invalidateInstances,
> etc)
> > > 
> > > I would have expected this to have been handled through the existing <use>
> > > element's SVGElement::removedFrom which calls invalidateInstances. Do you
> know
> > > why it isn't?
> > 
> > The notification in SVGElement::removedFrom comes in after the node has been
> removed from the document tree, and there's an early exit in
> SVGElement::invalidateInstances for that case.
> > 
> > So an alternative fix might be to check if the removed element had instances
> even if it wasn't in the document, and if it did, check if those instances
still
> exist in the document and invalidate the corresponding shadowtree if so.
> 
> I think that makes sense too. We may even be able to move the
> invalidateInstances call in SVGElement::removedFrom before
Element::removedFrom.

I tried that, but couldn't get it to work properly. I'm a bit unsure about what
assumptions have been made in the code, wrt whether things are in the document
or not, and what order the various calls need to happen in. I can file an issue
for the investigating how/if to reshuffle the method calls, if that's good
enough to proceed with this.

I think that the alternative solution (patchset #2) should be fine (and safe).
I'm aware that this is a slightly scary part of the code (past security issues
and so on). It might be good to run the existing fuzzers on this CL just to make
sure, but I'm not sure how to do that (or if it's even possible to do before
landing the change).

[pdr., 2015-04-23 23:03:04]

On 2015/04/23 at 12:10:26, ed wrote:
> On 2015/04/18 03:41:11, pdr (OOO April 18th to 22nd) wrote:
> > +cc Rob Buis who also worked in this area in case he has thoughts.
> > 
> > On 2015/04/17 at 13:11:35, ed wrote:
> > > On 2015/04/17 04:57:25, pdr wrote:
> > > >
> >
https://codereview.chromium.org/1087653004/diff/1/Source/core/svg/SVGUseEleme...
> > > > File Source/core/svg/SVGUseElement.cpp (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1087653004/diff/1/Source/core/svg/SVGUseEleme...
> > > > Source/core/svg/SVGUseElement.cpp:396: addReferenceTo(target);
> > > > It's been a while since I've been in SVGUseElement so please forgive
some
> > paging
> > > > out that occurred.
> > > > 
> > > > IIRC, we have two element "map" concepts:
> > > > 1) the ingoing/outgoing reference machinery used in addReferenceTo.
> > > > 2) the instances of use elements (instancesForElement,
invalidateInstances,
> > etc)
> > > > 
> > > > I would have expected this to have been handled through the existing
<use>
> > > > element's SVGElement::removedFrom which calls invalidateInstances. Do
you
> > know
> > > > why it isn't?
> > > 
> > > The notification in SVGElement::removedFrom comes in after the node has
been
> > removed from the document tree, and there's an early exit in
> > SVGElement::invalidateInstances for that case.
> > > 
> > > So an alternative fix might be to check if the removed element had
instances
> > even if it wasn't in the document, and if it did, check if those instances
still
> > exist in the document and invalidate the corresponding shadowtree if so.
> > 
> > I think that makes sense too. We may even be able to move the
> > invalidateInstances call in SVGElement::removedFrom before
Element::removedFrom.
> 
> I tried that, but couldn't get it to work properly. I'm a bit unsure about
what assumptions have been made in the code, wrt whether things are in the
document or not, and what order the various calls need to happen in. I can file
an issue for the investigating how/if to reshuffle the method calls, if that's
good enough to proceed with this.
> 
> I think that the alternative solution (patchset #2) should be fine (and safe).
I'm aware that this is a slightly scary part of the code (past security issues
and so on). It might be good to run the existing fuzzers on this CL just to make
sure, but I'm not sure how to do that (or if it's even possible to do before
landing the change).

LGTM

Unfortunately it's difficult to run this through the fuzzers on its own, but
clusterfuzz will quickly tell us once it hits canary :)

[Erik Dahlström (inactive), 2015-04-24 07:58:23]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2015-04-24 07:58:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087653004/20001

[commit-bot: I haz the power, 2015-04-24 09:17:51]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194363