346288 - croczilla sample makes latest firefox security release (1.5.0.5) unresponsive

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Some people say it crashes for them.

Comment 1

[Brian Polidoro]

Attachment: talkback

nsXULContentUtils::GetElementRefResource 155c1a03

Comment 2

[tor]

bz, you did some modifications of XUL on the 1.8.0.x branch during the last cycle - could those have affected this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

I do see the crash Brian posted a talkback for -- the document is null in nsXULContentUtils::GetElementRefResource when we do:

248 bzbarsky   1.54              nsCOMPtr<nsIDocument> doc = aElement->GetDocument();
249 waterson   1.1       
250 jst        1.59              nsIURI *url = doc->GetDocumentURI();
251 waterson   1.1               NS_ASSERTION(url != nsnull, "element has no document");
252                              if (! url)
253                                  return NS_ERROR_UNEXPECTED;

The callstack is the same as the talkback.  The nsXULContentUtils::GetElementRefResource code hasn't changed since 2003.

aElement here is an nsSVGPolygonElement.  I has a null GetParent(), in addition to a null document,  We really shouldn't be tossing it around in template match code, but none of the code that's relevant has changed in a while.  The changes in bug 329677 were for nearby code, but that's not called from here.

When did this regress on the branch?  Figuring that out would help a lot...

Comment 4

[Boris Zbarsky [:bzbarsky]]

tor, were there other changes you were thinking of, btw?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, I see this in 1.8 too.

Comment 6

[Martijn Wargers (dead)]

This regressed on the 1.8.1 branch between 2006-04-24 and 200-04-25:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-04-24+05&maxdate=2006-04-25+14&cvsroot=%2Fcvsroot
Since this is also happening on the 1.8.0.5 branch, I think this is a regression from bug 334341.

Comment 7

[Martijn Wargers (dead)]

Hmm, no, backing out the patch from bug 334341 doesn't make a difference in my 1.8.1 tree.

Comment 8

[Martijn Wargers (dead)]

Backing out the patch from bug 335142 fixes the crash for me.

Comment 9

[Martijn Wargers (dead)]

Attachment: patch

This fixes the crash for me, not sure if it is any good. My 1.8.1 build doesn't have svg enabled, so I can't check if the svg image is rendered with the patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

What I want to know is why that's even needed.  Why are we passing around an element with no document and no parent in template code?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fwiw, with Martijn's patch I do see the SVG render on that page.  Not sure whether it's the right rendering, of course.

Comment 12

[Jonathan Watt [:jwatt]]

Comparison screenshot is: http://croczilla.com/svg/samples/xulsvg1/screenshot.png

Comment 13

[Boris Zbarsky [:bzbarsky]]

Maybe we should just land the null-check in 1.8.0.6, since we're doing it anyway...

Comment 14

[[e-mail]]

(In reply to comment #10)
>Why are we passing around an element with no document and no parent in template code?
Because of template recursion; basically templates don't just match containment arcs from the root, they go on to match arcs from those matching resources too. In XUL, particularly in menus and trees, the recursion is lazy and saves the template builder from generating content for menus that are never opened or treerows that are never expanded.
However as non-XUL elements don't support lazy templating, the template builder has to scan for and generate all the subcontent for the created elements. For performance reasons it does this in one hit before adding anything to the document, thus resulting in the crash as seen.

Comment 15

[[e-mail]]

Comment on attachment 231220 [details] [diff] [review]
patch

This is wrong in as much as we do need a document in order to support recursive non-XUL templates. However I don't know which document or whether to null-test it.

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK.  So maybe we should change the template builder to insert things into the document before doing things with them that expect a document to be around?

Neil, if the branch drivers agree to respin 1.8.0.6 for the null-check patch, do you think it's worth taking it?  It does help if there's no recursion happening, right?

Comment 17

[[e-mail]]

(In reply to comment #16)
>OK.  So maybe we should change the template builder to insert things into the
>document before doing things with them that expect a document to be around?
Well this is all tied up with getting the resource from an element. The template builder sets the id of the element to the child resource and then expects to be able to get that back so that it can find grandchildren.

>Neil, if the branch drivers agree to respin 1.8.0.6 for the null-check patch,
>do you think it's worth taking it?  It does help if there's no recursion
>happening, right?
I looked at what I think 1.4.x did, and it doesn't look as if it supports non-XUL recursion either as it null-checks its documents everywhere. So I'll say the null-check patch is good for a last-minute 1.8.0.6 respin, but I still don't think it's right for the 1.8.1 branch.

Comment 18

[Boris Zbarsky [:bzbarsky]]

That's fine.  I want this fixed on 1.8.0.x; then we can worry about other things.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Since this is a problem in 1.8.0.4 too, no need to try to get this into 1.8.0.6.

Comment 20

[Daniel Veditz [:dveditz]]

Is a 1.8.0 patch a realistic expectation? Boris wants it fixed, but it's not assigned to anyone.

Comment 21

[Boris Zbarsky [:bzbarsky]]

The null-check patch attached here should be enough for 1.8.0.7, worst-case.  Things won't work quite right in some rare cases, but they won't crash...

Would any of the Neils be willing to write up a working patch for trunk/1.8.1?  I don't think anyone else understands this code...

Comment 22

[Madis Kalme]

Just wanted to let you know that Seamonkey 1.0.4 started to behave like that (depending on the numbering system that I haven't grasped yet you might already know that) independent of the OSs I tested on: Windows 2K and XP MCE.

Comment 23

[[e-mail]]

Attachment: Branch patch

I believe this is a more correct fix than attachment 231220 [details] [diff] [review], although it displays incorrectly (maybe because I'm testing it in 256 colours?).

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233222 [details] [diff] [review]
Branch patch

Please add an "XXX XBL2/sXBL issue" comment here so we know to revisit this once those are settled.

Also, GetOwnerDoc() can still return null if the document has gone away.  Is that known to be impossible here?

Oh, and want to fix the text of the immediately following assertion to match what's being asserted?  ;)

Comment 25

[[e-mail]]

(In reply to comment #24)
>(From update of attachment 233222 [details] [diff] [review])
>Please add an "XXX XBL2/sXBL issue" comment here so we know to revisit this
>once those are settled.
>
>Also, GetOwnerDoc() can still return null if the document has gone away.  Is
>that known to be impossible here?
We've only just used the document to create the content in question... this call will return the document that was just used to create the content, right?
 
>Oh, and want to fix the text of the immediately following assertion to match
>what's being asserted?  ;)
As in "document has no uri"?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Yes to both.

Comment 27

[[e-mail]]

Attachment: Updated string as per review comment

Seeking branch approvals for this crash regression fix.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233222 [details] [diff] [review]
Branch patch

a=drivers, please land on MOZILLA_1_8_BRANCH

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233285 [details] [diff] [review]
Updated string as per review comment

a=drivers, please land on MOZILLA_1_8_BRANCH

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 233285 [details] [diff] [review]
Updated string as per review comment

approved for 1.8.0 branch, a=dveditz for drivers