Sometimes it happens that SeaMonkey crashes when you close a tab (not reproducible). This is probably due to the Undo Close Tab feature on SeaMonkey trunk. This bug here looks similar to Bug 320488, but it's not the same. Also note Bug 350416 Comment 26, it seems like it is not clear yet if Undo Close Tab is implemented in a Gecko-friendly way. Stacktrace: ChildEBP RetAddr 0012e0b8 01e6602c docshell!nsSHistory::EvictWindowContentViewers(int aFromIndex = 0, int aToIndex = 5)+0x89 [h:\mozilla\tree-main\mozilla\docshell\shistory\src\nsshistory.cpp @ 798] 0012e0c4 01806b5b docshell!nsSHistory::EvictContentViewers(int aPreviousIndex = 972944920, int aIndex = 998869240)+0x14 [h:\mozilla\tree-main\mozilla\docshell\shistory\src\nsshistory.cpp @ 654] 0012e120 01816ee9 gklayout!DocumentViewerImpl::Show(void)+0xe4 [h:\mozilla\tree-main\mozilla\layout\base\nsdocumentviewer.cpp @ 1925] 0012e158 017fcc04 gklayout!nsPresContext::EnsureVisible(int aUnsuppressFocus = 0)+0xc9 [h:\mozilla\tree-main\mozilla\layout\base\nsprescontext.cpp @ 1358] 0012e18c 017fcd27 gklayout!PresShell::UnsuppressAndInvalidate(void)+0x15 [h:\mozilla\tree-main\mozilla\layout\base\nspresshell.cpp @ 4907] 0012e198 01803fbe gklayout!PresShell::UnsuppressPainting(void)+0x59 [h:\mozilla\tree-main\mozilla\layout\base\nspresshell.cpp @ 4955] 0012e1a4 01e501f7 gklayout!DocumentViewerImpl::Stop(void)+0x4a [h:\mozilla\tree-main\mozilla\layout\base\nsdocumentviewer.cpp @ 1644] 0012e38c 01e536bc docshell!nsDocShell::SetupNewViewer(class nsIContentViewer * aNewViewer = 0x39045bc0)+0x46d [h:\mozilla\tree-main\mozilla\docshell\base\nsdocshell.cpp @ 5913] 0012e3a8 01e53cf7 docshell!nsDocShell::Embed(class nsIContentViewer * aContentViewer = 0x39045bc0, char * aCommand = 0x01e795d8 "", class nsISupports * aExtraInfo = 0x00000000)+0x1e [h:\mozilla\tree-main\mozilla\docshell\base\nsdocshell.cpp @ 4532] 0012e40c 01e566d9 docshell!nsDocShell::CreateAboutBlankContentViewer(void)+0x2b5 [h:\mozilla\tree-main\mozilla\docshell\base\nsdocshell.cpp @ 4920] 0012e56c 01e4bdb2 docshell!nsDocShell::InternalLoad(class nsIURI * aURI = 0x04ff1740, class nsIURI * aReferrer = 0x00000000, class nsISupports * aOwner = 0x00000000, unsigned int aFlags = 1, unsigned short * aWindowTarget = 0x3a13c228, char * aTypeHint = 0x00000000 "", class nsIInputStream * aPostData = 0x00000000, class nsIInputStream * aHeadersData = 0x00000000, unsigned int aLoadType = 0x10000001, class nsISHEntry * aSHEntry = 0x00000000, int aFirstParty = 1, class nsIDocShell ** aDocShell = 0x00000000, class nsIRequest ** aRequest = 0x00000000)+0x69a [h:\mozilla\tree-main\mozilla\docshell\base\nsdocshell.cpp @ 6408] 0012e600 01e4d990 docshell!nsDocShell::LoadURI(class nsIURI * aURI = 0x04ff1740, class nsIDocShellLoadInfo * aLoadInfo = 0x00000000, unsigned int aLoadFlags = 0, int aFirstParty = 1)+0x354 [h:\mozilla\tree-main\mozilla\docshell\base\nsdocshell.cpp @ 861] 0012e6b0 10042ddd docshell!nsDocShell::LoadURI(unsigned short * aURI = 0x0206fa30, unsigned int aLoadFlags = 0x1000, class nsIURI * aReferringURI = 0x00000000, class nsIInputStream * aPostStream = 0x00000000, class nsIInputStream * aHeaderStream = 0x00000000)+0x1e7 [h:\mozilla\tree-main\mozilla\docshell\base\nsdocshell.cpp @ 2790] 0012e6e4 01275552 xpcom_core!XPTC_InvokeByIndex(class nsISupports * that = 0x39fdf628, unsigned int methodIndex = 8, unsigned int paramCount = 5, struct nsXPTCVariant * params = 0x0012e708)+0x27 [h:\mozilla\tree-main\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp @ 102] 0012e754 00000000 xpc3250!XPCWrappedNative::CallMethod(class XPCCallContext * ccx = 0x00ec6998, XPCWrappedNative::CallMode mode = 968028632 (No matching enumerant))+0x6cd [h:\mozilla\tree-main\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp @ 2162]

Need steps to reproduce... I'm not sure whether bryner is still dealing with bfcache stuff; if he's not, the code is effectively unowned, so without an easy way to debug nothing will happen with it.

One thing to try, btw, would be to create something that opens and closes lots of tabs (with the Seamonkey close tab thing in place) programmatically and see whether that reproduces the crash...

(In reply to comment #2) > One thing to try, btw, would be to create something that opens and closes lots > of tabs (with the Seamonkey close tab thing in place) programmatically and see > whether that reproduces the crash... > mcsmurf, you tried that, right?

Correct, tried it, did not crash (with the "open new windows in tabs" option turned on).

I don't suppose this is relevant, but could 823-824 be trashing trans? http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/shistory/src/nsSHistory.cpp&mark=798,823-824&rev=#796

Shouldn't be....

Here's a talkback id: TB31880143Y Stack Signature nsSHistory::EvictWindowContentViewers ded7e5a9 Product ID MozillaTrunk Build ID 2007042908 Trigger Time 2007-05-07 01:46:28.0 Platform Win32 Operating System Windows NT 5.1 build 2600 Module docshell.dll + (0002112a) URL visited User Comments Since Last Crash 8871 sec Total Uptime 241372 sec Trigger Reason Access violation Source File, Line No. d:\builds\tinderbox\seamonkeytrunk\winnt_5.2_clobber\mozilla\docshell\shistory\src\nsshistory.cpp, line 798 Stack Trace nsSHistory::EvictWindowContentViewers [mozilla/docshell/shistory/src/nsshistory.cpp, line 798] nsSHistory::EvictContentViewers [mozilla/docshell/shistory/src/nsshistory.cpp, line 654] nsPresContext::EnsureVisible [mozilla/layout/base/nsprescontext.cpp, line 1351] PresShell::UnsuppressAndInvalidate [mozilla/layout/base/nspresshell.cpp, line 4245] PresShell::UnsuppressPainting [mozilla/layout/base/nspresshell.cpp, line 4305] nsDocShell::Stop [mozilla/docshell/base/nsdocshell.cpp, line 3196] nsDocShell::InternalLoad [mozilla/docshell/base/nsdocshell.cpp, line 6731] nsScriptSecurityManager::GetPrincipalAndFrame [mozilla/caps/src/nsscriptsecuritymanager.cpp, line 2145] nsScriptSecurityManager::GetSubjectPrincipal [mozilla/caps/src/nsscriptsecuritymanager.cpp, line 2187] nsDocShell::LoadURI [mozilla/docshell/base/nsdocshell.cpp, line 871] nsDocShell::LoadURI [mozilla/docshell/base/nsdocshell.cpp, line 2800] NS_InvokeByIndex_P [mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102] XPCWrappedNative::CallMethod [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2246]

(In reply to comment #1) > Need steps to reproduce... (In reply to comment #8) > *** Bug 381257 has been marked as a duplicate of this bug. *** See there for steps :-) Not reduced, but I hope it should be a good starting point !

I'm not too sure how I end up changing "Status NEW ASSIGNED" :-<

Oooops, usually mid-air collisions aren't so bad...

testcase part 4 -- closes the new tab

a page to load in the iframe

popup window (opened in a new tab) I was trying to figure out how to have this work with part 3 in bugzilla cleanly, but it would be overly complicated, so you have to download each part and dump them in the same directory.

opens test2 in a new tab Dump each part in a directory 0. Set appropriate prefs. "windows to open in tabs" browser.link.open_newwindow = 3 (and browser.link.open_newwindow.restriction = 0, which is default) 1. Load test.html (testcase part 1) 2. Click the button (opens test2 in a new tab) 3. Click a link (click1) in the iframe and then the other one (click2). Repeat. I have to load 4-6 links to get it to crash. It's the same page each time, but with a query string that forces a reload. 4. Click the "then click me" button ==> submits the form, the new page closes the tab and SeaMonkey crashes

<CTho> ajschult: so, replacing the <form> with a button that just does window.close() causes the testcase to not crash? <ajschult> CTho: correct Clicking 4 times has crashed each time I tried.

OK, I think I figured out what seems going on. the newly-loaded page closes its tab and tabbrowser.xml makes a new session history and swaps it in place http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/suite/browser/tabbrowser.xml&rev=1.181&mark=1326-1328#1315 docshell's SetSession implementation holds a reference to the new session history, but does nothing to update its own internal state http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/docshell/base/nsDocShell.cpp&rev=1.834&mark=3255#3254 specifically, it does not update mPreviousTransIndex or mLoadedTransIndex The page finishes loading and nsDocumentViewer::Show attempts to evict content viewers. It gets mPreviousTransIndex and mLoadedTransIndex from the docshell and passes those to the session history. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsDocumentViewer.cpp&rev=1.527&mark=1799-1806#1786 Unfortunately, the session history has only one entry and the indices it receives are high enough that the session history thinks it should be evicting content viewers and everything goes downhill from there.

Hmm... So I'm not sure anyone ever designed SetSessionHistory to work if the docshell already has a session history. It's not clear exactly what the method should do in that case, esp if there have already been loads in the docshell (as here). Frankly, this method shouldn't really be exposed at all; session history is a docshell implementation detail... Why does this code need to reset the session history exactly? Ideally we would change this code to do something else and rip out the existing consumers of SetSessionHistory (including all the JS using it) and just have a way to tell docshell that it should set up a session history... at least that's what I think would make the most sense. Sadly, this is a semi-frozen nsIWebNavigation API, so we do need to do _something_ to not crash here (e.g. bail out if a non-null SH is passed in when we already have one).

(In reply to comment #15) >the new page closes the tab and SeaMonkey crashes So it seems to me that an easy way to prevent the crash (verified locally) is to disallow undo for windows closed by script. (In reply to comment #18) >Frankly, this method shouldn't really be exposed at all; session history is a >docshell implementation detail... Why does this code need to reset the session >history exactly? Because it's the only way we can call nsPIDOMWindow::SaveWindowState without messing up the session history.

Why do you need to call SaveWindowState manually? Why not just do an about:blank load in the webnavigation?

(In reply to comment #20) >Why not just do an about:blank load in the webnavigation? We do, we just can't afford to trash the existing session history.

Why would loading about:blank trash the existing session history?

(In reply to comment #22) >Why would loading about:blank trash the existing session history? Why wouldn't it? The only time loading about:blank doesn't touch your session history is when the current page is already about:blank.

Oh, I see. So all you'd need to do would be to go back one when restoring the tab, then get rid of the about:blank entry from the session history, no?

(In reply to comment #24) > Oh, I see. So all you'd need to do would be to go back one when restoring the > tab, then get rid of the about:blank entry from the session history, no? > Wouldn't that clobber forward-history?

Ah, I see. If you want to preserve both history directions, then yeah, you need to do something else... Out of curiousity, at what point do you drop the ref to the saved-off history if the user never undoes the tab close?

(In reply to comment #26) > Out of curiosity, at what point do you drop the ref to the saved-off history > if the user never undoes the tab close? http://lxr.mozilla.org/seamonkey/source/suite/browser/tabbrowser.xml#1336 An entry in savedBrowsers is an Object {browser: oldBrowser, history: oldSH}, where: var oldSH = oldBrowser.webNavigation.sessionHistory; and oldBrowser is the browser associated with the tab you're closing (this code is all in removeTab)

Andrew, do you have time to work on this?

This basically implements the final bit from comment 18, with the exception that passing a null session history when we have one is still bad because we always bail if the new one is null.

This clobbers forward history as mentioned in comment 25 (actually, you get a bogus forward history with about:blank after restoring the tab), but is otherwise functional.

Comment on attachment 279226 [details] [diff] [review] fix tabbbrowser to not set sessionHistory Sorry, that's unreasonable. If we can't use bfcache to undo close tab then don't bother trying.

the previous patch did accomplish 99% of what anybody would want, but... ok

(In reply to comment #32) >the previous patch did accomplish 99% of what anybody would want, but... ...but destroying forward history isn't acceptable.

The other option would be to make SetSessionHistory(not null) really work. That is, reget the docshell internal state, etc. But then we have to worry about a session history being passed in that doesn't match what we currently have displayed, which is just as bad. If people really want to use bfcache for this, would it work to expose ways to explicitly save and restore the presentation?

Comment on attachment 279225 [details] [diff] [review] fix docshell [Never landed] Looks ok if we don't want to try to make this work. But document the idl accordingly?

Same as before, but also removes the pref and updates from browser notification bitrot.

(In reply to comment #34) >If people really want to use bfcache for this, would it work to expose ways to >explicitly save and restore the presentation? What we would want to be able to do is to freeze the presentation and tear down the frame tree etc. (and then restore it again of course).

Neil, I don't think we should leave the feature in if we're not absolutely certain we can't have sessionhistory fixed or an alternate implementation (e.g. toolkit-style) written in time for releases.

>>If people really want to use bfcache for this, would it work to expose ways to >>explicitly save and restore the presentation? >What we would want to be able to do is to freeze the presentation and tear down >the frame tree etc. (and then restore it again of course). I tried getting this to work. I exposed CaptureState and then added an UnCaptureState (ok, bad name) that is pretty similar to RestorePresentation. It mostly works, but after restoration it seems the appropriate events don't get fired. So, nothing calls EndPageLoad (so mLSHE is non-null). Probably other bad things going on, but that's the biggest issue I'm noticing. Ah, also it seems nothing calls nsPresShell::Freeze because the events that would normally trigger that (after CaptureState) don't happen. Anyway, it seems to me that that fixing SetSessionHistory (robustly) would be hard... exposing setters for the m{Prev,Loaded}TransactionIndex would be easy, but there's lots of other state and it's hard to imagine that no other edge cases are out there waiting to bite us. That said, why doesn't nsSHistory evict its own content viewers from methods like GoBack and AddEntry (rather than being told to from nsDocumentViewer::Show)? The API (and code) would be so much simpler and we'd avoid all this mess (assuming no other problems are lurking).

This implements what I mentioned in the previous comment. nsSHistory takes responsibility for evicting content viewers (based on distance from current focus). Global eviction still happens from nsDocumentViewer::Show. Conceivably, this could happen from within nsSHistory in some method that gets called regularly (to further simplify things), but I'm not what would be most appropriate. The docshell state bits (previous and loaded indices) that were causing problems don't exist. The one issue I found was that if GotoIndex is called (and the new index is far away from the old one), the old index won't have a content viewer (yet), but once the load is finished, docshell will save the content viewer, and it's effectively lost (never evicted). Current behavior is the it gets saved and immediately evicted, which (while better than losing it) seems silly. With this patch, nsSHistory tags the entry so that docshell won't save the content viewer in the first place. The tag should get removed if the entry is ever loaded again. I've left nsSHistory's mRequestedIndex field and docshell still calls nsSHistory::UpdateIndex. It seems the primary use of these is gone, but I don't know that removing them wouldn't have side effects in some cases.

This avoids the crash case and is needed even with attachment 280220 [details] [diff] [review]. In the problem case with attachment 280220 [details] [diff] [review] applied, removeTab is being called from the page's onload handler, so when we loadURI("about:blank") that load replaces the page that was loading. And then we can't "go back" because there's nothing to go back to and things go downhill from there.

Comment on attachment 280264 [details] [diff] [review] avoid crash in tabbrowser [Checkin: Comment 42] this is checked in

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007090902 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007091002 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007091102 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4) (For the record, bug still there, per testcase URL, after attachment 280264 [details] [diff] [review] checkin. Looking forward for the other patches...)

Comment on attachment 280220 [details] [diff] [review] encapsulate content viewer eviction [Moved to bug 396519] I'll try to get to this ASAP, but this is really pretty scary code. Do we have any tests exercising this code or this patch? Would it make sense to spin this patch off into a separate bug?

> I'll try to get to this ASAP, but this is really pretty scary code. Do we > have any tests exercising this code or this patch? There are docshell tests, but they don't seem to load enough pages to trigger content viewer eviction. So the code seems mostly not tested. > Would it make sense to spin this patch off into a separate bug? Well, it seems that the recent tabbrowser patch doesn't actually fix this -- we still crash for tabs closed by script. So, this bug isn't fixed. We could still spin the patch off if it would make things easier to follow.

Care to write some tests, then? And yes, I think a separate bug would be good, esp. for regression-tracking.

Comment on attachment 280220 [details] [diff] [review] encapsulate content viewer eviction [Moved to bug 396519] (patch is in spinoff bug 396519)

Comment on attachment 279225 [details] [diff] [review] fix docshell [Never landed] Resetting approval flags on bugs that have not been checked in by Oct 22, 11:59PM PDT. Please re-request approval if needed.

Comment on attachment 279225 [details] [diff] [review] fix docshell [Never landed] Doesn't look like this ever landed. Re-requesting approval for after beta.

Comment on attachment 279225 [details] [diff] [review] fix docshell [Never landed] a=endgame drivers for M10, can only be landed after tree re-opens post-M9 freeze.

Reed, can you figure out what needs to be checked in here? It seems like this is getting neglected.

Andrew, will you have time to check this in yourself, or do you want me to do it?

Reed, see comment 34. Landing attachment 279225 [details] [diff] [review] would mean SeaMonkey would need an alternative (less spiffy) way to undo tab close, which Neil seems to be not a fan of. attachment 280220 [details] [diff] [review] would make it so that calling setSessionHistory should "work", and (as requested by Boris) that patch is now in bug 396519. Realistically, either bug 396519 or attachment 279225 [details] [diff] [review] should land for 1.9.

renominate if there is a clear plan.

Any update on which path to take?

This just makes EvictWindowContentViewers bail if the arguments are clearly bogus (probably cause a crash). I didn't want to do this because 1) It hides the real problem. but... Bug 396519 would solve this stuff properly. 2) Content viewers that should have been evicted might not be. but... they'll get evicted eventually by the expiration timer 3) Somewhat less wrong indices could result in content viewers being evicted that shouldn't be. but... this shouldn't be a huge problem (back/forward just not so fast) 4) Other problems might show up due to the same underlying problem. but... in the most problematic case, SeaMonkey avoids this situation (attachment 280264 [details] [diff] [review]). In the ctho.ath.cx testcase, SeaMonkey seems OK with this patch. The tab can be unclosed although it immediately recloses itself. If other problem cases arise we can try to avoid them in SeaMonkey code.

Comment on attachment 307648 [details] [diff] [review] bail if passed indices are clearly bogus [Checkin: Comment 60] Awesome. I agree that it's better to fix the underlying problem, but having this check (and esp. the asserts) is a good thing to have. If it's possible to also add some tests that would exercise this code, that would be awesome. I don't know whether your tests in the other bug would hit this. Drivers, I think this change is safe to take for 1.9 and that we should absolutely take it: if nothing else, it prevents memory-safety crashes in some cases.

Comment on attachment 307648 [details] [diff] [review] bail if passed indices are clearly bogus [Checkin: Comment 60] [*] bz-recommended [*] crash-safety [*] helps SeaMonkey Easy call: a=shaver, thanks for the work!

(But yeah, mochis for this? That would be super-sweet.)

Comment on attachment 307648 [details] [diff] [review] bail if passed indices are clearly bogus [Checkin: Comment 60] I landed this. As mentioned to bz on IRC, index == mLength is bad, so I changed the "<= mLength" tests to "< mLength" and "> mLength" to ">= mLength"

It should no longer be possible to crash in this way in EvictWindowContentViweers, so resolving FIXED.

Can you put the testcases in as crashtests or mochis, as appropriate? That would be swell. Thanks a bunch for the patch!

A unit test as simple as var history = Components.classes["@mozilla.org/browser/shistory;1"] .createInstance(Components.interfaces.nsISHistoryInternal); history.evictContentViewers(7, 10); would suffice, but unit tests run with fatal assertions, so it's a non-starter. A mochitest could be similar, but a test failure (crash) wouldn't be nice and bug 404077 would make assertions fatal there too. And it appears crashtests run in the reftest framework (no messing with history directly).

K -- can we track the needed test infra somewhere, and mark a "put these in as crashtests" bug as dep? Don't want this to get lost, as this code needs all the test-help it can get.