Crash [@ nsSVGPathGeometryFrame::UpdateCoveredRegion] with SVG filter

VERIFIED FIXED in Firefox 11



6 years ago
5 years ago


(Reporter: Jesse Ruderman, Assigned: jwatt)


(Blocks: 1 bug, {crash, regression, testcase})

Mac OS X
crash, regression, testcase
Dependency tree / graph

Firefox Tracking Flags

(firefox8- unaffected, firefox9- unaffected, firefox10+ unaffected, firefox11+ verified, firefox12 verified, firefox-esr10 unaffected, status1.9.2 unaffected)


(Whiteboard: [sg:critical][qa!], crash signature)


(3 attachments, 1 obsolete attachment)



6 years ago
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

6 years ago
Created attachment 576371 [details]
debug stack trace


6 years ago
Assignee: nobody → jwatt

Comment 2

6 years ago
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

6 years ago
Created attachment 577528 [details] [diff] [review]
Attachment #577528 - Flags: review?(bzbarsky)

Comment 4

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

Comment 5

6 years ago
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.
Attachment #577528 - Attachment is obsolete: true
Attachment #577528 - Flags: review?(bzbarsky)
Attachment #577598 - Flags: review?(bzbarsky)


6 years ago
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox8: --- → wontfix
status-firefox9: --- → wontfix
tracking-firefox10: --- → +
tracking-firefox11: --- → +
tracking-firefox8: --- → -
tracking-firefox9: --- → -


6 years ago
status-firefox8: wontfix → unaffected
status-firefox9: wontfix → unaffected

Comment 6

6 years ago
Comment on attachment 577598 [details] [diff] [review]

Switching to longsonr for review to lighten bz's load.
Attachment #577598 - Flags: review?(bzbarsky) → review?(longsonr)

Comment 7

6 years ago
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.


6 years ago
Attachment #577598 - Flags: review?(longsonr) → review+

Comment 8

6 years ago
r=longsonr with comments addressed.

Comment 9

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

Comment 10

6 years ago
inbound/m-c merge by Ed Morley
Last Resolved: 6 years ago
status-firefox11: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Keywords: regression
status1.9.2: --- → unaffected
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

6 years ago
firefox 10 is unaffected. bug 696078 only landed on 11.
Duplicate of this bug: 711963


6 years ago
status-firefox10: affected → unaffected
status-firefox12: --- → fixed
Whiteboard: [sg:critical] → [sg:critical][qa+]
status-firefox-esr10: --- → unaffected
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.
status-firefox11: fixed → verified
status-firefox12: fixed → verified
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Verified in trunk (Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120314 Firefox/14.0a1).
Group: core-security
You need to log in before you can comment on or make changes to this bug.