306915 - XUL/SVG crash [@ nsFrame::GetAscent]

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Comment 1

[Jesse Ruderman]

Attachment: apple crash report with stack trace

Comment 2

[Jesse Ruderman]

Attachment: rdf file used by the testcase (xulsvg1.rdf)

Comment 3

[Jesse Ruderman]

Attachment: testcase (not reduced - reveals 306663 code)

Comment 4

[Jesse Ruderman]

Crashes while the status bar counter says "65".

Comment 5

[Jesse Ruderman]

Same crash on the Gecko 1.8 branch.

Comment 6

[Boris Zbarsky [:bzbarsky]]

A reduced testcase would help a lot...  Based on the asserts I see, though,
there are some SVG issues, and the actual cause of the crash is:

###!!! ASSERTION: A box layout method was called but InitBoxMetrics was never
called: 'metrics', file ../../../mozilla/layout/generic/nsFrame.cpp, line 5601

And indeed, our frame's display is NS_STYLE_DISPLAY_INLINE (it's an
nsSVGPolygonFrame) but we're calling nsFrame::GetAscent on it (from
nsSprocketLayout::GetAscent).

This looks like fallout from the box/frame merge...

Comment 7

[Martijn Wargers (dead)]

Attachment: Testcase

Well, this is what I got when trying to minimise the "testcase (not reduced)"
testcase.
It might not be related, but it still crashes my trunk build.
When removing the datasources attribute, it doesn't crash. Also when using a
xul vbox as element, it doesn't crash. 
It seems like every xhtml/svg/... other than xul crashes with it.
Talkback ID's: TB9077983W TB9077959W

Comment 8

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050905
Firefox/1.6a1

Martijn's testcase crashed for me with the instruction pointer at 0xf800a1d8. 
The next stack item is nsXULTemplateBuilder::DocumentWillBeDestroyed or
nsXULDocument::~nsXULDocument, depending on whether you ask Talkback or the
Apple crash reporter tool.  Talkback Indicent ID: 9078530.

Comment 9

[Jesse Ruderman]

Attachment: another apple crash report for the reduced testcase

0x00000000
nsAttrAndChildArray::~nsAttrAndChildArray
nsGenericElement::~nsGenericElement
nsXULElement::~nsXULElement
...

Comment 10

[Jesse Ruderman]

I got another stack like the one in comment 9 but with a nonzero address at the
top of the stack.  This crash seems exploitable.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for Martijn's testcase

A quick look in the debugger at Martijn's testcase made it look like the crash
was due to |this| being deleted mid-function.  This is the obvious fix,
although not necessarily the correct one.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for original testcase - make SVG frame construction more robust

This makes the SVG frame construction code more robust -- i.e., much less
likely to construct combinations of frames that don't work together.  See
comments for spec analysis.

Boris, I'm specifically interested in your thoughts on whether "return NS_OK"
is the right thing to do here.	I haven't thought about it very much yet,
myself.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 195208 [details] [diff] [review]
patch for original testcase - make SVG frame construction more robust

We should also make sure this is the right thing regarding anonymous content --
I'd think getting the content node from the parent frame will do the right
thing for XBL and XTF, but it's worth checking...

Comment 14

[Boris Zbarsky [:bzbarsky]]

|return NS_OK| here just means "construct frame by display type instead".  Is
that what we want to be doing?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 195208 [details] [diff] [review]
patch for original testcase - make SVG frame construction more robust

No.  We want the equivalent of display:none.  I'll poke around.

Comment 16

[Boris Zbarsky [:bzbarsky]]

You probably want a "stop" out param like ConstructXULFrame has.

Comment 17

[Neil Deakin]

Comment on attachment 195207 [details] [diff] [review]
patch for Martijn's testcase

seems fine, but why is the template builder being deleted in there anyway?

Comment 18

[Boris Zbarsky [:bzbarsky]]

The template builder is being deleted because the only ref to it at this point
is from mDB, and it's removing that ref?

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Presumably, yeah.  It crashed on mDB = nsnull, so it seems that the cycle being
broken was actually the last reference.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for original testcase - make SVG frame construction more robust

I'm still a little nervous about the way nsXBLService and nsXBLResourceLoader
use the undisplayed content map, but not adding things that aren't actually
'display:none' to the undisplayed content map will reduce thrashing, and adding
would be pointless for the primary use of the map.

Comment 21

[Neil Deakin]

I see. I had assumed that the document was keeping a strong reference to any
builders it had, but that doesn't seem to be the case. Perhaps it should. (See
nsXULDocument::SetTemplateBuilderFor)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195294 [details] [diff] [review]
patch for original testcase - make SVG frame construction more robust

sr=bzbarsky, but we should think about a better way to do the stuff that XBL is
doing...

Comment 23

[tor]

Comment on attachment 195294 [details] [diff] [review]
patch for original testcase - make SVG frame construction more robust

>+#ifdef MOZ_SVG_FOREIGNOBJECT
>+  if (aParentFrame && aParentFrame->GetContent() &&
>+      aParentFrame->GetContent()->GetNodeInfo()->
>+        Equals(nsSVGAtoms::foreignObject, kNameSpaceID_SVG)) {
>+    // The container is a svg:foreignObject, which SVG 1.1, section 23.2
>+    // says should never contain SVG children (even SVG), so don't
>+    // display anything.
>+    // Style mutation can't change this situation, so don't bother
>+    // adding to the undisplayed content map.
>+    *aHaltProcessing = PR_TRUE;
>+    return NS_OK;
>+  }
>+#endif

