228774 - M17beta crash on close if datasource empty? [@ nsXULDocument::RemoveSubtreeFromDocument]

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[basic]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6b) Gecko/20031216

I've this xul app that uses the sql component
(http://www.mozilla.org/projects/sql/) to access data from a postgresql database. 
Mozilla crashes when I do the following and the result I get back is empty:

1) I add the empty result datasource to my xul tree
2) do a rebuild
3) close the xul document

#0  0x406e7006 in nanosleep () from /lib/i686/libc.so.6
#1  0xffffffa0 in ?? ()
#2  0x08070886 in ah_crap_handler(int) (signum=11)
    at ../../../xpfe/bootstrap/nsSigHandlers.cpp:135
#3  0x41c7bf67 in nsProfileLock::FatalSignalHandler(int) (signo=0)
    at ../../../../profile/dirserviceprovider/src/nsProfileLock.cpp:195
#4  0x401004ec in __pthread_clock_settime () from /lib/i686/libpthread.so.0
#5  0x40664ca8 in __libc_sigaction () from /lib/i686/libc.so.6
#6  0x415eea20 in nsXULDocument::RemoveSubtreeFromDocument(nsIContent*) (
    this=0x82197d8, aElement=0x845bb58)
    at ../../../../../content/xul/document/src/nsXULDocument.cpp:1959
#7  0x415eea20 in nsXULDocument::RemoveSubtreeFromDocument(nsIContent*) (
    this=0x82197d8, aElement=0x845a0e0)
    at ../../../../../content/xul/document/src/nsXULDocument.cpp:1959
#8  0x415bdaae in nsXBLBinding::ChangeDocument(nsIDocument*, nsIDocument*) (
    this=0x8459320, aOldDocument=0x82197d8, aNewDocument=0x4173038c)
    at ../../../../content/xbl/src/nsXBLBinding.cpp:1019
#9  0x415db6b7 in nsBindingManager::ChangeDocumentFor(nsIContent*, nsIDocument*,
nsIDocument*) (this=0x8450f38, aContent=0x8291ed0, aOldDocument=0x82197d8,
    aNewDocument=0x0) at ../../../../content/xbl/src/nsBindingManager.cpp:553
#10 0x413f5858 in nsGenericElement::SetDocument(nsIDocument*, int, int) (
    this=0x8291ed0, aDocument=0x0, aDeep=1, aCompileEventHandlers=1)
    at ../../../../content/base/src/nsGenericElement.cpp:1694
#11 0x41642f02 in nsXULElement::SetDocument(nsIDocument*, int, int) (
    this=0x84b0630, aDocument=0x0, aDeep=1, aCompileEventHandlers=1)
    at ../../../../../content/xul/content/src/nsXULElement.cpp:1825
#12 0x41642f02 in nsXULElement::SetDocument(nsIDocument*, int, int) (
    this=0x84b0598, aDocument=0x0, aDeep=1, aCompileEventHandlers=1)
    at ../../../../../content/xul/content/src/nsXULElement.cpp:1825
#13 0x413bea4c in nsDocument::SetScriptGlobalObject(nsIScriptGlobalObject*) (
    this=0x82197d8, aScriptGlobalObject=0x0)
    at ../../../../content/base/src/nsDocument.cpp:1580
#14 0x413d7912 in DocumentViewerImpl::Close() (this=0x8473a40)
    at ../../../../content/base/src/nsDocumentViewer.cpp:1022
#15 0x41f56d97 in nsDocShell::SetupNewViewer(nsIContentViewer*) (
    this=0x8444530, aNewViewer=0x846b1b0)
    at ../../../docshell/base/nsDocShell.cpp:4782
#16 0x41f54b26 in nsDocShell::Embed(nsIContentViewer*, char const*,
nsISupports*) (this=0x8444530, aContentViewer=0x846b1b0, aCommand=0x41fb1085 "",
    aExtraInfo=0x0) at ../../../docshell/base/nsDocShell.cpp:4126
#17 0x41f56052 in nsDocShell::CreateContentViewer(char const*, nsIRequest*,
nsIStreamListener**) (this=0x8444530,
    aContentType=0x8467828 "application/vnd.mozilla.xul+xml",
    request=0x8483be0, aContentHandler=0x8483a50)
    at ../../../docshell/base/nsDocShell.cpp:4536
#18 0x41f8145e in nsDSURIContentListener::DoContent(char const*, int,
nsIRequest*, nsIStreamListener**, int*) (this=0x8444470,
    aContentType=0x8467828 "application/vnd.mozilla.xul+xml",
    aIsContentPreferred=0, request=0x8483be0, aContentHandler=0x8483a50,
    aAbortProcess=0xfffffffc)
    at ../../../docshell/base/nsDSURIContentListener.cpp:109
