73262 - Crash on quitting while page is loading

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Simon Fraser [no longer active]]

I just crashed twice while quitting when a page was loading. To repro:
1. Loaded the above URL into a browser window (I actually had it set to be my
   home page, so it loaded as the first page).

   www.verisign.com is being very slow to load right now, so that make this easy
   to repro.

2. Do Command-Q to quit the app. We crash in ~nsXULDocument; stack coming.
   we crash at observer->DocumentWillBeDestroyed(this); because the observer
   has been deleted already.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Stack log for the crash

Comment 2

[David Hyatt]

--> danm

Comment 3

[Paul Chen]

Also run into a crash in the same spot when closing the manage bookmarks window
with any folder node opened in tree (or could possibly be the specific node I
have opened) or when just plain surfing sometimes. For some reason, this doesn't
seem to show up on my win32 build. Adding myself to cc-list.

Comment 4

[Patrick C. Beard]

I get this same crash (same stack crawl) when I quit the browser, even if my 
default page is about:blank. Sounds like we got an overeager reference counting 
problem. Time to break out the reference count logger.

Comment 5

[Patrick C. Beard]

Looking in a debugger, at PresShell::~PresShell(), on line 1408 of 
nsPresShell.cpp:

    mFrameManager->Destroy();

During this call, the reference count of mDocument goes to 0, which then causes a 
crash when the destructor completes (mDocument is an nsCOMPtr<nsIDocument>). I 
also observe that this is the 3rd PresShell object to be destructed when this 
crash occurs.

Comment 6

[Simon Fraser [no longer active]]

this crash is frequent enough to be dogfood.

Comment 7

[Paul Chen]

Ok, the observer getting deleted is an nsMenuBar, there is code to remove the 
nsMenuBar as an observer from the document in nsMenuBar::~nsMenuBar(), but the 
call to GetDocument doesn't get the document.

Comment 8

[Paul Chen]

More digging, the nsWebShellWeakRef in nsMenuBar has a null mContentViewer in 
it's nsDocShell part. This returns null for nsMenuBar::GetDocument(), so the 
observer isn't removed from the document observer list.

Comment 9

[Patrick C. Beard]

One more clue, this is probably the hidden window, because it doesn't happen upon 
closing the last visible window, but when quitting after that. But you probably 
already knew that. No wonder this is happening only on the Mac.

Comment 10

[Paul Chen]

Beard, don't think so. I'm crashing just closing a browser window.

Comment 11

[Paul Chen]

So it looks like the docShell/webShell gets destroyed right before the 
nsMenuBar. Problem is that the nsMenuBar needs to docShell/webShell around to 
get to the nsXULDocument (as far as I can tell looking at the code).

Comment 12

[Simon Fraser [no longer active]]

Yeah, I've seen this when closing my 3rd open browser window. It's not hidden 
window.

Comment 13

[Paul Chen]

Ok, so I've found the change that started causing this problem. jst turned on 
the code that calls observer->DocumentWillBeDestroyed(this) on all the observers 
of the xul document in version 1.388 of nsXULDocument; this was previously in an 
#if 0. Nice. We've been masquerading this bug for who knows how long. Adding jst 
to cc list because misery loves company.

Comment 14

[Patrick C. Beard]

What I mean is that I can see the crash as well when no windows are visible.

Comment 15

[Paul Chen]

Attachment: patch for mac nsMenuBar.h, add weakref member for doc

Comment 16

[Paul Chen]

Attachment: patch to mac nsMenuBar.cpp, create weak ref to doc after adding nsMenuBar as doc observer

Comment 17

[Paul Chen]

Just attached patches; here's the lo-down.

The problem started happening after jst removed the #if 0 around code in 
~nsXULDocument which calls observer->DocumentWillBeDestroyed() on all the 
observers of the xul document. The crash occurs because there is an observer 
that's already deleted that didn't remove itself from the observer list. The mac 
debug memory allocator fills freed block contents with 0xEFEFEFEF, so it kinda 
hurts trying to call DocumentWillBeDestroyed(). The observer is the mac 
nsMenuBar, and the reason it doesn't remove itself as an observer from the 
nsXULDocument is because it uses a weak reference on the webshell to get at the 
document.