My reading of the discussion on www-svg suggests that they realize that this is
a mistake, and any content (appropriately namespaced) can be a child of
foreignObject.

>+  if (aTag != nsSVGAtoms::svg &&
>+      (!aParentFrame || !aParentFrame->GetContent() ||
>+       aParentFrame->GetContent()->GetNameSpaceID() != kNameSpaceID_SVG)) {
>+    // Sections 5.1 and G.4 of SVG 1.1 say that SVG elements not contained
>+    // within svg:svg are incorrect, although they don't seem to specify
>+    // error handling.  Ignore them, since many of our frame classes
>+    // can't deal.  It *may* be that the document should at that point
>+    // be considered in error according to F.2, but it's hard to tell.
>+    // Style mutation can't change this situation, so don't bother
>+    // adding to the undisplayed content map.
>+    *aHaltProcessing = PR_TRUE;
>+    return NS_OK;
>+  }

Could you adjust the comment to clarify that this test is only for
non-<svg:svg> SVG elements?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for original testcase - make SVG frame construction more robust

This addresses tor's comments, but if I remove the foreignObject section, I
need to add checks for foreignObject elsewhere.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seems to break constructing kids of svg:a for some reason...

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Because of the binding that uses an *element* in the xlink namespace. 
Technically it's against the rules of the spec to construct frames for such a
thing.  But it would also probably be better if we used implementation of
nsISVGContainerFrame to make these tests.  However, I don't understand why:
 * nsSVGForeignObjectFrame does implement nsISVGContainerFrame
 * pretty much all the other SVG frame classes do not (Pretty much all SVG
elements allow children, although in some cases it's only animate.  But there
are some that do allow children now (other than animate) and don't implement
nsISVGContainerFrame.)

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for original testcase - make SVG frame construction more robust

The wacky element I don't like is from bug 267657.  This makes it an svg:g
instead.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, this might actually not fix the bug for some cases where XBL extends
is used, so a frame test might really be better.  I don't even know how we
implement xbl:extends.

Comment 29

[Boris Zbarsky [:bzbarsky]]

David, the reason we didn't use an svg:g to start with (and in fact the reason
this whole binding rigmarole is needed) is that the xlink:* attributes only work
on nsXMLElement instances in Gecko.  Which means that they don't actually work
on SVG elements, XHTML elements, or XUL elements.

That is, with this patch the link won't be a link, as far as I can tell.

> I don't even know how we implement xbl:extends.

xbl:extends just affects the localname and namespace ID that frame construction
uses for constructing the frame.  That's all it does.  So in this case, it
causes an svg frame of some sort to be constructed for the <xlink:xlink> element.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #29)
> xbl:extends just affects the localname and namespace ID that frame construction
> uses for constructing the frame.  That's all it does.  So in this case, it
> causes an svg frame of some sort to be constructed for the <xlink:xlink> element.

Yeah, I'd remembered aXBLBaseTag and ResolveTag by this point, though it seems
like it ought to do something else too.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for original testcase - make SVG frame construction more robust

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196558 [details] [diff] [review]
patch for original testcase - make SVG frame construction more robust

sr=bzbarsky

Comment 33

[tor]

Comment on attachment 196558 [details] [diff] [review]
patch for original testcase - make SVG frame construction more robust

>+  PRBool parentIsSVG = PR_FALSE;
>+  if (aParentFrame && aParentFrame->GetContent()) {
>+    nsCOMPtr<nsIAtom> tag;

Nit - call this parentTag or similar.

r=tor

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

attachment 195207 [details] [diff] [review] checked in to trunk 2005-09-19 22:15 -0700.

attachment 196558 [details] [diff] [review] with suggested variable name changes checked in to trunk,
2005-09-19 12:55 -0700.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Patches checked in on MOZILLA_1_8_BRANCH, 12:36 -0700 and 12:33 -0700.

Comment 36

[Bob Clary [:bc] (inactive)]

I get a glibc detected corrupted double linked list with this testcase on linux and a 20060221 1.0.8 build. 

<http://test.mozilla.com/tests/mozilla.org/security/306915-01.html>, click open xul page, then close the xul page to reproduce.

Comment 37

[Bob Clary [:bc] (inactive)]

Attachment: v.zip (valgrind log of ff 1.0.8)

Comment 38

[Bob Clary [:bc] (inactive)]

Attachment: v.zip (valgrind log with better options and a debug build)

Comment 39

[timeless]

Comment on attachment 212945 [details]
v.zip (valgrind log with better options and a debug build)

For people reading this log file, the starting point of interest is:

WARNING: waaah!, file /work/mozilla/builds/ff/1.0.x/mozilla/content/xul/document/src/nsXULPrototypeDocument.cpp, line 900

Comment 40

[Boris Zbarsky [:bzbarsky]]

Yeah, that valgrind log basically shows this bug, looks like to me -- the RemoveObserver destroys |this| and then we access members.

Since these patches never landed on the 1.7 and aviary branches, that makes sense.