Crashes while the status bar counter says "65".

Same crash on the Gecko 1.8 branch.

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...

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

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.

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

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.

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.

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 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...

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

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.

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

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?

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

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

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.

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 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 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?

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

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

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.)

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

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.

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.

(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 on attachment 196558 [details] [diff] [review] patch for original testcase - make SVG frame construction more robust sr=bzbarsky

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

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.

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

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 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

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.