329335 - Crash [@ nsXULDocument::~nsXULDocument] involving <svg:svg datasources=""> in XUL

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jesse Ruderman]

Mac trunk (both release and debug) crashes if you load the testcase and wait a few seconds.  I think the crash is exploitable.

Comment 1

[Jesse Ruderman]

Attachment: testcase - crashes Firefox

Comment 2

[Jesse Ruderman]

Attachment: stack trace - looks exploitable

Comment 3

[Jesse Ruderman]

The testcase reloads itself over and over until it crashes, so it crashes faster (e.g. in 2 seconds instead of 5) if you save it and then load your local copy.

Comment 4

[Daniel Veditz [:dveditz]]

I confirm the crash using windows on the trunk, calling a method on a deleted observer vtbl. The 1.8.0 branch does not crash. Neither does my 1.8 branch build, but it's a month old so we should check whether the current ff2 does: might give us a regression range or point us at trunk-only changes.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This regressed between 2006-02-13-06 and 2006-02-14-08.  Given "datasources", I posit bug 285631 is responsible.

Comment 6

[Neil Deakin]

Attachment: Remove the builder and document observer when the root node of a template is removed from the DOM

My stack trace isn't identical but similar.

The patch should also fix bugs 323988 and 327508.

Comment 7

[Boris Zbarsky [:bzbarsky]]

We're a document observer, no?  Why can't we just handle this in ContentRemoved without having to add support elsewhere?

Fwiw, I bet the null-check in nsXULDocument::SetTemplateBuilderFor was supposed to be checking aBuilder, not aContent...

Comment 8

[Neil Deakin]

(In reply to comment #7)
> We're a document observer, no?  Why can't we just handle this in ContentRemoved
> without having to add support elsewhere?

I originally did implement it that way, but ContentRemoved didn't seem to be called recursively.

> 
> Fwiw, I bet the null-check in nsXULDocument::SetTemplateBuilderFor was supposed
> to be checking aBuilder, not aContent...
> 

Likely, although there are no callers that pass null. I can change it if desired.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> but ContentRemoved didn't seem to be called recursively.

It's not -- you need to check whether the content removed is an ancestor of the root.

> Likely, although there are no callers that pass null.

Ah.  Then your change is OK if an assert is added that null is not being passed, I guess.

Comment 10

[Neil Deakin]

Attachment: Use ContentRemoved instead for notifications

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 214314 [details] [diff] [review]
Use ContentRemoved instead for notifications

>Index: content/xul/templates/src/nsXULTemplateBuilder.cpp

>+nsXULTemplateBuilder::ContentRemoved(nsIDocument* aDocument,

>+    nsIContent* parent = mRoot;
>+
>+    while (parent) {
>+        if (parent == aChild) {

How about:

  if (nsContentUtils::ContentIsDescendantOf(mRoot, aChild)) {
    // Do cleanup here

instead of doing the while loop yourself?

r+sr=bzbarsky with that change.

Comment 12

[Neil Deakin]

checked in

Comment 13

[Boris Zbarsky [:bzbarsky]]

This apparently caused bug 330167.  ;(

Comment 14

[Martijn Wargers (dead)]

Just wondering, could this have fixed bug 326468?

Comment 15

[Jesse Ruderman]

Crashtest checked in.