The problem is that the destruction sequence is first delete the webshell, then 
the nsMenuBar, and then the nsXULDocument. In the destructor for nsMenuBar, it 
tries to get at the document through the weak reference on webshell. 
Unfortuantely, the webshell has been deleted, so nsMenuBar can't get at the 
document, and thus, it can't remove itself from observer list.

My fix is to add a weak reference to the nsXULDocument which is set after the 
nsMenuBar is attached as an observer on the xul document. In the destructor for 
nsMenuBar, we just use that weak ref to the document to call RemoveObserver(). It 
certainly fixes my crash. ;-)

Awaiting r= and sr=

Comment 18

[Paul Chen]

got an r=pinkerton

Comment 19

[Simon Fraser [no longer active]]

sounds good, sr=sfraser

Comment 20

[Johnny Stenback (:jst)]

Me? nsXULDocument.cpp ver 1.388, DocumentWillBeDestroyed(), I don't think so:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsXULDocument.cpp&root=/cvsroot&subdir=mozilla/content/xul/document/src&command=DIFF_FRAMESET&rev1=1.387&rev2=1.388

Looks like waterson was the one who took out the #if 0 in version 1.389

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/xul/document/src&command=DIFF_FRAMESET&file=nsXULDocument.cpp&rev2=1.389&rev1=1.388

(this was the right thing to do, btw)

Anyway, there's no need to add nsWeakRef code here, just a raw nsIDocument
pointer in nsMenuBar would do, you set the pointer whenever the nsMenuBar is
added as a document observer, and you null out the pointer in
nsMenuBar::DocumentWillBeDestroyed().

And PLEASE attach cvs diff -u's and not cvs diff -c's. Thanks!

Comment 21

[Paul Chen]

Attachment: updated patch to nsMenubar.h to use nsIDocument * instead of weak reference

Comment 22

[Paul Chen]

Attachment: new patch to nsMenuBar.cpp, use nsIDocument* instead of weak ref, also set nsIDocument* to null in DocumentWillBeDestroyed()()

Comment 23

[Paul Chen]

Ok, so jst persuaded me to use just an nsIDocument*, actually, pinkerton 
mentioned it too, but I thought weak references were sexier. ;-) Also set the 
nsIDocument* to null in DocumentWillBeDestroyed().

Comment 24

[Simon Fraser [no longer active]]

Are you sure that the mDocument will live longer than you do? If so, sr=sfraser

Comment 25

[Johnny Stenback (:jst)]

If the document goes away before the nsMenuBar then the document will notify the
nsMenuBar (::DocumentWillBeDestroyed()) and it'll null out mDocument, after this
the document is gone and the nsMenuBar isn't a document observer any more so
there's no need to remove ourselves from the documents obeserver list.
Regardless of lifetime issues here, the attached patch(es) work. I like it!

r/sr=jst

Comment 26

[Patrick C. Beard]

Do we need to apply this patch in nsMenuBarX.cpp?

Comment 27

[Paul Chen]

Attachment: apply fix to nsMenuBarX.h

Comment 28

[Paul Chen]

Attachment: apply fix to nsMenuBarX.cpp

Comment 29

[Paul Chen]

Yes, we should apply the fixes to nsMenuBarX.h and nsMenuBarX.cpp. ;-)

Comment 30

[Johnny Stenback (:jst)]

r=jst for the nsMenuBarX files too, but please include this change (in both .coo
files), just to avoid uninitialized pointers...:

Index: widget/src/mac/nsMenuBarX.cpp
===================================================================
RCS file: /cvsroot/mozilla/widget/src/mac/nsMenuBarX.cpp,v
retrieving revision 1.5
diff -u -r1.5 nsMenuBarX.cpp
--- nsMenuBarX.cpp      2001/03/22 04:09:41     1.5
+++ nsMenuBarX.cpp      2001/04/01 22:47:17
@@ -66,7 +66,7 @@
 //
 // nsMenuBarX constructor
 //
-nsMenuBarX::nsMenuBarX()
+nsMenuBarX::nsMenuBarX() : mDocument(nsnull)
 {
     NS_INIT_REFCNT();
     mNumMenus       = 0;

Comment 31

[Paul Chen]

Fixes checked in with setting mDocument = nsnull in constructor (in both 
nsMenuBar.cpp and nsMenuBarX.cpp