Bug 314992
| Summary: | SVGTransformList re-parse mutates existing same-type items in place, violating SVG2 SVGTransform.matrix [SameObject] semantics | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Ahmad Saleem <ahmad.saleem792> |
| Component: | SVG | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED WONTFIX | ||
| Severity: | Normal | CC: | karlcow, sabouhallawa, webkit-bug-importer, zimmermann |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=207421 https://bugs.webkit.org/show_bug.cgi?id=322973 https://bugs.webkit.org/show_bug.cgi?id=322974 https://bugs.webkit.org/show_bug.cgi?id=322976 |
||
Ahmad Saleem
Summary
Re-parsing a reflected SVG transform attribute (e.g. setAttribute("transform", …)) mutates an
existing SVGTransform in the list in place when the new token has the same transform type at the
same index. Per SVG 2, list items should be detached on re-parse and retain their pre-reparse
values, with [SameObject] SVGTransform.matrix tear-offs continuing to reflect the (now-detached)
SVGTransform. WebKit's in-place mutation makes a script-held tear-off observably switch values,
which the spec disallows.
This is not a regression from 294342@main (Remove some of the ref-counting churns when parsing
SVGTransformList) — that commit refactored the existing in-place-reuse path into two helpers
(parseAndReplaceTransform / parseTransform) without changing semantics.
Affected APIs
The bug affects every reflected SVGTransformList attribute that goes through
SVGTransformList::parse(StringView) from attributeChanged:
- transform on SVGGraphicsElement
- gradientTransform on SVGGradientElement
- patternTransform on SVGPatternElement
---
Spec
- SVG 2 — SVGTransform interface, matrix IDL:
https://svgwg.org/svg2-draft/coords.html#InterfaceSVGTransform
▎ [SameObject] readonly attribute DOMMatrix matrix;
▎ "When the matrix object is first created, its values are set to match the SVGTransform's transform
function value, and is set to reflect the SVGTransform."
- SVG 2 — DOMMatrix extensions for reflecting SVGTransform:
https://svgwg.org/svg2-draft/coords.html#DOMMatrixExtensions
▎ "Every DOMMatrix object operates in one of two modes. It can: reflect an SVGTransform … or be
detached …"
▎ Mutating a reflecting matrix updates the SVGTransform's value; if the SVGTransform reflects an
element of a presentation attribute, the host attribute is reserialized.
- SVG 2 — list interface "detached" definition:
https://svgwg.org/svg2-draft/types.html#TermDetachedObject
▎ "A detached object retains its current values…"
---
Minimal reproduction
<!DOCTYPE html>
<svg xmlns="http://www.w3.org/2000/svg">
<g id="g" transform="translate(10, 20)"></g>
</svg>
<script>
const g = document.getElementById("g");
const t = g.transform.baseVal.getItem(0);
const m = t.matrix; // [SameObject] tear-off, reflects t
console.log(m.e, m.f); // 10 20
g.setAttribute("transform", "translate(500, 600)");
// Per SVG2: t is detached, retains its old value. m still reflects t.
// Per SVG2: g.transform.baseVal.getItem(0) is a *new* SVGTransform.
console.log(m.e, m.f); // EXPECTED: 10 20
console.log(g.transform.baseVal.getItem(0).matrix.e); // EXPECTED: 500
console.log(t.matrix === m); // EXPECTED: true ([SameObject])
</script>
Expected output (per spec, also produced by Firefox/WebKit only after this fix):
10 20
10 20
500
true
Actual output (current WebKit, also Firefox):
10 20
500 600 <-- BUG: detached SVGTransform's value was mutated under the script's feet
500
true
A type-changing reparse (setAttribute("transform", "matrix(1,0,0,1,500,600)")) does not exhibit the
bug, because the type-mismatch branch in parseGeneric already takes the spec-conformant
detach-and-insert path. This narrows the bug precisely to the same-type-same-index fast path.
---
Real-world symptom
A common drag-handler pattern that captures getItem(0).matrix once at mousedown and re-reads it on
every mousemove is silently corrupted: because the captured tear-off shares identity with the one
re-read in the move handler, writes alias each other, and after each setAttribute the captured
"anchor" matrix advances with the live value. The dragged element accelerates off-screen. Repro at
https://jsfiddle.net/8xvobd43/3/. Where trying to drag makes `rect` go out of the view (in Safari & Firefox).
---
Source
Source/WebCore/svg/SVGTransformList.cpp:138 — the same-type fast path:
if (currentListReplacement == ListReplacement::Replace && itemIndex < m_items.size()
&& parsedTransformType == m_items[itemIndex]->type()) {
if (!parseAndReplaceTransform(*parsedTransformType, buffer, m_items[itemIndex]))
return false;
} else {
// Switch to `Append` mode and remove the existing SVGTransforms starting from `itemIndex`.
...
}
The else branch already does the right thing (resize(itemIndex) detaches existing items, then a
fresh SVGTransform is appended). The if branch should be removed; every iteration should take the
detach-and-append path. Removing the fast path also makes parseAndReplaceTransform dead code.
---
Proposed fix
Drop the same-type fast path in parseGeneric, always detach existing items on the first iteration of
a Replace parse, and remove the now-unused parseAndReplaceTransform (declaration in
SVGTransformList.h, definition in SVGTransformListInlines.h).
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Radar WebKit Bug Importer
<rdar://problem/177837909>
Karl Dubost
TLDR:
I suggest closing this and filing the reverse bugs for SVGNumberList, SVGLengthList and SVGPointList.
Details:
SVG2 doesn't seem to request this.
A re-parse detaches list items only when the list shrinks, and for this case the algorithm explicitly updates the matrix object the script is holding.
Taking the repro from comment 0:
<svg xmlns="http://www.w3.org/2000/svg">
<g id="g" transform="translate(10, 20)"></g>
</svg>
const g = document.getElementById("g");
const list = g.transform.baseVal; // the list interface object
const t = list.getItem(0); // the SVGTransform at index 0
const m = t.matrix; // its matrix object
g.setAttribute("transform", "translate(500, 600)");
That last line changes the base value of a reflected content attribute, which triggers:
"Whenever a reflected content attribute's base value changes, then the reflecting object must be synchronized, immediately after the value changed"
step 1: "If the reflecting object is a list interface object, then run the steps for synchronizing a list interface object."
https://w3c.github.io/svgwg/svg2-draft/types.html#SynchronizingReflectedValues
Walking those steps against the example:
https://w3c.github.io/svgwg/svg2-draft/types.html#TermSynchronizeList
step 1 value is translate(500, 600)
step 2 length is 1, the list holds one SVGTransform, which is t
step 3 new length is 1
step 4 the element type is SVGTransform, so run 4.1 to 4.4
step 4.1 "If length > new length" is 1 > 1, false. THIS IS THE ONLY DETACH IN THE ALGORITHM AND IT DOES NOT RUN.
step 4.2 "While length < new length" is 1 < 1, false. No object is created, so index 0 still holds t.
step 4.3 index is 0
step 4.4 index < length, so:
4.4.1 "Let item be the object in the list at index index" -> item is t, the object the script holds
4.4.2 "Let v be the value in value at index index" -> v is translate(500, 600)
4.4.3 "Set item's value to v" -> t's value becomes translate(500, 600)
4.4.4 "If item is an SVGTransform object, then set the components of its matrix object to match the new transform function value"
Step 4.4.4 is the whole question. It says set the components of ITS matrix object, which is m. Not replace m, not leave m alone: set its components. So after the setAttribute:
list.getItem(0) === t; // true, nothing was detached (4.1) and nothing was created (4.2)
t.matrix === m; // true, 4.4.4 updates this matrix object rather than replacing it
m.e; // 500, 4.4.4 set its components
Step 5, "Replace the list with a new list", is the DOMString branch and is not reached.
Measured, attached testcase, the same observable on all four object-typed lists:
in place (conformant) detached
STP 251 1 of 4 3
Firefox 156 4 of 4 0
Chrome 154 0 of 4 4
Chrome prints the expected output listed in comment 0, including the 10 20 on the second read. No other engine does.
No WPT test covers this. Nothing in the corpus holds a list item across a re-parse, in either direction. The near misses all re-fetch instead of holding, so they pass under both readings: SVGTransformList.list-01.html only sets the attribute during setup, SVGLengthList-basics.html sets x then re-reads getItem(0).value, and the SMIL list tests re-read animVal.getItem(i).value at every sample.
There is one test that asserts the reading in comment 0, and it is ours, not WPT: LayoutTests/svg/dom/SVGLengthList-xml-dom-modifications.xhtml, added 2010-10-21. It holds all three items of x="500 1000 1500", sets x="50 100", and asserts under "Assure that the wrappers still point to the OLD values" that all three keep their old values.
That case is a shrink, 3 items to 2, so step 4.1 does run. It detaches "each object in the list at an index greater than or equal to new length", which is index 2 only. Indices 0 and 1 survive the truncation and step 4.4 then sets their values to 50 and 100. So that test is right about item 2 and wrong about items 0 and 1, and it needs a partial rebaseline rather than deletion.
The one list WebKit gets right is SVGTransformList, which is the one this bug proposes to change. Dropping the same-type fast path would take WebKit to 0 of 4 and away from Firefox. The real defect is the other three: rotate, x and points detach where they should update, so a script holding an item silently keeps a stale value.
On the drag handler: "An SVGTransform object's matrix object is always kept synchronized with its value", https://w3c.github.io/svgwg/svg2-draft/coords.html#TransformMatrixObject, which is the same statement as 4.4.4 from the other direction. getItem(0).matrix is a live handle rather than an anchor. Copying the numbers at mousedown is the fix on the page side.
Suggest closing this and filing the reverse for SVGNumberList, SVGLengthList and SVGPointList.