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.
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
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)
Created attachment 791726 [details] [diff] [review] v2 Oops, uploaded the wrong patch. This is the one that's not the workaround.
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.
Created attachment 794860 [details] [diff] [review] v3
Attachment #791726 - Attachment is obsolete: true
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
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.