Closed Bug 979407 Opened 11 years ago Closed 11 years ago

"ASSERTION: viewport height must be nonnegative", with negative 'markerHeight' attribute (and similar for negative markerWidth)

Categories

(Core :: SVG, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jruderman, Assigned: longsonr)

References

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached image testcase
###!!! ASSERTION: viewport height must be nonnegative!: 'aViewportHeight >= 0', file content/svg/content/src/SVGContentUtils.cpp, line 295
This is likely because markerHeight is negative. The SVG Spec says "A negative value is an error" for this attribute[1]. Maybe we're not honoring that? [1] http://www.w3.org/TR/SVG11/painting.html#MarkerHeightAttribute
Summary: "ASSERTION: viewport height must be nonnegative" → "ASSERTION: viewport height must be nonnegative", with negative 'markerHeight' attribute
(I can repro on 64-bit Linux Nightly, BTW)
OS: Mac OS X → All
Attached image testcase 2
(Here's another testcase with simpler coordinates that also reproduces the bug.)
Note: Technically this puts the document "in error", which means we're only supposed to render the document up to the point of the error, per: http://www.w3.org/TR/SVG11/implnote.html#ErrorProcessing but I don't think we currently do that for other invalid attributes. I suspect it's probably OK to just reject it as a parse error. Assuming we go that route: SVGMarkerElement::ParseAttribute doesn't currently have any error checking for negative attributes: http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGMarkerElement.cpp#208 It just ends up parsing the attribute using SVGMarkerElementBase::ParseAttribute -- which is really nsSVGElement::ParseAttribute -- which iterates over the entries in SVGMarkerElement::sLengthInfo and finds the attribute-name there and parses it as a length. (without any knowledge about the nonnegative requirement) Maybe we should add a bitfield to "LengthInfo", with a flag that (if set) tells us that a given attribute explicitly excludes negative values?
Actually, disregard comment 4, I think -- documents can set this attribute from the DOM as well, and ParseAttribute tweaks wouldn't catch that. And the IDL shows that it's a SVGAnimatedLength: http://www.w3.org/TR/SVG11/painting.html#InterfaceSVGMarkerElement so it probably shouldn't have any extra magic parsing behavior beyond any other SVGAnimatedLength. Maybe we just have to be graceful about handling (successfully-parsed) negative values internally.
Summary: "ASSERTION: viewport height must be nonnegative", with negative 'markerHeight' attribute → "ASSERTION: viewport height must be nonnegative", with negative 'markerHeight' attribute (and similar for negative markerWidth)
(This bug applies equally well to markerWidth. Here's a testcase, with markerWidth="-1px", which triggers: ###!!! ASSERTION: viewport width must be nonnegative!: 'aViewportWidth >= 0', file SVGContentUtils.cpp, line 294 )
This is happening in SVGMarkerElement::GetViewBoxTransform, BTW. In the similar method for the <svg> element (SVGSVGElement::GetViewBoxTransform), we have this early-return: > 657 if (viewportWidth <= 0.0f || viewportHeight <= 0.0f) { > 658 return gfx::Matrix(0.0, 0.0, 0.0, 0.0, 0.0, 0.0); // singular > 659 } http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGSVGElement.cpp#657 We probably need the same early-return here.
Without an explicit viewBox we create a viewBox from the markerWidth and markerHeight. We then trap that in nsSVGMarkerFrame::PaintMark and nsSVGMarkerFrame::GetMarkBBoxContribution like so... const nsSVGViewBoxRect viewBox = content->GetViewBoxRect(); if (viewBox.width <= 0.0f || viewBox.height <= 0.0f) { return bbox; } However if we do have a viewBox then we use that rather than the markerWidth and markerHeight and we don't check those things. What we need are calls to nsSVGMarkerElement::HasValidDimensions in nsSVGMarkerFrame::PaintMark and nsSVGMarkerFrame::GetMarkBBoxContribution.
Attached patch marker.txtSplinter Review
Assignee: nobody → longsonr
Attachment #8386021 - Flags: review?(dholbert)
Comment on attachment 8386021 [details] [diff] [review] marker.txt Thanks!
Attachment #8386021 - Flags: review?(dholbert) → review+
Comment on attachment 8386021 [details] [diff] [review] marker.txt Oh, one nit: >+++ b/layout/svg/crashtests/crashtests.list >@@ -175,8 +175,10 @@ load 893510-1.svg > load 974746-1.svg >+load 979407-1.svg >+load 979407-2.svg >\ No newline at end of file Please add a newline at the end of this file.
Flags: in-testsuite+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: