320488 - crash [@ nsSHistory::EvictWindowContentViewers]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Brian Ryner (not reading)]

Just getting this one on file... there are a lot of Talkback reports from Firefox 1.5 with crashes at nsSHistory::EvictWindowContentViewer.  Many of them mention reloading the page and/or clearing history/cache via prefs.

Here's a typical crash report:

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=12999605

Comment 1

[timeless]

is the claim that g... will really be the end of the list guaranteed?

Incident ID: 12999605
Stack Signature	nsSHistory::EvictWindowContentViewers() bf1a1217
Product ID	Firefox15
Build ID	2005111116
Trigger Time	2005-12-15 15:14:16.0
Platform	LinuxIntel
Operating System	Linux 2.6.14-1.1644_FC4
Module	firefox-bin + (004bb5f3)
URL visited	
User Comments	refreshed all tabs after clearing personal data
Since Last Crash	6 sec
Total Uptime	58 sec
Trigger Reason	SIGSEGV: Segmentation Fault: (signal 11)
Source File, Line No.	/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpfe/components/shistory/src/nsSHistory.cpp, line 848
Stack Trace 	
nsSHistory::EvictWindowContentViewers()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpfe/components/shistory/src/nsSHistory.cpp, line 848]
nsSHistory::EvictContentViewers()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpfe/components/shistory/src/nsSHistory.cpp, line 645]
DocumentViewerImpl::Show()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/layout/base/nsDocumentViewer.cpp, line 1160]
nsPresContext::EnsureVisible()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/layout/base/nsPresContext.cpp, line 1294]
PresShell::UnsuppressAndInvalidate()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/layout/base/nsPresShell.cpp, line 5022]
PresShell::UnsuppressPainting()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/layout/base/nsPresShell.cpp, line 5070]
PresShell::sPaintSuppressionCallback()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/layout/base/nsPresShell.cpp, line 2942]
nsTimerImpl::Fire()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/threads/nsTimerImpl.cpp, line 395]
handleTimerEvent()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/threads/nsTimerImpl.cpp, line 462]
PL_HandleEvent()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/threads/plevent.c, line 689]
PL_ProcessPendingEvents()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/threads/plevent.c, line 623]
nsEventQueueImpl::ProcessPendingEvents()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/xpcom/threads/nsEventQueue.cpp, line 421]
event_processor_callback()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/widget/src/gtk2/nsAppShell.cpp, line 67]
libglib-2.0.so.0 + 0x494fc (0x0075b4fc)
libglib-2.0.so.0 + 0x234ce (0x007354ce)
libglib-2.0.so.0 + 0x264d6 (0x007384d6)
libglib-2.0.so.0 + 0x267c3 (0x007387c3)
libgtk-x11-2.0.so.0 + 0x109a46 (0x03204a46)
nsAppShell::Run()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/widget/src/gtk2/nsAppShell.cpp, line 141]
nsAppStartup::Run()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp, line 151]
XRE_main()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/toolkit/xre/nsAppRunner.cpp, line 2315]
main()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/browser/app/nsBrowserApp.cpp, line 62]
libc.so.6 + 0x14d5f (0x0091ed5f)

Comment 2

[timeless]

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

Comment 3

[Marria Nazif]

I'm having trouble reproducing the crash.  Has anyone been able to reproduce?

Comment 4

[Adam Guthrie]

Bug 320742 seems to have some JS that causes this same crash.

Comment 5

[Matthew Waymost]

