375775 - Filter crash [@ nsSVGUtils::PaintChildWithEffects] with display:none

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Crashes [@ nsSVGUtils::PaintChildWithEffects] dereferencing 0xdddddde9.

Comment 1

[tor]

What appears to be happening is that display:none is causing the filter frame to go away, not something that we watch for.  One way of handling this would be the nsSVGFilterFrame supporting weak references and then using a weakptr to track the frame.  Are there more elegant ways of doing what we need in gecko?

Likely there are similar problems for other internally referenced objects (paint servers, gradients, markers, clipPaths).

Comment 2

[Boris Zbarsky [:bzbarsky]]

You can use an nsWeakFrame if you really need to, but it's expensive-ish.

That said, why are you holding pointers to frames at all?  Who's holding the pointer?  Why aren't they getting the frame as needed?  In general, frames can die at the drop of a hat, so holding pointers to them is very dangerous.

Comment 3

[tor]

We tend to store frame pointers in the properties of the frames they're referenced from, to avoid the lookup hit when painting.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Is the hit actually noticeable?  GetPrimaryFrameFor is just a hashtable lookup after the first time...

Comment 5

[tor]

Attachment: first cut at just fixing filters

Comment 6

[tor]

We need to consider what the appropriate behavior should be when a filter has display:none.

Option 1: Shouldn't display the filtered geometry - current behavior of the patch.

Option 2: Should display filtered geometry unfiltered.  This introduces a complication that we need to invalidate the union of the filter invalidation area and the geometry area when the filter frame comes/goes, as the filter region could be a subset of the geometry covered by the filter.  How do we find out when this happens?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you raise it with the SVG WG? It's definitely a spec issue.

Comment 8

[Jonathan Watt [:jwatt]]

Or:

Option 3: we should display the filtered geometry since the filter element isn't shown directly, so display:none on it shouldn't matter.

Comment 9

[tor]

(In reply to comment #8)
> Option 3: we should display the filtered geometry since the filter element
> isn't shown directly, so display:none on it shouldn't matter.

That one is tricky since we need the frames for style information on a few filter elements, and display:none stops the frame generation.

Comment 10

[Jonathan Watt [:jwatt]]

Oh sure, #3 would involved the most work by far since it doesn't fit into how things work now in Mozilla, but from a spec point of view it's probably how things should work I'd think.

Comment 11

[tor]

Actually it looks like #3 is what the spec requires: "'filter' elements are never rendered directly; their only usage is as something that can be referenced using the 'filter' property. The 'display' property does not apply to the 'filter' element; thus, 'filter' elements are not directly rendered even if the 'display' property is set to a value other than none, and 'filter' elements are available for referencing even when the 'display' property on the 'filter' element or any of its ancestors is set to none.".

Similar language is in the other items which have references to them (gradients, patterns, clipPaths, and masks).

Comment 12

[Boris Zbarsky [:bzbarsky]]

The elements in question could probably store references to their style contexts...  The problems come with knowing when to restyle them, since we don't restyle display:none subtrees, since CSS has this little invariant that display:none stuff never affects the rendering of the document...

Comment 13

[tor]

Wouldn't this involve modifying all SVG elements to store their style information, since any content can be the child of <marker>, <pattern>, <clipPath>, and <mask>?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Oh, you need info about the kids, not just the node itself?  Yeah, indeed.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You can get the style for an element the way nsComputedDOMStyle does, in the case where there's no frame. It'd be slower but this is a corner case.

http://lxr.mozilla.org/seamonkey/source/layout/style/nsComputedDOMStyle.cpp#314

Comment 16

[tor]

Moved discussion of general display:none handling in SVG to bug 376027.

Comment 17

[Jesse Ruderman]

The patch here no longer applies :(

Comment 18

[tor]

Attachment: checkin version - update to tip (NS_GetSVGFilterFrame signature change)

Comment 19

[tor]

Checked in.

Comment 20

[Jesse Ruderman]

Crashtest checked in.