282027 - crash when displaying a SVG marker defined in a hidden SVG group

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Gildas Trebaol]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050210
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050210

A <svg:defs> section defined in a <svg:g style="display:none"> element
contains a <svg:marker>. It crashes Mozilla.


Reproducible: Always

Steps to Reproduce:
1. http://gtrebaol.free.fr/bugzilla/marker_crash.svg

Actual Results:  
The <marker> defined in a <defs> section inside a hidden <g> element
crashes Mozilla somewhere in "GKLAYOUT.dll".


Expected Results:  
Even if a <defs> section is inside a hidden <g> element, the <marker>
should behave like in a <defs> section not defined in a <g> element.

Comment 1

[tor]

Attachment: bulletproofing

Fixes the crash, but has the same problem as bug 282028 in that
display=none means we don't have the frames around to reference.

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 174512 [details] [diff] [review]
bulletproofing

Initialising with nsnull should be unnecessary in both cases. Both
GetPrimaryFrameFor and CallQueryInterface *need* to make sure the pointer they
return is nulled out on failure. If that wasn't the case then that's what we
should correct.

All we should really care about here is that we get a frame. I would prefer you
just use:

  if (!frame)
    return NS_ERROR_NO_INTERFACE;  // or NS_ERROR_FAILURE

It seems GetPrimaryFrameFor always returns NS_OK anyway.

Comment 3

[Jonathan Watt [:jwatt]]

Err, scrap NS_ERROR_NO_INTERFACE. I was rushing and got confused with CallQI.

Comment 4

[tor]

Attachment: minimal fix, also do nsSVGClipPathFrame

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 174516 [details] [diff] [review]
minimal fix, also do nsSVGClipPathFrame

r=jwatt

Since GetDocument is deprecated, it would be nice if you could also make the
following changes in both files.

-  nsCOMPtr<nsIDOMDocument> doc = do_QueryInterface(aContent->GetDocument());
+  nsCOMPtr<nsIDOMDocument> doc = do_QueryInterface(aContent->GetCurrentDoc());

and

-  nsCOMPtr<nsIPresShell> ps =
do_QueryInterface(aContent->GetDocument()->GetShellAt(0));
+  nsCOMPtr<nsIPresShell> ps = do_QueryInterface(doc->GetShellAt(0));

Comment 6

[tor]

Checked in.  Changing summary to reflect new status of bug.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> +  nsCOMPtr<nsIPresShell> ps = do_QueryInterface(doc->GetShellAt(0));

Err.... why does that need the QI?

Comment 8

[tor]

Attachment: remove do_QueryInterface

Copying an old mistake.  Offhand it looks like the svg frame code might
be overusing do_QueryInterface in a number of places.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175288 [details] [diff] [review]
remove do_QueryInterface

This code is wrong.  In particular, printing SVG is broken with this setup...

This is all frame code; if these are non-member methods, the caller should pass
in a presshell or a frame that a presshell can be gotten from, so we use the
right presshell.

Comment 10

[Jonathan Watt [:jwatt]]

The original crash is fixed. See bug 376027 for the display:none thing.