I think the crash here and the crash described in bug 320742 are slightly different. Here the crash is a seg fault when trying to get the transaction at startIndex in nsSHistory. In the other bug, the crash comes from an access violation when trying to get the entry from some transaction (since it's unclear at what transaction the crash is occurring in the for loop at line 850; it should be at nsSHistory->mIndex before modifications to it are made by the listener). You can also see in the stack traces that the events leading up to these crashes come from an event being fired (presumably when the history/cache is cleared) whereas the events leading to the crash in bug 320742 come from a nsWebProgressListener when a page is done loading.

Comment 6

[Brian Ryner (not reading)]

I managed to hit this once and figured out what I did.  These steps should reproduce the bug on any machine:

1. Set browser.sessionhistory.max_total_viewers to 3 and restart
2. Load a few sites until you have 7 pages in your session history.  That is, 6 pages show up when you hit the back button dropdown arrow.
3. Go back one page so you're on the next-to-last page in session history
4. Tools -> Clear Private Data, and just clear the browsing history
5. Reload

I didn't really look at what's needed to fix it, but it seems like at a minimum there's some edge case in the computation, and as a larger issue, why are we evicting anything on reload?

Comment 7

[Marria Nazif]

Attachment: update loaded index after history purge

Thanks, Brian, that was extremely helpful.  I found the issue - we were not updating the loaded index when the history was purged.  I'm attaching a patch for this, which will at least fix the crash.

As for attempting to evict at all on a reload - It's a good point that it's wasted effort.  The reason why we attempt to evict is because the previous and loaded indexes are not updated on a reload.  Also, there is apparently a valid mPreviousViewer in Show() on reload.  As a low effort optimization, we could potentially update previous and loaded indices if the load type is LOAD_CMD_RELOAD.  I'm still poking around to see if this makes sense.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 206660 [details] [diff] [review]
update loaded index after history purge

r=biesi

Comment 9

[Darin Fisher]

Comment on attachment 206660 [details] [diff] [review]
update loaded index after history purge

Please add a comment explaining why decrementing these indices by aNumEntries is important.  It wasn't obvious to me when I first read the patch.

sr=darin with that change.

Comment 10

[Marria Nazif]

Attachment: updated patch with comment

Thanks guys.  This is the updated patch with a comment.  I'm going to need help submitting it though since I don't have a cvs account.

Comment 11

[Darin Fisher]

Comment on attachment 206676 [details] [diff] [review]
updated patch with comment

This seems like something that we should consider for a minor update to FF 1.5

Comment 12

[Darin Fisher]

Comment on attachment 206676 [details] [diff] [review]
updated patch with comment

we also want this for FF 2.0

Comment 13

[Adam Guthrie]

Patch checked into trunk by timeless.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 206676 [details] [diff] [review]
updated patch with comment

Darin,bryner: can you make sure this gets in on the branch?

a=dveditz for 1.8/1.8.0.1 branches. Please add the fixed1.8.1 and fixed1.8.0.1 keywords when checked in.

Comment 15

[Brian Ryner (not reading)]

checked in on branches

Comment 16

[Marcia Knous [:marcia]]

verified on the branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1. I was not able to crash following bryner's steps in Comment 6.

Comment 17

[Brian Ryner (not reading)]

There are still reports like this coming in for 1.5.0.1, which has the above patch included.  Here are links for the 1.5.0.1 reports:

Windows / Mac: http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=begins&searchfor=nsSHistory%3A%3AEvictWindowContentViewers&vendor=MozillaOrg&product=Firefox15&platform=All&buildid=2006011112&sdate=&stime=&edate=&etime=&sortby=bbid

Linux: http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=begins&searchfor=nsSHistory%3A%3AEvictWindowContentViewers&vendor=MozillaOrg&product=Firefox15&platform=All&buildid=2006012415&sdate=&stime=&edate=&etime=&sortby=bbid  (none currently)

At least from the comments so far, users aren't triggering this via clearing history anymore, but are still triggering it some other way.

Comment 18

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

ajschult seems to have hit this; we're concerned that SeaMonkey's undo close tab may be exposing this crash.

Our implementation of the feature forces the currently loaded page into bfcache by opening a blank document (to save RAM by allowing for cache evicitions), then hides the tab until [number] more tabs have been closed, at which point the browser gets killed.  It hooks up a new session history to the browser first though, to avoid clobbering any forward-history on that browser.

Comment 19

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

If this is reopened, should the fixed1.8.* keywords be cleared?

Comment 20

[Boris Zbarsky [:bzbarsky]]

This bug is fixed.  Please file a new bug with detailed steps to reproduce your _different_ problem.  Preferably with code that could be run with UniversalXPConnect from local disk or something.

General hint: if you have to do something weird to session history to trigger the crash, then it's not this bug, even if the stack is similar.