Last Comment Bug 704706 - Crash [@ nsSVGPathGeometryFrame::UpdateCoveredRegion] with SVG filter
: Crash [@ nsSVGPathGeometryFrame::UpdateCoveredRegion] with SVG filter
: crash, regression, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86_64 Mac OS X
-- critical (vote)
: mozilla11
Assigned To: Jonathan Watt [:jwatt]
: Jet Villegas (:jet)
: 711963 (view as bug list)
Depends on:
Blocks: 344905 696078
  Show dependency treegraph
Reported: 2011-11-22 17:57 PST by Jesse Ruderman
Modified: 2012-04-10 21:20 PDT (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (crashes Firefox when loaded) (280 bytes, image/svg+xml)
2011-11-22 17:57 PST, Jesse Ruderman
no flags Details
debug stack trace (7.63 KB, text/plain)
2011-11-22 17:58 PST, Jesse Ruderman
no flags Details
patch (6.38 KB, patch)
2011-11-29 02:12 PST, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch (5.22 KB, patch)
2011-11-29 06:57 PST, Jonathan Watt [:jwatt]
longsonr: review+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2011-11-22 17:57:54 PST
Created attachment 576370 [details]
testcase (crashes Firefox when loaded)

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?
Comment 1 User image Jesse Ruderman 2011-11-22 17:58:16 PST
Created attachment 576371 [details]
debug stack trace
Comment 2 User image Jonathan Watt [:jwatt] 2011-11-28 11:26:11 PST
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.
Comment 3 User image Jonathan Watt [:jwatt] 2011-11-29 02:12:58 PST
Created attachment 577528 [details] [diff] [review]
Comment 4 User image Jonathan Watt [:jwatt] 2011-11-29 02:14:41 PST
Oops. Ignore the parentTag change, I forgot to remove that.
Comment 5 User image Jonathan Watt [:jwatt] 2011-11-29 06:57:37 PST
Created attachment 577598 [details] [diff] [review]

Simplify the code.

As longsonr points out, it's possible to further simplify:

  if ((parentIsFilter && !filterPrimitive) ||
      (!parentIsFilter && filterPrimitive)) {
    return &sSuppressData;


  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 6 User image Jonathan Watt [:jwatt] 2011-12-09 09:40:52 PST
Comment on attachment 577598 [details] [diff] [review]

Switching to longsonr for review to lighten bz's load.
Comment 7 User image Robert Longson 2011-12-09 09:57:29 PST
Comment on attachment 577598 [details] [diff] [review]

>+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.
Comment 8 User image Robert Longson 2011-12-09 09:58:50 PST
r=longsonr with comments addressed.
Comment 9 User image Jonathan Watt [:jwatt] 2011-12-09 10:04:24 PST
Sorry, there were indeed test failures and I fixed that bug locally, but somehow I forgot to mention that here. Thanks.
Comment 10 User image Jonathan Watt [:jwatt] 2011-12-19 18:52:33 PST
Comment 11 User image Daniel Veditz [:dveditz] 2011-12-20 07:21:41 PST
inbound/m-c merge by Ed Morley
Comment 12 User image Daniel Veditz [:dveditz] 2011-12-20 07:35:08 PST
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).
Comment 13 User image Robert Longson 2011-12-20 07:38:57 PST
firefox 10 is unaffected. bug 696078 only landed on 11.
Comment 14 User image Gary Kwong [:gkw] [:nth10sd] 2011-12-22 15:24:26 PST
*** Bug 711963 has been marked as a duplicate of this bug. ***
Comment 15 User image juan becerra [:juanb] 2012-03-12 17:54:09 PDT
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.
Comment 16 User image Al Billings [:abillings] 2012-03-14 15:47:30 PDT
Verified in trunk (Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120314 Firefox/14.0a1).

Note You need to log in before you can comment on or make changes to this bug.