SVG feOffset filters clipped for dynamically created SVG nodes

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: eugeniom, Assigned: mstange)

Tracking

unspecified
mozilla26
x86_64
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jwatt:invalidation])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 758455 [details]
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.

Updated

6 years ago
Component: Untriaged → SVG
Product: Firefox → Core

Updated

6 years ago
Attachment #758455 - Attachment mime type: text/plain → text/html

Comment 1

6 years ago
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...
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Whiteboard: [jwatt:invalidation]
(Assignee)

Comment 2

6 years ago
Created attachment 791725 [details] [diff] [review]
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.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attachment #791725 - Flags: review?(jwatt)
(Assignee)

Comment 3

6 years ago
Created attachment 791726 [details] [diff] [review]
v2

Oops, uploaded the wrong patch. This is the one that's not the workaround.
Attachment #791725 - Attachment is obsolete: true
Attachment #791725 - Flags: review?(jwatt)
Attachment #791726 - Flags: review?(jwatt)
(Assignee)

Comment 4

6 years ago
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 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.
Attachment #791726 - Flags: review?(jwatt) → review+
(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.
(Assignee)

Comment 7

6 years ago
Created attachment 794860 [details] [diff] [review]
v3
Attachment #791726 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.