Closed Bug 1024926 Opened 5 years ago Closed 5 years ago

SVG Path getTotalLength() does not change after modifying pathSegList

Categories

(Core :: SVG, defect)

31 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox30 --- unaffected
firefox31 + fixed
firefox32 + fixed
firefox33 + fixed
firefox-esr24 --- unaffected

People

(Reporter: alancutter, Assigned: longsonr)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
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
Component: Untriaged → SVG
Product: Firefox → Core
(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. :-)
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?
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
"""
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
Blocks: 992850
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Assignee: nobody → longsonr
Attached patch cache.txtSplinter Review
Attachment #8440337 - Flags: review?(jwatt)
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.
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.
Flags: needinfo?(jwatt)
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
Attachment #8440337 - Flags: review?(bas)
Comment on attachment 8440337 [details] [diff] [review]
cache.txt

This code needs an SVG peer to review it. I'm reviewing it just now.
Attachment #8440337 - Flags: review?(bas)
Flags: needinfo?(jwatt)
Attachment #8440337 - Flags: review?(jwatt) → review+
Wait for Jwatt's review though :-).
Attachment #8440337 - Flags: review?(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
Attachment #8440337 - Flags: review?(jwatt) → review+
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
Attachment #8440337 - Flags: approval-mozilla-beta?
Attachment #8440337 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5cfae96d4136
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment on attachment 8440337 [details] [diff] [review]
cache.txt

Recent regression. Approved.
Attachment #8440337 - Flags: approval-mozilla-beta?
Attachment #8440337 - Flags: approval-mozilla-beta+
Attachment #8440337 - Flags: approval-mozilla-aurora?
Attachment #8440337 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.