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: | NEW | ||
| 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 | ||
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>