610990 - Regression: SVGPathSegList should allow manipulation of invalid paths

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

STEPS TO REPRODUCE:
 1. Load http://svgtorture.googlecode.com/svn/trunk/automated/test.html

EXPECTED RESULTS: We fail test #9, #11, and no others.
ACTUAL RESULTS: We recently started failing tests #19 & #22:
 19. SVGPathSegList module: Supports insertItemBefore() (2, 1, 3)
 22. SVGPathSegList module: Supports initialize() (2, 1, 3)

Regression range was between the 11/8 & 11/9 nightlies:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0f92c4e81cf3&tochange=09a2c950dae3

Since the failure is in tests for SVG path APIs, this looks most likely to be a regression from bug 522306.

Comment 1

[Daniel Holbert [:dholbert]]

If I click the failing tests, it gives this additional information:

> #19: Supports insertItemBefore()
>   [green] SVGPathSegList has 'insertItemBefore' property
>   [red]   Died on test #2: Index or size is negative or greater than the allowed amount
>   [red]  Expected 3 assertions, but 2 were run

> #22:
>   [green] SVGPathSegList has 'initialize' property
>   [red]   Died on test #2: Index or size is negative or greater than the allowed amount
>   [red]   Expected 3 assertions, but 2 were run

Comment 2

[Daniel Holbert [:dholbert]]

I get this output in the error console, when I reload the test:
> Warning: Unexpected value L100,200 M1,2 L3,4 parsing d attribute.
> Source File: http://svgtorture.googlecode.com/svn/trunk/automated/test.html
...and...
> Warning: Unexpected value L1,2 parsing d attribute.
> Source File: http://svgtorture.googlecode.com/svn/trunk/automated/test.html

Looks like the first one is from test #19 running this script:
>  list.insertItemBefore(makeSeg(100, 200), 0);

and the second one is from test #22 running this script:
>  list.initialize(makeSeg(1, 2));

makeSeg is just a helper function in the test that returns "path.createSVGPathSegLinetoAbs(x, y);"

My initial guess is that what's going on is this:  whenever we serialize the generated segments to create a "d" attribute, we're not creating a "dummy" move command at the beginning ("m0 0"), and so our "d" attribute ends up rejecting our own generated path specification, because it starts with a non-move-command.  (At least in SVG 1.1, path specifications have to start with a move command.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase

Here's a mostly-reduced testcase.  It runs initialize & insertItemBefore on a path's pathSegList.

Trunk behavior: fails -- the list's number of items is "0" after the initialize call & also after the insertItemBefore call. The final "d" attr is L100,200

Firefox 3.6 behavior: PASS -- the list's number of items is "1" after initialize and "2" after insertItemBefore. The final "d" attr is L100,200 L1,2

Opera matches Firefox 3.6.  In all cases, nothing is actually drawn (because the path is invalid at least for the purposes of drawing, without an initial moveto command)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: visual testcase (should show two lines)

Here's a visual testcase with a little more real-world usefulness.  (The previous test and the test on the svg torture site both built paths that wouldn't paint anything, so they were arguably less useful).

This testcase builds up two paths, which both have a description "Move; draw Line".  In one case, we add the "line" segment first &  then insert the move before it; in the other case, we add the "move" segment first & append the line after it.

The first case fails -- it looks like we completely drop the initially-added line segment (since there's no "move" command in our path's pathSegList at that point and we're perhaps overzealously failing too early).

Comment 5

[Jonathan Watt [:jwatt]]

Thanks for filing this, and for all the debugging, dholbert!

Comment 6

[Jonathan Watt [:jwatt]]

The reason this is happening is because whenever the DOM list is changed by script, we call DidChangePathSegList(), which serializes the list, and reparses it. Since the path doesn't start with a valid "M" command, the reparse actually trashes the DOM list.

Fixing bug 609527 in general would fix this bug. Alternatively an easy fix would be to add a guard member to SVGPathSegList which we set/unset around calls to DidChangePathSegList in DOMSVGPathSegList, then in SVGPathData::SetValueFromString just return early if that guard is set.

Comment 7

[Jonathan Watt [:jwatt]]

Another way to fix this would be to stop requiring the first segment in a path to be a moveto segment. That would be my preferred course of action actually. I've sent an email to the SVG WG about that:

http://lists.w3.org/Archives/Public/www-svg/2010Nov/0033.html

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch with better test

Comment 10

[Jonathan Watt [:jwatt]]

In terms of perf, note that SetAttrForSVGDOMChange serializes the nsAttrValue that's passed in, so we only avoid half the current perf hit of the serialize-then-parse circle on SVG DOM changes. There are several reasons for doing this including:

 1) I think this late in the game we should be keeping the code paths
    as similar to what's currently executed (i.e. what SetAttr currently
    does in this case) as we can.

 2) Even in the long run I think we want to call BeforeSetAttr/
    AfterSetAttr as it's unclear that it wont bite us somewhere/
    someday to have diverged from SetAttr on that point.

 3) It would be nice to keep SetAttrForSVGDOMChange as close to SetAttr
    as possible in the long run to make keeping them in sync easier.

 4) Perf improvement is not the issue reported in this bug, although
    the partial improvement that results from the patch is welcome.