#19 0x41f8728f in nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*,
nsIChannel*) (this=0x8483a40, aListener=0x8444470, aChannel=0x8483be0)
    at ../../../uriloader/base/nsURILoader.cpp:669
#20 0x41f8606f in nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*)
(this=0x8483a40, request=0x8483be0, aCtxt=0x0)
    at ../../../uriloader/base/nsURILoader.cpp:413
#21 0x41f85b28 in nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*)
    (this=0x8483a40, request=0x8483be0, aCtxt=0x0)
    at ../../../uriloader/base/nsURILoader.cpp:287
#22 0x40e0e1df in nsFileChannel::OnStartRequest(nsIRequest*, nsISupports*) (
    this=0x8483be0, req=0x8457150, ctx=0x0)
    at ../../../../../netwerk/protocol/file/src/nsFileChannel.cpp:568
#23 0x40d8b5e0 in nsInputStreamPump::OnStateStart() (this=0x8457150)
    at ../../../../netwerk/base/src/nsInputStreamPump.cpp:377
#24 0x40d8b508 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (
    this=0x8457150, stream=0x8456f8c)
    at ../../../../netwerk/base/src/nsInputStreamPump.cpp:333
#25 0x40a5017b in nsInputStreamReadyEvent::EventHandler(PLEvent*) (
    plevent=0xfffffffc) at ../../../xpcom/io/nsStreamUtils.cpp:116
#26 0x40a70703 in PL_HandleEvent (self=0x846f83c)
    at ../../../xpcom/threads/plevent.c:671
#27 0x40a70549 in PL_ProcessPendingEvents (self=0x810c1f0)
    at ../../../xpcom/threads/plevent.c:606
#28 0x40a728c3 in nsEventQueueImpl::ProcessPendingEvents() (this=0x810c1c8)
    at ../../../xpcom/threads/nsEventQueue.cpp:391
#29 0x41bfcc95 in event_processor_callback (source=0x823ccb0,
    condition=G_IO_IN, data=0x407689a0)
    at ../../../../widget/src/gtk2/nsAppShell.cpp:67

Comment 1

[Chase Tingley]

*** Bug 231725 has been marked as a duplicate of this bug. ***

Comment 2

[Chase Tingley]

The dup (bug 231725) has a testcase.  I didn't look at it closely, so I'm not
sure it involved the datasource being empty.  Dup was on WinXP, so platform ->
All.  Manel reported that the same testcase works fine in Moz 1.4.1, so +regression.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So... this looks like a straight-out bug in XUL.  The code in question is:

    PRUint32 count =
        xulcontent ? xulcontent->PeekChildCount() : aElement->GetChildCount();

    while (count-- > 0) {
        rv = RemoveSubtreeFromDocument(aElement->GetChildAt(count));

Now the problem is that PeekChildCount(), unlike GetChildCount(), doesn't
actually ensure that content is generated before returning the number of kids. 
On the other hand, GetChildAt() _does_ ensure that the content is generated (and
returns null if that fails).  So what happens here is that PeekChildCount()
returns a nonzero number, and then GetChildAt returns null (and then we crash in
RemoveSubtreeFromDocument).

The reason EnsureContentsGenerated() is failing, of course, is that this node is
not in the document to start with, which is the underlying cause of the bug. But
this code should probably still be changed to not use PeekChildCount() (since it
will turn around and call GetChildAt, which will EnsureContentsGenerated anyway,
as far as I can tell).  The assert on mDocument will still fire with that change...

sicking, can you just remove all this crap?  ;)

Comment 4

[Johnny Stenback (:jst)]

Attachment: Maybe just like this?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 144185 [details] [diff] [review]
Maybe just like this?

'twould fix it, yes.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 144185 [details] [diff] [review]
Maybe just like this?

Seems unneccesary that we generate children just to destroy them. But so did
the old code, so r=me

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 144185 [details] [diff] [review]
Maybe just like this?

Trivial change that fixes a crash, would be good to have one less crasher in
1.7 final.

Comment 8

[chris hofmann]

Comment on attachment 144185 [details] [diff] [review]
Maybe just like this?

a=chofmann for 1.7

Comment 9

[Johnny Stenback (:jst)]

Fix checked in.

Comment 10

[Jay Patel [:jay]]

Adding topcrash keyword for tracking since this is showing up in the latest
Talkback data for Mozilla 1.7 Beta.

Comment 11

[Jay Patel [:jay]]

Verified fixed based on latest Talkback data for MozillaTrunk and Mozilla 1.7 rc1.