Closed Bug 346288 Opened 14 years ago Closed 14 years ago

croczilla sample makes latest firefox security release (1.5.0.5) unresponsive

Categories

(Core :: SVG, defect)

1.8 Branch
x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jwatt, Assigned: neil)

References

()

Details

(Keywords: regression, verified1.8.0.7, verified1.8.1)

Attachments

(3 files, 1 obsolete file)

Some people say it crashes for them.
Attached file talkback
nsXULContentUtils::GetElementRefResource 155c1a03
bz, you did some modifications of XUL on the 1.8.0.x branch during the last cycle - could those have affected this?
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...

tor, were there other changes you were thinking of, btw?
Oh, I see this in 1.8 too.
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Flags: blocking1.8.0.6? → blocking1.8.0.7?
Hmm, no, backing out the patch from bug 334341 doesn't make a difference in my 1.8.1 tree.
Backing out the patch from bug 335142 fixes the crash for me.
Blocks: 335142
Attached patch patchSplinter Review
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.
Attachment #231220 - Flags: superreview?(neil)
Attachment #231220 - Flags: review?(enndeakin)
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?
Fwiw, with Martijn's patch I do see the SVG render on that page.  Not sure whether it's the right rendering, of course.
Maybe we should just land the null-check in 1.8.0.6, since we're doing it anyway...
Flags: blocking1.8.0.6?
(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 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.
Attachment #231220 - Flags: superreview?(neil) → superreview-
Attachment #231220 - Flags: review?(enndeakin)
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?
(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.
That's fine.  I want this fixed on 1.8.0.x; then we can worry about other things.
Since this is a problem in 1.8.0.4 too, no need to try to get this into 1.8.0.6.
Flags: blocking1.8.0.6?
Flags: blocking1.8.1? → blocking1.8.1+
Is a 1.8.0 patch a realistic expectation? Boris wants it fixed, but it's not assigned to anyone.
Flags: blocking1.8.0.7? → blocking1.8.0.7+
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...
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.
Attached patch Branch patch (obsolete) — Splinter Review
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?).
Attachment #233222 - Flags: superreview?(bzbarsky)
Attachment #233222 - Flags: review?(enndeakin)
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?  ;)
Attachment #233222 - Flags: superreview?(bzbarsky) → superreview+
Attachment #233222 - Flags: review?(enndeakin) → review+
(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"?
Yes to both.
Seeking branch approvals for this crash regression fix.
Attachment #233222 - Attachment is obsolete: true
Attachment #233285 - Flags: approval1.8.1?
Attachment #233285 - Flags: approval1.8.0.8?
Attachment #233285 - Flags: approval1.8.0.7?
Comment on attachment 233222 [details] [diff] [review]
Branch patch

a=drivers, please land on MOZILLA_1_8_BRANCH
Attachment #233222 - Flags: approval1.8.1+
Comment on attachment 233285 [details] [diff] [review]
Updated string as per review comment

a=drivers, please land on MOZILLA_1_8_BRANCH
Attachment #233285 - Flags: approval1.8.1? → approval1.8.1+
Keywords: fixed1.8.1
Comment on attachment 233285 [details] [diff] [review]
Updated string as per review comment

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #233285 - Flags: approval1.8.0.8?
Attachment #233285 - Flags: approval1.8.0.7?
Attachment #233285 - Flags: approval1.8.0.7+
Keywords: regression
Assignee: general → neil
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.0.7
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.