Closed Bug 324988 Opened 19 years ago Closed 19 years ago

Crash if stop loading while favicons are coming in

Categories

(Core :: XUL, defect, P1)

Product:
Core
Component:
XUL
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: brettw, Assigned: bryner)

References

Details

(Keywords: crash, fixed1.8.0.7, fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

working fix (diff -w)
5.62 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
bzbarsky
: approval-branch-1.8.1+
dveditz
: approval1.8.0.7+
Details | Diff | Splinter Review 
If you open the places page with a screenful of history and lots of favicons and navigate to another page before the icons have finished loading, you will get a segfault. Occurs on trunk and 1.8 branch. You probably have to do that after starting the browser so the images aren't cached. You might also need to be running on a networked drive on Linux to make sure the images come in slowly enough. I've found the best way to get this is to start the browser, go to some page, click on places, and quickly hit back.

Stack trace:
#0  CancelImageRequest (aKey=0x8a6ebe8, aData=0x8a6ea98, aClosure=0x0)
    at /home/brettw/moz/branch/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:119
#1  0x401572e1 in hashEnumerate (table=0x8a6c458, hdr=0x0, i=0, arg=0x41a0da88)
    at /home/brettw/moz/branch/mozilla/xpcom/ds/nsHashtable.cpp:131
#2  0x40146a29 in PL_DHashTableEnumerate (table=0x8a6c458,
    etor=0x401572ca <hashEnumerate>, arg=0xbfffe4f0) at pldhash.c:621
#3  0x401579e5 in nsHashtable::Enumerate (this=0x8a6c450, aEnumFunc=0,
    aClosure=0x0)
    at /home/brettw/moz/branch/mozilla/xpcom/ds/nsHashtable.cpp:319
#4  0x4213ca73 in nsSupportsHashtable::Enumerate (this=0x8a6c450,
    aEnumFunc=0x4250bba4 <CancelImageRequest>, aClosure=0x0)
    at nsHashtable.h:221
#5  0x4250c45b in ~nsTreeBodyFrame (this=0x8963dcc)
    at /home/brettw/moz/branch/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:173
#6  0x4217c9de in nsFrame::Destroy (this=0x8963dcc, aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsFrame.cpp:674
#7  0x4250ccf0 in nsTreeBodyFrame::Destroy (this=0x8963dcc,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:366
#8  0x4218c931 in nsFrameList::DestroyFrames (this=0x8963cb8,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsFrameList.cpp:138
#9  0x42178a1d in nsContainerFrame::Destroy (this=0x8963c84,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsContainerFrame.cpp:161
#10 0x422c3680 in nsBoxFrame::Destroy (this=0x8963c84, aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1120
#11 0x4218c931 in nsFrameList::DestroyFrames (this=0x8963bf4,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsFrameList.cpp:138
#12 0x42178a1d in nsContainerFrame::Destroy (this=0x8963bc0,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsContainerFrame.cpp:161
#13 0x422c3680 in nsBoxFrame::Destroy (this=0x8963bc0, aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1120
#14 0x4218c931 in nsFrameList::DestroyFrames (this=0x89631e4,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsFrameList.cpp:138
#15 0x42178a1d in nsContainerFrame::Destroy (this=0x89631b0,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsContainerFrame.cpp:161
#16 0x422c3680 in nsBoxFrame::Destroy (this=0x89631b0, aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1120
#17 0x4218c931 in nsFrameList::DestroyFrames (this=0x8962ee4,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsFrameList.cpp:138
#18 0x42178a1d in nsContainerFrame::Destroy (this=0x8962eb0,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsContainerFrame.cpp:161
#19 0x422c3680 in nsBoxFrame::Destroy (this=0x8962eb0, aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1120
#20 0x4218c931 in nsFrameList::DestroyFrames (this=0x88d6358,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsFrameList.cpp:138
#21 0x42178a1d in nsContainerFrame::Destroy (this=0x88d6324,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsContainerFrame.cpp:161
#22 0x422c3680 in nsBoxFrame::Destroy (this=0x88d6324, aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1120
#23 0x4218c931 in nsFrameList::DestroyFrames (this=0x88d6268,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsFrameList.cpp:138
#24 0x42178a1d in nsContainerFrame::Destroy (this=0x88d6234,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsContainerFrame.cpp:161
#25 0x422c3680 in nsBoxFrame::Destroy (this=0x88d6234, aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1120
#26 0x4218c931 in nsFrameList::DestroyFrames (this=0x88c0b8c,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsFrameList.cpp:138
#27 0x42178a1d in nsContainerFrame::Destroy (this=0x88c0b58,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsContainerFrame.cpp:161
#28 0x422c3680 in nsBoxFrame::Destroy (this=0x88c0b58, aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1120
#29 0x4218c931 in nsFrameList::DestroyFrames (this=0x88c0828,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsFrameList.cpp:138
#30 0x42178a1d in nsContainerFrame::Destroy (this=0x88c07f4,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsContainerFrame.cpp:161
#31 0x422c3680 in nsBoxFrame::Destroy (this=0x88c07f4, aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1120
#32 0x4218c931 in nsFrameList::DestroyFrames (this=0x88c0798,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsFrameList.cpp:138
#33 0x42178a1d in nsContainerFrame::Destroy (this=0x88c0764,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsContainerFrame.cpp:161
#34 0x421ee61c in ViewportFrame::Destroy (this=0x88c0764,
    aPresContext=0x88c9d70)
    at /home/brettw/moz/branch/mozilla/layout/generic/nsViewportFrame.cpp:67
