Closed Bug 704706 Opened 8 years ago Closed 8 years ago

Crash [@ nsSVGPathGeometryFrame::UpdateCoveredRegion] with SVG filter

Categories

(Core :: SVG, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla11
Tracking Status
firefox8 - unaffected
firefox9 - unaffected
firefox10 + unaffected
firefox11 + verified
firefox12 --- verified
firefox-esr10 --- unaffected
status1.9.2 --- unaffected

People

(Reporter: jruderman, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase, Whiteboard: [sg:critical][qa!])

Crash Data

Attachments

(3 files, 1 obsolete file)

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?
Attached file debug stack trace
Assignee: nobody → jwatt
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.
Attached patch patch (obsolete) — Splinter Review
Attachment #577528 - Flags: review?(bzbarsky)
Oops. Ignore the parentTag change, I forgot to remove that.
Attached patch patchSplinter Review
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.
Attachment #577528 - Attachment is obsolete: true
Attachment #577528 - Flags: review?(bzbarsky)
Attachment #577598 - Flags: review?(bzbarsky)
Comment on attachment 577598 [details] [diff] [review]
patch

Switching to longsonr for review to lighten bz's load.
Attachment #577598 - Flags: review?(bzbarsky) → review?(longsonr)
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.
Attachment #577598 - Flags: review?(longsonr) → review+
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.
inbound/m-c merge by Ed Morley
https://hg.mozilla.org/mozilla-central/rev/cc106438a2ff
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Keywords: regression
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.
Duplicate of this bug: 711963
Whiteboard: [sg:critical] → [sg:critical][qa+]
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.
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).
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.