358599 - Crash when closing tab (SeaMonkey Undo Close Tab related) [@ nsSHistory::EvictWindowContentViewers]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Frank Wein [:mcsmurf]]

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]

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

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

(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?

Comment 4

[Frank Wein [:mcsmurf]]

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

Comment 5

[[e-mail]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Shouldn't be....

Comment 7

[Philip Chee]

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]

Comment 9

[Serge Gautherie (:sgautherie)]

(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 !

Comment 10

[Serge Gautherie (:sgautherie)]

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

Comment 11

[Caio Tiago Oliveira (asrail)]

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

Comment 12

[Andrew Schultz]

Attachment: testcase (part 4)

testcase part 4 -- closes the new tab

Comment 13

[Andrew Schultz]

Attachment: testcase (part 3)

a page to load in the iframe

Comment 14

[Andrew Schultz]

Attachment: testcase (part 2)

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.

Comment 15

[Andrew Schultz]

Attachment: testcase (part 1)

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

Comment 16

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

<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.

Comment 17

[Andrew Schultz]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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).

Comment 19

[[e-mail]]

(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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[[e-mail]]

(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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[[e-mail]]

(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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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?

Comment 25

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

(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?

Comment 26

[Boris Zbarsky [:bzbarsky]]

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?

Comment 27

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

(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)

Comment 28

[Benjamin Smedberg]

Andrew, do you have time to work on this?

Comment 29

[Andrew Schultz]

Attachment: fix docshell [Never landed]

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.

Comment 30

[Andrew Schultz]

Attachment: fix tabbbrowser to not set sessionHistory

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 31

[[e-mail]]

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.

Comment 32

[Andrew Schultz]

Attachment: rip it out

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

Comment 33

[[e-mail]]

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

Comment 34

[Boris Zbarsky [:bzbarsky]]

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 35

[Boris Zbarsky [:bzbarsky]]

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?

Comment 36

[Andrew Schultz]

Attachment: rip it out (include pref)

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

Comment 37

[[e-mail]]

(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).

Comment 38

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

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.

Comment 39

[Andrew Schultz]

>>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).

Comment 40

[Andrew Schultz]

Attachment: encapsulate content viewer eviction [Moved to bug 396519]

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.

Comment 41

[Andrew Schultz]

Attachment: avoid crash in tabbrowser [Checkin: Comment 42]

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 42

[Andrew Schultz]

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

this is checked in

Comment 43

[Serge Gautherie (:sgautherie)]

[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 44

[Boris Zbarsky [:bzbarsky]]

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?

Comment 45

[Andrew Schultz]

> 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.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Care to write some tests, then?

And yes, I think a separate bug would be good, esp. for regression-tracking.

Comment 47

[Andrew Schultz]

Comment on attachment 280220 [details] [diff] [review]
encapsulate content viewer eviction
[Moved to bug 396519]

(patch is in spinoff bug 396519)

Comment 48

[Damon Sicore (:damons)]

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 49

[Reed Loden [:reed]]

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 50

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 51

[Robert Sayre]

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

Comment 52

[Reed Loden [:reed]]

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

Comment 53

[Andrew Schultz]

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.

Comment 54

[Robert Sayre]

renominate if there is a clear plan.

Comment 55

[Reed Loden [:reed]]

Any update on which path to take?

Comment 56

[Andrew Schultz]

Attachment: bail if passed indices are clearly bogus [Checkin: Comment 60]

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 57

[Boris Zbarsky [:bzbarsky]]

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 58

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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!

Comment 59

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

Comment 60

[Andrew Schultz]

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"

Comment 61

[Andrew Schultz]

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

Comment 62

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

Comment 63

[Andrew Schultz]

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).

Comment 64

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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.