#35 0x42131535 in nsFrameManager::Destroy (this=0x888eb24)
    at /home/brettw/moz/branch/mozilla/layout/base/nsFrameManager.cpp:297
#36 0x4213fb2d in PresShell::Destroy (this=0x888eb08)
    at /home/brettw/moz/branch/mozilla/layout/base/nsPresShell.cpp:1954
#37 0x4211f938 in DocumentViewerImpl::Destroy (this=0x86c0948)
    at /home/brettw/moz/branch/mozilla/layout/base/nsDocumentViewer.cpp:1437
#38 0x4212059b in DocumentViewerImpl::Show (this=0x88eadb0)
    at /home/brettw/moz/branch/mozilla/layout/base/nsDocumentViewer.cpp:1715
#39 0x4213bf7a in nsPresContext::EnsureVisible (this=0x8838ad0,
    aUnsuppressFocus=0)
    at /home/brettw/moz/branch/mozilla/layout/base/nsPresContext.cpp:1293
#40 0x421482f0 in PresShell::UnsuppressAndInvalidate (this=0x890d108)
    at /home/brettw/moz/branch/mozilla/layout/base/nsPresShell.cpp:5029
#41 0x4214c34e in PresShell::ProcessReflowCommands (this=0x890d108,
    aInterruptible=1)
    at /home/brettw/moz/branch/mozilla/layout/base/nsPresShell.cpp:6942
#42 0x4215617e in ReflowEvent::HandleEvent (this=0x890d108)
    at /home/brettw/moz/branch/mozilla/layout/base/nsPresShell.cpp:6702
#43 0x4214baf1 in HandlePLEvent (aEvent=0x89b4720)
    at /home/brettw/moz/branch/mozilla/layout/base/nsPresShell.cpp:6719
#44 0x401aaf0a in PL_HandleEvent (self=0x89b4720)
    at /home/brettw/moz/branch/mozilla/xpcom/threads/plevent.c:688
#45 0x401aade3 in PL_ProcessPendingEvents (self=0x80b1d18)
    at /home/brettw/moz/branch/mozilla/xpcom/threads/plevent.c:623
#46 0x401ad9a2 in nsEventQueueImpl::ProcessPendingEvents (this=0x80b7898)
    at /home/brettw/moz/branch/mozilla/xpcom/threads/nsEventQueue.cpp:417
