475181 - Crash [@ nsSVGFETileElement::Filter]

Status: VERIFIED FIXED

(Core :: SVG, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: Tile overflows surface rect, this code can't handle it: 'instance->GetSurfaceRect().Contains(tile)', file /Users/jruderman/central/content/svg/content/src/nsSVGFilters.cpp, line 2857

Crash [@ nsSVGFETileElement::Filter] trying to access 0x1ed51400.

Bug 448244's testcase triggers the same assertion, but does not crash.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

This patch will happily stop us from reading bad memory in the feTile case.

I'm wondering if the more fundamental problem though is that we don't clamp mFilterPrimitiveSubregion to be within the filter region. (In the testcase, because the 'y' attribute on the feImage is set to a large value, it's filter primitive subregion lies way outside the filter region.) I'm still trying to figure out whether it ever makes sense to have a filter primitive subregion that is partially or fully outside the filter region.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we did, that would create visible rendering changes for things like feImage. (Wouldn't it?) And therefore the spec would have to say that clamping should happen.

Comment 3

[Jonathan Watt [:jwatt]]

Well the spec seems less clear than it could be, but it does seem to effectively say that. The first sentence of the section on the filter primitive subregion says that x, y, width and height identify the subregion, but it goes on to say in the fourth paragraph:

  All intermediate offscreens are defined to not exceed the intersection of
  x, y, width and height with the filter region. The filter region and any
  of the x, y, width and height subregions are to be set up such that all
  offscreens are made big enough to accommodate any pixels which even partly
  intersect with either the filter region or the x,y,width,height subregions.

  http://www.w3.org/TR/SVG/filters.html#FilterPrimitiveSubRegion

If intermediate offscreens are clipped to the intersection of the filter region and filter primitive subregion, I'm not sure why you wouldn't just clip the filter primitive subregion straight off the bat. In fact I'm not sure why the spec doesn't simply say that the filter primitive subregion is defined to be x, y, width and height clipped to the filter region.

I'm also not sure whether allowing filter primitive subregions AND intermediary offscreens (contrary to spec) to extend outside the filter region is even useful. You could certainly test for that (see the following snippet), but I can't so far think of an example that is both useful and not possible if filter primitive subregions were clipped to the filter region.

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <filter id="filter" x="0" y="0" width="100" height="100">
    <feImage x="50" width="100" xlink:href="green.png"/> <!-- native size {100,100} -->
    <feOffset x="-50"/>
  </filter>
  <rect width="100" height="100" fill="red"/>
  <rect width="100" height="100" fill="red" filter="url(#filter)"/>
</svg>

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #3)
>   All intermediate offscreens are defined to not exceed the intersection of
>   x, y, width and height with the filter region. The filter region and any
>   of the x, y, width and height subregions are to be set up such that all
>   offscreens are made big enough to accommodate any pixels which even partly
>   intersect with either the filter region or the x,y,width,height subregions.

I don't think this quite says what you want it to say; it's just talking about the offscreens, and doesn't tell us to adjust the filter subregion itself. If the spec intent is that the subregions are clamped to the filter region, it really needs to say that.

(And spec text about "offscreens" really should be relegated to non-normative notes, because that's implementation detail that may or may not make sense for a particular implementation and should not make any behavioural difference.)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/mozilla-central/rev/7b3ae53a4181 and the test as a crashtest http://hg.mozilla.org/mozilla-central/rev/1a63a436e531.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/81f711716c9b and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/529bf01785e8

Comment 7

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090211 Shiretoko/3.1b3pre. Adding verified keyword.

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090211 Minefield/3.2a1pre.