467498 - Second level of <svg:use> is not live to changes

Status: RESOLVED DUPLICATE of bug 1032124

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

The three stripes (blue, green, and red) should all extend to the bottom.

Based on layout/reftests/svg/use-01-extref.svg, but all references in the reduced testcase are internal.  (Is that the only reftest that relies on multiple levels of <svg:use>?)

Comment 1

[Jesse Ruderman]

Attachment: reference

Comment 2

[Boris Zbarsky [:bzbarsky]]

I suspect it's the only <use>-within-<use> reftest, yes.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the way <use> handles mutations is by setting itself up as the listener on the used node.

So if we have

 <use xlink:href="#x"/>
 <svg id="x"><use xlink:href="#y"/></svg>

And we change an attribute on a kid of id="y", then the second ("inner") <use> will get notified and reclone.  But that reclone fires no mutation notificatons, so there's no notification dispatched to the first (outer) <use>.

Ideally, any time a <use> element reclones all <use> elements that have it as mOriginal will also reclone, no?  That should fix this bug, I would think...  Is that a reasonable approach?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why would that fix this bug? The first <use> does not point directly to the second <use> in your example.

Why doesn't reclone fire a mutation observer? Is it because the cloned nodes are anonymous? I think even changes to anonymous children should cause <use> observers to reclone (and the anonymous children should be cloned, too).

Comment 5

[Boris Zbarsky [:bzbarsky]]

The first use doesn't point to the second use, but the clone of the second use that's in the first use's cloned tree does point to the second use, and if it recloned when the second use reclones that would fix the bug, no?

Recloning doesn't trigger mutation events because it just uses nsIAnonymousContentCreator, yeah.

Cloning doesn't explicitly clone the anonymous kids right now.  It just clones the other <use>, which then creates its own anonymous kids when its frame is constructed....

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right, OK.

Comment 7

[Robert Longson [:longsonr]]

Testcase seems to work on trunk now.

Comment 8

[Jesse Ruderman]

Reftest: http://hg.mozilla.org/mozilla-central/rev/2f0055db840c

Comment 9

[Daniel Holbert [:dholbert]]

This issue is actually still broken -- we only get lucky on the testcase posted here because it does all the JS work before our first reflow, I think.

If you change the onload handler to use a setTimeout (particularly with a timeout-length longer than a few milliseconds), then it re-breaks the test.

Reopening.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: testcase with setTimeout added

here's the testcase with a setTimeout added in the onload handler.  It's broken on every single load for me, in both today's nightly and yesterday's nightly. (yesterday's nightly is relevant because it's from before this reftest started sporadically failing, as tracked in bug 571036)

Comment 11

[Robert Longson [:longsonr]]

I think we should back out the testcase in the tree since it isn't testing what we think it should.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to comment #11)

Well, the test *is* testing *something* that didn't used to work but now does work.  It would be useful to know if that part of this issue regresses.

As I noted in comment 9, the test does currently only pass if the dynamic modifications occurring synchronously -- we could preserve that synchronicity (after onload becomes async in bug 552938) by using an inline <script> tag towards the end of the document instead of an onload handler, right?

Comment 13

[Robert Longson [:longsonr]]

Attachment: patch

So if we have use1->use2->content and the content changes we're going to get use2 to send a modification event as if it was itself modified that use1 picks up using its existing observer infrastructure.

Comment 14

[Robert Longson [:longsonr]]

Please ignore the layout/svg/base/src/nsSVGUseFrame.cpp change I don't need that, the reclone is enough to cause a complete refresh.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 634014 [details] [diff] [review]
patch

>diff --git a/content/svg/content/src/nsSVGSVGElement.h b/content/svg/content/src/nsSVGSVGElement.h
>--- a/content/svg/content/src/nsSVGSVGElement.h
>+++ b/content/svg/content/src/nsSVGSVGElement.h
>@@ -168,18 +168,16 @@ public:
>-  // Copy our width or height to the target
>-  void SyncWidthOrHeight(nsIAtom* aName, nsSVGElement *aTarget) const;

I'm confused... nsSVGUseFrame.cpp still calls this method, like so:
> 128       static_cast<nsSVGUseElement*>(mContent)->SyncWidthOrHeight(aAttribute);

Is that call no longer needed?  I'd guess that this patch wouldn't compile as long as that line remains.

(Also: it looks like we don't have any remaining callers for the other "SyncWidthOrHeight" impl, which lives on nsSVGSVGElement. Somewhat unrelated, but it looks like that impl can go away too?)

Comment 16

[Robert Longson [:longsonr]]

We're keeping the still-used nsSVGUseElement::SyncWidthOrHeight implemented but we can ditch the unused nsSVGSVGElement::SyncWidthOrHeight

Comment 17

[Daniel Holbert [:dholbert]]

Oh, of course -- sorry, I glossed over the filename and assumed the removed chunk was from nsSVGUseElement.  Sorry / thanks for the correction. :)

Comment 18

[Robert Longson [:longsonr]]

And one reason for removing the unused method is that it's quite confusing ;-)

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 634014 [details] [diff] [review]
patch

Yup. :) Thanks for that.

So just to make sure I understand: this works because our  TriggerReclone() makes us call "nsSVGUseElement::AttributeChanged()" on any <use> elements that target us, which hits their own "TriggerReclone()" method (etc.)?

Assuming I'm understanding that correctly -- does this cause problems if we have a <use> cycle, like so:

  <use id="a" xlink:href="#b"/>
  <use id="b" xlink:href="#a"/>
  <script>
    // Some script that tweaks "a" and causes a TriggerReclone call
  </script>

It seems like this could cause endless ping-ponging.  Does it?

(Even if it we're safe against that for some reason, I'd still like this patch to include a test like that -- either as a crashtest or a trivial reftest of some sort -- so that a future patch won't combine with this to accidentally open ourselves up to that in the future.)

Comment 20

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #19)
>
> So just to make sure I understand: this works because our  TriggerReclone()
> makes us call "nsSVGUseElement::AttributeChanged()" on any <use> elements
> that target us, which hits their own "TriggerReclone()" method (etc.)?

use1->use2->content

use2's nsSVGUseElement::AttributeChanged picks up a change in content then calls TriggerReclone which now calls nsNodeUtils::AttributeChanged this is picked up by use1's nsSVGUseElement::AttributeChanged as an attribute change and that reclones it's contents which is use2.

> 
> Assuming I'm understanding that correctly -- does this cause problems if we
> have a <use> cycle, like so:
> 
>   <use id="a" xlink:href="#b"/>
>   <use id="b" xlink:href="#a"/>
>   <script>
>     // Some script that tweaks "a" and causes a TriggerReclone call
>   </script>
> 
> It seems like this could cause endless ping-ponging.  Does it?

nsSVGUseElement::CreateAnonymousContent stops this as it checks for loops and refuses to do carry on cloning.

> 
> (Even if it we're safe against that for some reason, I'd still like this
> patch to include a test like that -- either as a crashtest or a trivial
> reftest of some sort -- so that a future patch won't combine with this to
> accidentally open ourselves up to that in the future.)

I can turn your snippet into a crashtest.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 634014 [details] [diff] [review]
patch

(In reply to Robert Longson from comment #20)
> nsSVGUseElement::CreateAnonymousContent stops this as it checks for loops
> and refuses to do carry on cloning.

Awesome. I thought we must already have protection against that... Glad we do.

> I can turn your snippet into a crashtest.

That would be awesome, thanks! r=me with that.

Comment 22

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4dbc71a506

Comment 23

[Robert Longson [:longsonr]]

backed out the change that fixes this bug as content/svg/content/src/crashtests/405639-1.svg intermittently fails on Windows.

Comment 24

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd3a8bc49325 is the backout

Comment 25

[Daniel Holbert [:dholbert]]

Here are some logs of the crashes in 405639-1.svg, for reference:
WinOpt:  https://tbpl.mozilla.org/php/getParsedLog.php?id=12795462&tree=Mozilla-Inbound
WinDbg:  https://tbpl.mozilla.org/php/getParsedLog.php?id=12795487&tree=Mozilla-Inbound
OSX Dbg: https://tbpl.mozilla.org/php/getParsedLog.php?id=12795446&tree=Mozilla-Inbound

Comment 26

[Daniel Holbert [:dholbert]]

It looks like the crashtest failure is something like the ping-ponging that I mentioned in comment 19, BTW -- at least, one of the full logs from my previous comment has a 1024-level backtrace that looks like:

> 1017  xul.dll!nsSVGUseElement::TriggerReclone() [nsSVGUseElement.cpp:16b70566af66 : 445 + 0xf]
> 1018  xul.dll!nsSVGUseElement::AttributeChanged(nsIDocument *,mozilla::dom::Element *,int,nsIAtom *,int) [nsSVGUseElement.cpp:16b70566af66 : 182 + 0x6]
> 1019  xul.dll!nsNodeUtils::AttributeChanged(mozilla::dom::Element *,int,nsIAtom *,int) [nsNodeUtils.cpp:16b70566af66 : 108 + 0x77]
> 1020  xul.dll!nsSVGUseElement::TriggerReclone() [nsSVGUseElement.cpp:16b70566af66 : 445 + 0xf]
> 1021  xul.dll!nsSVGUseElement::AttributeChanged(nsIDocument *,mozilla::dom::Element *,int,nsIAtom *,int) [nsSVGUseElement.cpp:16b70566af66 : 182 + 0x6]
> 1022  xul.dll!nsNodeUtils::AttributeChanged(mozilla::dom::Element *,int,nsIAtom *,int) [nsNodeUtils.cpp:16b70566af66 : 108 + 0x77]
> 1023  xul.dll!nsSVGUseElement::TriggerReclone() [nsSVGUseElement.cpp:16b70566af66 : 445 + 0xf]
> 1024  xul.dll!nsSVGUseElement::AttributeChanged(nsIDocument *,mozilla::dom::Element *,int,nsIAtom *,int) [nsSVGUseElement.cpp:16b70566af66 : 182 + 0x6]

Interestingly, all of the TriggerReclone()/AttributeChanged() calls are for the same element (at address 16b70566af66) -- it must be registering as its own listener somehow, I guess?

Comment 27

[Robert Longson [:longsonr]]

Attachment: new patch

Comment 28

[Robert Longson [:longsonr]]

I've sent this to try. The idea is to send the attribute changed later and only when the listener for updates to the current use element are disabled.

Comment 31

[Thomas Brierley]

Wow... i accidentally made a dup of this. Disappointed to see it is now 6 years old.

Seeing as it looks like this is not going to be fixed any time soon, i was forced to come up with a generalised workaround, for others who are interested:

It involves triggering redraws by modifying an attribute in any way on affected use elements. You could be clever about this and walk the references recursively picking up all the elements related to the target you are modifying but the overhead likely exceeds the simplicity of doing a basic global 'use' query selection.

Ether way you should only do your query when new use elements might be injected into the DOM, then save the reference to the use elements for updating in your animation step.

I've put together a fix at the gist bellow... it gives you a constructor that returns an object with two methods:

"gather" and "redraw"

gather does your query selection, redraw is what you want to call in your animation draw steps.

It takes two arguments "root" and "auto"

If your feeling lazy and want a quick drop in fix then set the auto argument to true, that will set up a redraw loop for you... for performance reasons this does not also call the gather method automatically. Ideally you should call the redraw method in your animation step callback.

SvgUseFix:
https://gist.github.com/anonymous/d006a644618033ffda2b

Example:
http://jsbin.com/piricejuvo/5/edit

Comment 33

[Markus Stange [:mstange]]

Robert, how do you feel about picking this bug up again? Do you still have the patch that you sent to try in comment 28?

Comment 34

[Robert Longson [:longsonr]]

It caused hangs due to infinite loops. I'd have to figure out how to resolve that. I'd be more inclined to fix use not to clone which would likely fix this bug as fall out. I almost had that implemented but display list conversion broke it.

Comment 35

[Markus Stange [:mstange]]

(In reply to Robert Longson from comment #34)
> I almost had that implemented but display list
> conversion broke it.

Are those patches in a bug somewhere? Maybe I can take it over the finish line.

Comment 38

[Robert Longson [:longsonr]]

Thanks Violet.