nsSVGUtils::GetCanvasTM makes some major assumptions

VERIFIED FIXED

Status

()

Core
SVG
--
major
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: bz, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want])

In particular it assumes something about the concrete classes of all SVG frames that return true or false from IsLeaf().  If that assumption gets violated, it'll call virtual methods via the wrong vtable, and then bad things happen.

Is there a reason to not just have an interface implemented by SVG frames that hands out the object we want here?  Or a reason to not stick the method on a common superclass of all SVG frames?
I'd love to have a common superclass, but we don't ATM. In fact I'd like to rethink our class structure at some point. There have been several occasions where it has just seemed wrong to me in the past. The fact that we static_cast in hundreds of places in the SVG code makes me nervous, since we do have assumptions like this that are going to bite us occasionally as me make changes.
OS: Mac OS X → All
Hardware: x86 → All

Updated

9 years ago
Whiteboard: [sg:want]
There's a fix for this in the patch on bug 488314.
Depends on: 488314
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
So... what fixed this?  Bug 488314 isn't fixed, right?
I'm fixing bug 488314 in two parts, and the first part, which is now checked in, fixes this. I've explicitly marked the patch as checked in now. Sorry about that.
Ah, gotcha.  All good then.  Verified via code inspection, though we're still assuming that any SVG frame that's not a container is a geometry frame...
Status: RESOLVED → VERIFIED
Ideally I'd like to get rid of pretty much all our casts, with a very few exceptions such as where frames cast their mContent. Even that's not always safe though, since we reuse some frames that sound like they're specific to a certain content class for multiple content classes (nsSVGGFrame). That's all a bigger task though. :-(
You need to log in before you can comment on or make changes to this bug.