879682 - SVG feOffset filters clipped for dynamically created SVG nodes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Eugenio Marchiori]

Attachment: firefox-bug2.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.93 Safari/537.36

Steps to reproduce:

Dynamically generated a SVG with an feOffset filter.

SVG generated automatically through Swiffy (Flash to HTML5 conversion tool) easily expose this error. However, this error can be reproduced with manually crafted HTML.

I was able to reproduce this error in versions starting from 17 in windows and linux. It is still present in the nightlies in both systems.

The attached example exposes the problem, as it should not display any red areas. Refreshing the browser sometimes fixes the issue. Zooming in or out usually fixes the issue, as it causes a full refresh and redraw of the SVG. 

NOTE: This bug can be overcome by creating a rect with opacity 0.0 that has the size of the filter's bounding box, however this probably negatively affects performance.


Actual results:

The clipping used for elements that have feOffset filters is wrong, causing part of the filtered element to be clipped off.


Expected results:

The clipping area should have been extended beyond the elements bounds, to allow the filter to be fully rendered.

In the example, it should only render a green rect, with no red areas.

Comment 1

[Alice0775 White]

I can also reproduce in Nightly24.0a1
http://hg.mozilla.org/mozilla-central/rev/7e3a4ebcf067
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130604 Firefox/24.0 ID:20130604031112

Reload of the testcase reproduce the problem sometimes...

Comment 2

[Markus Stange [:mstange]]

Attachment: v1

I see this bug in a locally-built opt build about 50% of the time.

The filter is not painted completely because the nsSVGFilterInstance has a wrong mPostFilterDirtyRect, which comes from a wrong overflow rect set on the frame. When calculating the frame's overflow, FinishAndStoreOverflow calls ComputeOutlineAndEffectsRect, which calls nsSVGUtils::GetPostFilterVisualOverflowRect, which calls nsSVGEffects::GetFilterFrame, which tries to get the filter frame from the frame's filter property and bails out because that property has not been set yet. The reason this property hasn't been set is that nsSVGEffects::UpdateEffects (which usually creates the effects properties) hasn't been called for the frame from nsSVGDisplayContainerFrame::ReflowSVG() because the frame's parent frame has already had its first reflow before. This last bit seems wrong to me.

The first patch I had changed nsSVGUtils::GetPostFilterVisualOverflowRect to call nsSVGEffects::GetEffectProperties instead of nsSVGEffects::GetFilterFrame, because the former initializes the filter property if it doesn't exist. This is what nsSVGUtils::PaintFrameWithEffects does. But it seems like a workaround.

Comment 3

[Markus Stange [:mstange]]

Attachment: v2

Oops, uploaded the wrong patch. This is the one that's not the workaround.

Comment 4

[Markus Stange [:mstange]]

I think it would be good to reduce the fragility here somehow. I think either UpdateEffects should stop initializing the filter property and all filter frame getters (like nsSVGEffects::GetFilterFrame) should be able to do lazy initialization, or UpdateEffects should be the only way to create the filter property. In the latter solution, all other filter frame getters could assert that the property's existence is up to date. Thoughts?

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 791726 [details] [diff] [review]
v2

r+ if you add a reftest and make the same change in nsSVGSwitchFrame.

I'd suggest that for the test you crib from something like:

https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/filters/dynamic-filtered-foreignObject-01.svg

integrate the minimal amount you need from the attached test, and put the test in that directory with a name along the lines of dynamic-filter-invalidation-01.svg

I find it a bit surprising that this bug hasn't been more prevalent.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Markus Stange [:mstange] from comment #4)
> I think it would be good to reduce the fragility here somehow. I think
> either UpdateEffects should stop initializing the filter property and all
> filter frame getters (like nsSVGEffects::GetFilterFrame) should be able to
> do lazy initialization, or UpdateEffects should be the only way to create
> the filter property. In the latter solution, all other filter frame getters
> could assert that the property's existence is up to date. Thoughts?

Yeah, I tried to clean this stuff up some time ago but ran into some annoying failures. If you want to have a go it'd be great if you could open a new bug and I'd be happy to review there. I'd suggest you do a Try run that includes reftests before requesting review.

Comment 7

[Markus Stange [:mstange]]

Attachment: v3

Comment 8

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a3b0532422

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Caused assertion failures on Windows like:

https://tbpl.mozilla.org/php/getParsedLog.php?id=26962547&tree=Mozilla-Inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/beba6103e786

Comment 10

[Markus Stange [:mstange]]

This was relanded on mozilla-inbound as
http://hg.mozilla.org/integration/mozilla-inbound/rev/48a3aeb26ed6
and merged to central as
http://hg.mozilla.org/mozilla-central/rev/48a3aeb26ed6