14450 - XULDocumentImpl dtor does not call DocumentWillBeDestroyed

Status: VERIFIED WONTFIX

(Core Graveyard :: RDF, defect, P3)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Shouldn't documents call the DocumentWillBeDestroyed method of its document
observers when it is being destroyed?

Thanks to [e-mail] for help in tracking this one down, and the fix (will
attach).

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: FIX: Calls DocumentWillBeDestroyed for doc observers in dtor

Comment 2

[Chris Waterson]

Heikki: if this patch works, then please check it in. Looks good to me.

Comment 3

[jst]

Waterson: (I'm answering on Heikkis behaf since he's leaving for a conferance
tomorrow morning and might not see this untill early next week) AFAIK heikki
does not have a CVS account (and neither do I) so he can't check it in, so if
you can, please check it in, I did verify that it both compiles and works (on
win32).

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Since I do not have CVS checkin rights, I am assigning this back to waterson.

(By the way Johnny, waterson did not see your message as he was not on the list
of people that get copies of changes in this bug. Remember to check assigned,
reporter, cc and QA fields ;)

Comment 5

[Chris Waterson]

ok then. thanks for the patch!

Comment 6

[jst]

Oops, sorry about that, I assumed that since it had been assigned to waterson he
would still get notified about status changes to the bug.

Comment 7

[chris hofmann]

what are some good ways to test this patch?

Comment 8

[Chris Waterson]

it does not need to be tested. it is the right thing to do.

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

If you are interested, here is how this bug was found and verified to be true:

I created a window. I wanted to know when the user closed the window. I grabbed
the XUL document of the opened window and added a document observer to it. Once
the user closes the window, the XUL document gets destroyed and it should (and
with the fix it does) call the observer's DocumentWillBeDestroyed() method.

If you look at nsDocument you will notice it does the same thing.

Comment 10

[Chris Waterson]

As an additional data point, I accidentally checked this in (unapproved) with
another bug fix. Turns out that on Mac, it causes a crash-on-exit (see 14911).
We'll need to get that sorted out before landing this. My guess is native menu
observers are doing something bad, like not removing themselves as document
observers when they get destroyed.

Comment 11

[jst]

I just took a look at nsXULDocument.cpp and around line 1988 (just above
AddObserver) there's a comment that says that the document doesn't keep a
reference to the document observer, in stead the document observer keeps a
live reference to the document! If this is the case then calling
DocumentWillBeDestroyed on the observers from the document destructor
wouldn't work since as long as one or more observers keep a reference
to the document the document wouldn't be destroyed!!!

This is confusing and IMO this can not work! IMO the document should keep a
strong reference to the observers and the observers should keep weak
references to the document. This way one could an observer that is
constructed, added as an observer to a document and then deleted with the
document unless there are other references to the observer. This is what
observers are for (among other things) right!

nsDocument has this same problem.

Comment 12

[Chris Waterson]

bulldozer to M12

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Um, have you checked this on the Mac lately? Bug 14911 was marked fixed, but has 
the situation changed so this fix could be checked in?

Comment 14

[Chris Waterson]

i don't know. try it. it's wrapped in an #ifdef.

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I can't, I don't have any suitable Macs I could use :/

Comment 16

[Chris Waterson]

marking WONTFIX. this caused problems, and I'm under the impression that none of 
the content models do this. let me know if i'm wrong.

Comment 17

[Tom Everingham]

verified WONTFIX