#47 0x412f199e in event_processor_callback (source=0x809e750,
    condition=G_IO_IN, data=0x41a0da88)
    at /home/brettw/moz/branch/mozilla/widget/src/gtk2/nsAppShell.cpp:67
#48 0x4067bddf in g_vsnprintf () from /usr/lib/libglib-2.0.so.0
#49 0x4065ab35 in g_get_current_time () from /usr/lib/libglib-2.0.so.0
#50 0x4065bb78 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#51 0x4065be8d in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#52 0x4065c58f in g_main_loop_run () from /usr/lib/libglib-2.0.so.0
#53 0x40382f5f in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
Oh yeah, that stack trace was on the 1.8 branch
Priority: -- → P1
Moving from Firefox/Places to Core/Layout:XUL since this is a core XUL crash.
Component: Places → XP Toolkit/Widgets: XUL
Keywords: crash
Product: Firefox → Core
(In reply to comment #2)
> Moving from Firefox/Places to Core/Layout:XUL since this is a core XUL crash.
> 

Or rather, XP Toolkit/XUL
Component: XP Toolkit/Widgets: XUL → XP Toolkit/Widgets: Trees
QA Contact: places → xptoolkit.trees
I think the reason it crashes is that we cancel the document's load group when loading a new page into the docshell, which cancels all of the image requests.  This happens before frame teardown, so the decoderObserver has already been nulled out by the time CancelImageRequest is called (see imgRequestProxy::Cancel).

We need to fix this without leaking the TreeImageListener objects, of course.  Since we need to keep 1 outstanding reference to the image listener, and the image request doesn't hold onto it long enough, maybe we should extend the image cache so that it maps from URL -> {request, listener}.

Another option would be to create a new loadgroup for these image requests, and add it to the document loadgroup.  As the loadgroup owner, the tree frame could observe OnStopRequest for the loadgroup and release the listeners as the requests are removed.  This works because the loadgroup observer is notified before the requests are actually cancelled.

Boris, any preference here, or other ideas?
Attached patch possible fix (obsolete) — — Splinter Review 
This patch ensures that the listener is released when the corresponding request is removed from the image cache.  I switched over to nsDataHashtable, which makes the code a good deal cleaner (tree frames are now slightly larger, but I don't think that's a big deal).

Note: I haven't yet tested this to confirm that the crash is gone
Attachment #210019 - Flags: review?(bzbarsky)
Comment on attachment 210019 [details] [diff] [review]
possible fix

This isn't quite right, the destructor gets invoked as temporary objects are created, so we end up cancelling the requests too early
Attachment #210019 - Attachment is obsolete: true
Attachment #210019 - Flags: review?(bzbarsky)
I'm no longer able to reproduce the crash with this patch.
Attachment #210025 - Flags: review?(bzbarsky)
Comment on attachment 210025 [details] [diff] [review]
working fix (diff -w)

Oh, excellent!  Sorry about the mess the first time around.  :(  This is a vast improvement.

It might be worth getting this in on the 1.8.0 branch too.
Attachment #210025 - Flags: superreview+
Attachment #210025 - Flags: review?(bzbarsky)
Attachment #210025 - Flags: review+
Comment on attachment 210025 [details] [diff] [review]
working fix (diff -w)

I'd like to get this on the branch since FF2 will be loading XUL files into the content area and thereby make this much easier to hit.
Attachment #210025 - Flags: branch-1.8.1?(bzbarsky)
Attachment #210025 - Flags: branch-1.8.1?(bzbarsky) → branch-1.8.1+
checked in on trunk and branch
Status: NEW → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
*** Bug 325343 has been marked as a duplicate of this bug. ***
Flags: blocking1.8.0.7+
Comment on attachment 210025 [details] [diff] [review]
working fix (diff -w)

may fix bug 256297
Attachment #210025 - Flags: approval1.8.0.7?
Comment on attachment 210025 [details] [diff] [review]
working fix (diff -w)

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #210025 - Flags: approval1.8.0.7? → approval1.8.0.7+
checked in on 1.8.0 branch
Keywords: fixed1.8.0.7
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: