Nightly, bp-5e57c857-ed24-4c53-9911-d52ff2111122 [@ @0x0 | nsSVGPathGeometryFrame::GetCanvasTM ] Local debug build [@ @0x0 | nsSVGPathGeometryFrame::UpdateCoveredRegion ] This seems to be a regression from within the last few days. Perhaps due to the fix for bug 696078?

This crashes due to the <rect> in the <feMerge>. The top stack frame from the crash stack is nsSVGPathGeometryFrame::UpdateCoveredRegion. This method calls nsSVGPathGeometryFrame::GetCanvasTM, which assumes it can static_cast mParent to nsSVGContainerFrame, but the frame type created for feMerge (SVGFEContainerFrame) is not of that type. Hence bad things happen. Since bug 696078 landed, we create SVGFEContainerFrame frames for feMerge, so the frames we create for feMerge are now of type nsContainerFrame. Seems like we should explicitly prohibit non-leaf filter primitive frames as children.

Oops. Ignore the parentTag change, I forgot to remove that.

Simplify the code. As longsonr points out, it's possible to further simplify: if ((parentIsFilter && !filterPrimitive) || (!parentIsFilter && filterPrimitive)) { return &sSuppressData; } to: if (parentIsFilter != bool(filterPrimitive)) { return &sSuppressData; } However, I think that makes the logic harder to understand, so I'd rather have the code as it is in this patch.

Comment on attachment 577598 [details] [diff] [review] patch Switching to longsonr for review to lighten bz's load.

Comment on attachment 577598 [details] [diff] [review] patch >+static bool >+IsFilterPrimitiveChildTag(nsIAtom* aTag) const nsIAtom* >- // Some elements must be children of filter primitive elements. >- if ((aTag == nsGkAtoms::feDistantLight || aTag == nsGkAtoms::fePointLight || >- aTag == nsGkAtoms::feSpotLight || >- aTag == nsGkAtoms::feFuncR || aTag == nsGkAtoms::feFuncG || >- aTag == nsGkAtoms::feFuncB || aTag == nsGkAtoms::feFuncA || >- aTag == nsGkAtoms::feMergeNode) && >- aParentFrame->GetType() != nsGkAtoms::svgFEContainerFrame) { >+ // Prevent bad frame types being children of filter primitives or parents of >+ // filter primitive children: >+ bool parentIsFEContainerFrame = >+ aParentFrame->GetType() != nsGkAtoms::svgFEContainerFrame; Well that's confusing, calling a variable the logical opposite of what it is. >+ if ((parentIsFEContainerFrame && !IsFilterPrimitiveChildTag(aTag)) || >+ (!parentIsFEContainerFrame && IsFilterPrimitiveChildTag(aTag))) { > return &sSuppressData; > } So this makes sense, and is therefore currently wrong given the variable definition above isn't it? In other words if you changed the variable above to be == based then you would not change this and everything would be correct. There are tests on feXXXLight so I'm surprised that there aren't reftest failures given this reversed logic. If there aren't then can you an additional reftest as a sanity check. An feFunc test may be simplest.

r=longsonr with comments addressed.

Sorry, there were indeed test failures and I fixed that bug locally, but somehow I forgot to mention that here. Thanks.

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/cc106438a2ff

inbound/m-c merge by Ed Morley https://hg.mozilla.org/mozilla-central/rev/cc106438a2ff

If this patch works as-is for Firefox 10 please request approval to land on beta. If it requires a modified back-port please create one and request approval on that (and review if it's a non-trivial change).

firefox 10 is unaffected. bug 696078 only landed on 11.

Verified on Fx11 and Fx12. On older builds of Fx11 the application crashes on loading the testcase. Using the latest Fx11(beta8) and Fx12 you can load the testcase and the application does not crash. The nightly doesn't crash either.

Verified in trunk (Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120314 Firefox/14.0a1).