1024926 - SVG Path getTotalLength() does not change after modifying pathSegList

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[alancutter]

User Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140609004001

Steps to reproduce:

http://jsfiddle.net/7LpsZ/

1. Create a path with some data.
2. Read getTotalLength() from the path.
3. Add/remove path segments to/from the pathSegList.
4. Read getTotalLength() from the path again.


Actual results:

The old value from the first time getTotalLength() was called is returned.
It appears a cached value didn't get invalidated after the pathSegList was modified.


Expected results:

A new value should be returned from getTotalLength() after the pathSegList is modified.

Comment 1

[alancutter]

This bug was introduced in Firefox 31, was previously working in Firefox 30.

Note the jsfiddle example throws an exception in Firefox 30 due to a separate bug in getTotalLength().
See: https://bugzilla.mozilla.org/show_bug.cgi?id=1024860

Comment 2

[:Gijs (he/him)]

(In reply to alancutter from comment #1)
> This bug was introduced in Firefox 31, was previously working in Firefox 30.
> 
> Note the jsfiddle example throws an exception in Firefox 30 due to a
> separate bug in getTotalLength().
> See: https://bugzilla.mozilla.org/show_bug.cgi?id=1024860

Can you provide an example that does actually work in Fx 30? That'll make it easier for someone to figure out when this regressed. :-)

Comment 3

[Robert Longson [:longsonr]]

bug 1024860 says that getTotalLength throws when there's no path. Is that all you're seeing as that's what the jsfiddle does or is there something else?

Comment 4

[alancutter]

Sorry, I can see it'd be confusing with two bugs in the same example now. :)
Updated example to avoid bug 1024860: http://jsfiddle.net/7LpsZ/2/


Test output for Firefox 31.0a2:
"""
Created path

getTotalLength() == 500
Expected 500

Removed 2 path segments

getTotalLength() == 500
Expected 150
"""


Test output for Firefox 30.0b9 and Chrome:
"""
Created path

getTotalLength() == 500
Expected 500

Removed 2 path segments

getTotalLength() == 150
Expected 150
"""

Comment 5

[Alice0775 White]

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea97b234c940
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140528161650
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cb1217dc3ff
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140528165349
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ea97b234c940&tochange=8cb1217dc3ff
Regressed by:
8cb1217dc3ff	Bas Schouten — Bug 992850: Cache Moz2D path for length or position measuring. r=jwatt

Comment 6

[Robert Longson [:longsonr]]

Attachment: cache.txt

Comment 7

[Robert Longson [:longsonr]]

Jonathan, Daniel do you think we should get bug 992850 backed out of everywhere except the trunk? If so can one of you do that please? If not we will need to land this instead. Personally I'd go with the backout.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Regression, tracking.

Comment 9

[Daniel Holbert [:dholbert]]

Sorry, I was out last week. Tagging jwatt for needinfo for response to Comment 7, since he was involved [as reviewer] on the bug in question, whereas I don't really know the backstory.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8440337 [details] [diff] [review]
cache.txt

Bas, could you help for the review? We could accept a late fix for beta 7 (Thursday) Thanks

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 8440337 [details] [diff] [review]
cache.txt

This code needs an SVG peer to review it. I'm reviewing it just now.

Comment 12

[Bas Schouten (:bas.schouten)]

Wait for Jwatt's review though :-).

Comment 13

[Jonathan Watt [:jwatt]]

Comment on attachment 8440337 [details] [diff] [review]
cache.txt

I did a close audit of the code and fixed one remaining source of stale measuring paths in DOMSVGPathSeg.cpp. Otherwise this looks correct though, so r+. I pushed the patch plus the DOMSVGPathSeg.cpp fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cfae96d4136

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 8440337 [details] [diff] [review]
cache.txt

Approval Request Comment
[Feature/regressing bug #]: bug 992850
[User impact if declined]: some broken SVG
[Describe test coverage new/current, TBPL]: been on m-i overnight
[Risks and why]: very low - only clears a cached object, forcing us to recreate it when appropriate
[String/UUID change made/needed]: none

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5cfae96d4136

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8440337 [details] [diff] [review]
cache.txt

Recent regression. Approved.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/213f1b2d7d76

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a6cde8bb0578

Comment 19

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I see this is covered by test in: https://hg.mozilla.org/releases/mozilla-beta/diff/a6cde8bb0578/content/svg/content/test/test_pathLength.html