Comment 11

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

Comment on attachment 502812 [details] [diff] [review]
patch with better test

I think we should move SetAttrForSVGDOMChange to nsGenericElement and call it SetParsedAttr or something like that. We an also share more code here, probably all the code from SetAttr before it calls ParseAttribute can be moved to a helper function and called from SetParsedAttr too.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch with nsGenericElement::SetParsedAttr

Comment 13

[Jonathan Watt [:jwatt]]

I'm not really in love with the three outparams - refactoring suggestions would be welcome.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 504570 [details] [diff] [review]
patch with nsGenericElement::SetParsedAttr

> @@ -256,16 +263,25 @@ nsAttrValue::SetTo(const nsAttrValue& aO
> +      // XXX under what circumstances do we get here? Are both nsAttrValue
> +      // objects guaranteed to be outlived by the nsSVGElement who's member
> +      // SVGAnimatedPathSegList they're pointing at?!

I don't know :(

In all other cases the nsAttrValue owns the contained value and we clone the value in this function. Is there any way you could do the same here?

That said, I think it is safe. But we'd need to investigate and document.

One option might be to just copy the string value in this case...

> +      // Since [the] SVG path data attribute is never mapped into style, is
> +      // this case even necessary (i.e. ever reached)? The comment for the
> +      // eAtomArray case seems to suggest the answer is "no".
> +      //
> +      // XXX should we actually be comparing objects unlike eCSSStyleRule?

Yeah, we should never get here.

You are changing the call order between nsNodeUtils::AttributeWillChange and ParseAttribute. This is probably not safe given that ParseAttribute often change state element state (which is unfortunate but nevertheless the case).

Looks good otherwise.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, wait, you're not changing that call order. My bad.

Still nervous about the nsAttrValue lifetime issue though.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Oh, wait, you're not changing that call order. My bad.

Well...  With the new SetParsedAttribute code, are we guaranteed that when AttributeWillChange is called GetAttr still returns the old (pre-change) value?

Comment 17

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

No.

I guess we need to do more invasive surgery here, so that our SVG code can do some kind of "BeginChangingAttribute"/"EndChangingAttribute" protocol around its modifications to the underlying data.

Probably we should fix this regression in a different way for FF4 and then think about what kind of DOM API restructuring we need here.

Comment 18

[Boris Zbarsky [:bzbarsky]]

The other option is to not modify objects in-place and instead to clone-on-modification.  That's what inline style does, to deal with these issues.

Comment 19

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

Another expedient option, which jwatt suggested, is to simply serialize the value to a string and always store that in the nsAttrValue. So when the value changes, we can get the string for the old value from the nsAttrValue.

Cloning the SVG data on modification isn't the ultimate solution here because we can be dealing with multi-megabyte path data objects. If someone loops over the path components modifying them (or just builds a really big path by appending to the component list), we'll get O(N^2) behavior due to the clone.

Comment 20

[Jonathan Watt [:jwatt]]

(In reply to comment #14)
> 
> > @@ -256,16 +263,25 @@ nsAttrValue::SetTo(const nsAttrValue& aO
> > +      // XXX under what circumstances do we get here? Are both nsAttrValue
> > +      // objects guaranteed to be outlived by the nsSVGElement who's member
> > +      // SVGAnimatedPathSegList they're pointing at?!
> 
> One option might be to just copy the string value in this case...

[Replying to this for future reference when I/we will no doubt be looking back on this code when thinking about fixing the general perf issues with modifying SVG element members.]

I don't think we want to just copy the string value and not take over the pointer to the SVGAnimatedPathSegList. I assume we'd get here from under DidChangePathSegList where aOther will be the nsAttrValue passed into SetParsedAttr by DidChangePathSegList, and the nsAttrValue that this SetTo call is occurring on is the nsAttrValue being stored in mAttrsAndChildren. In that case the nsAttrValue in mAttrsAndChildren wouldn't have a reference to the SVGAnimatedPathSegList which would defeat the point of storing SVGAnimatedPathSegList references in nsAttrValues.

Comment 21

[Jonathan Watt [:jwatt]]

(In reply to comment #16)
> With the new SetParsedAttribute code, are we guaranteed that when
> AttributeWillChange is called GetAttr still returns the old (pre-change) value?

Good catch!

(In reply to comment #17)
> No.

Well, "no" if the SVGAnimatedPathSegList has been modified via the SVG DOM since the attribute was last set by the parser/a setAttribute call, but "yes" otherwise since nsSVGElement::ParseAttribute passes the serialized value to the nsAttrValue which stores it using SetMiscAtomOrString.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Good catch!

Add tests?  ;)

I need to think about comment 19; more on that tomorrow.

Comment 23

[Jonathan Watt [:jwatt]]

Attachment: patch just storing a string

In this approach we always store the serialization in the nsAttrValue so that GetAttr will always return the old value under AttributeChanged. Since we're always storing the serialization in nsAttrValue, there's actually no point in it having a reference to the SVGAnimatedPathSegList, so it now doesn't. This neatly sidesteps the nsAttrValue vs element lifetime problem.

Comment 24

[Jonathan Watt [:jwatt]]

Hmm. There is one problem with the SetAttr changes that we missed, which is that there's a |return NS_OK| line in the code that was moved from SetAttr to SetAttrInternal that is supposed to trigger an early return from SetAttr. As it stands, SetAttr doesn't know when SetAttrInternal returns that NS_OK, so it doesn't know that it too should return early. Jonas, do you have a preferred way to handle that? I'm not sure there's a particularly appropriate error code I can return to signal "return early, but return NS_OK instead of what I'm returning".

Comment 25

[Jonathan Watt [:jwatt]]

(In reply to comment #22)
> Add tests?  ;)

Sure. I'll add a test in the patch to fix comment 24.

Comment 26

[Jonathan Watt [:jwatt]]

I've added the following to the mochitest:

+  d = 'M0,0 L12,34'
+  path.setAttribute('d', d);
+  function check_old_value(e) {
+    is(e.target, path, 'check mutation event is for expected node');
+    is(e.attrName, 'd', 'check mutation event is for expected attribute');
+    is(e.prevValue, d, 'check old attribute value is correctly reported');
+    isnot(e.newValue, d, 'check attribute value has changed');
+  }
+  path.addEventListener('DOMAttrModified', check_old_value, false);
+  list.getItem(1).y = 35;
+  path.removeEventListener('DOMAttrModified', check_old_value, false);

Comment 27

[Boris Zbarsky [:bzbarsky]]

Also worth adding styling tests, since one obvious way to make mutation events work would be to just pass in the old string... but that won't work for restyles.  :(

Comment 28

[Jonathan Watt [:jwatt]]

Attachment: patch 3: now with extra test, and addressing comment 24

This is one way to address comment 24. This patch is still working its way thorough Try, but it's looking good so far.

Comment 29

[Jonathan Watt [:jwatt]]

Passed Try fine, BTW.

Comment 30

[Boris Zbarsky [:bzbarsky]]

OK, so the point is that we still have a serialize/reparse cycle, but GetAttr
just gets the string instead of serializing the object, so there are no
ordering issues of the sort I was worried about, right?

Comment 31

[Jonathan Watt [:jwatt]]

(In reply to comment #30)
> OK, so the point is that we still have a serialize/reparse cycle,

Actually we now only have the serialization step because we avoid the reparse by using SetParsedAttr which doesn't call ParseAttribute.

> but GetAttr just gets the string instead of serializing the object,

Right.

> so there are no ordering issues of the sort I was worried about, right?

Right.

Comment 32

[Jonathan Watt [:jwatt]]

Attachment: patch without NS_NOTHING_TO_DO and without code sharing

Jonas, is this more palatable? I'd like this to not get stuck and miss the Tuesday deadline because the code sharing ugly, so how about we just do this for now and I file a separate bug for sharing code (or not) to be fixed post FF4?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Er... were sicking's comments outside this bug somewhere?  The sharing code approach seemed way better to me, personally.  If Jonas is just swamped, I can review as needed.

Comment 34

[Jonathan Watt [:jwatt]]

Yes, Jonas and I were talking in #developers.

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 508225 [details] [diff] [review]
patch without NS_NOTHING_TO_DO and without code sharing

Yeah, I like this better. Though change SetParsedAttr to not take a namespaceID or prefix as I'd imagine all such attributes are in the null namespace (or does xlink:href need to go through SetParsedAttr?)

And remove the |aPrefix == info.mName->GetPrefix()| check as that will always be true.

We should also make the .style code go through SetParsedAttr rather than duplicating much of it. But that can wait until later.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Jonas, can we at least get a bug filed on sharing that mutation listener goop?  copy/pasting all that complexity makes me _really_ unhappy...  :(

Comment 37

[Boris Zbarsky [:bzbarsky]]

> We should also make the .style code go through SetParsedAttr 

It can't, as this function is currently implemented, because it needs to call AttributeWillChange before the style change happens.

Comment 38

[Jonathan Watt [:jwatt]]

Attachment: Patch with outcome of IRC discussion implemented

Hopefully this is pretty close to what we agreed. :)

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 508680 [details] [diff] [review]
Patch with outcome of IRC discussion implemented

Sold! Though maybe simply call the function MaybeCheckSameAttrVal or just CheckSameAttrVal. The documentation can mention that as a perf optimization we don't actually check if we aren't notifying and don't have mutation listeners.

Comment 40

[Jonathan Watt [:jwatt]]

Done!

http://hg.mozilla.org/mozilla-central/rev/bcf40420cf32

Comment 41

[Boris Zbarsky [:bzbarsky]]

Fwiw, the documentation for MaybeCheckSameAttrVal is wrong.  Needs s/different to the current value/the same as the current value/.

Comment 42

[Steve Scott (pxbugz)]

Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre