Crash with linearGradient, feFuncG and removing style -moz-groupbox

RESOLVED FIXED

Status

()

Core
SVG
--
critical
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: tor)

Tracking

({crash, testcase})

Trunk
x86
Windows XP
crash, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
See upcoming testcase, which crashes current trunk build of Firefox.

Talkback ID: TB15586554W
(Reporter)

Comment 1

12 years ago
Created attachment 213116 [details]
testcase

The testcase crashes on load.
(Reporter)

Comment 2

12 years ago
Created attachment 213120 [details]
backtrace from debug build

Backtrace from debug build. It also contains the backtrace from two assertions I got.

The essential crash is in here:
Program received signal SIGSEGV, Segmentation fault.
0x05616b43 in nsSVGCairoPathGeometry::GeneratePath(_cairo*, nsISVGCairoCanvas*)
(this=0xe59cac0, ctx=0xf634aa8, aCanvas=0x0)
    at c:/mozilla/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoPathGeometry.c
pp:167
167       ctm->GetA(&val);
Current language:  auto; currently c++
(gdb) bt
#0  0x05616b43 in nsSVGCairoPathGeometry::GeneratePath(_cairo*, nsISVGCairoCanva
s*) (this=0xe59cac0, ctx=0xf634aa8, aCanvas=0x0)
    at c:/mozilla/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoPathGeometry.c
pp:167
#1  0x05617c73 in nsSVGCairoPathGeometry::GetCoveredRegion(nsISVGRendererRegion*
*) (this=0xe59cac0, _retval=0x22ea38)
    at c:/mozilla/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoPathGeometry.c
pp:500
#2  0x05617b5b in nsSVGCairoPathGeometry::Update(unsigned, nsISVGRendererRegion*
*) (this=0xe59cac0, updatemask=4294967295, _retval=0x22eae8)
    at c:/mozilla/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoPathGeometry.c
pp:472
etc.
(Assignee)

Comment 3

12 years ago
*** Bug 328535 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 4

12 years ago
Caused by the new filter elements not in the CSS frame constructors list of special content.

Comment 5

12 years ago
tor could you please add the standard disclaimer:

// Make sure to keep IsSpecialContent in synch with this code also to http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsSVGElementFactory.cpp#133

thanks
(Assignee)

Comment 6

12 years ago
Created attachment 213686 [details] [diff] [review]
stop crash

State all SVG content is special, which I think is right.

Generate nsFrame objects for filter element leaf nodes instead of a container based frame.  nsFrame.h needed modification to allow this.
Assignee: general → tor
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #213686 - Flags: review?(bzbarsky)
Comment on attachment 213686 [details] [diff] [review]
stop crash

>Index: content/svg/content/src/nsSVGElementFactory.cpp
>+  // Make sure to keep nsCSSFrameConstructor::IsSpecialContent in
>+  // synch with this code

Also keep ConstructSVGFrame in sync, no?  Or does the "null frame" thing there handle this?  In which case, why do we need to mention IsSpecialContent here, since all SVG content is special?

>Index: layout/base/nsCSSFrameConstructor.cpp
>+    // All SVG content is special...

So anything in the SVG namespace is special.  And anything in the SVG namespace ends up calling ConstructSVGFrame.  OK.  That works.  ;)

>@@ -7854,16 +7830,40 @@ nsCSSFrameConstructor::ConstructSVGFrame


>+  else if (aTag == nsGkAtoms::feDistantLight ||
....
>+           aTag == nsGkAtoms::feTurbulence) {

Why not just let these fall into the NS_NewSVGGenericContainerFrame case?  To save space?  If so, please have an nsSVGLeafFrame or something extending nsFrame?  And have it claim the right thing in IsFrameOfType()?  Then you also don't need to change the nsFrame protected constructor setup, which would be nice -- nsFrame is really not meant to be instantiated on its own.

r=bzbarsky with those changes.
Attachment #213686 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

12 years ago
(In reply to comment #7)
> >Index: content/svg/content/src/nsSVGElementFactory.cpp
> >+  // Make sure to keep nsCSSFrameConstructor::IsSpecialContent in
> >+  // synch with this code
> 
> Also keep ConstructSVGFrame in sync, no?  Or does the "null frame" thing there
> handle this?  In which case, why do we need to mention IsSpecialContent here,
> since all SVG content is special?

It was just a thought in case someone modifies IsSpecialContent in the future.

> >+  else if (aTag == nsGkAtoms::feDistantLight ||
> ....
> >+           aTag == nsGkAtoms::feTurbulence) {
> 
> Why not just let these fall into the NS_NewSVGGenericContainerFrame case?  To
> save space?

Space saving and to stop frame construction on invalid content.


(Assignee)

Comment 9

12 years ago
Created attachment 213714 [details] [diff] [review]
adjust per review comments
Attachment #213686 - Attachment is obsolete: true
Attachment #213714 - Flags: review?(bzbarsky)
Attachment #213714 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

12 years ago
Attachment #213714 - Flags: superreview?(dbaron)
(Reporter)

Updated

12 years ago
Blocks: 321107
Comment on attachment 213714 [details] [diff] [review]
adjust per review comments

There's no need to implement nsIFrame::GetType if you don't need it.  Other than that, sr=dbaron.
Attachment #213714 - Flags: superreview?(dbaron) → superreview+
(Assignee)

Comment 11

12 years ago
Checked in without GetType.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.