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

RESOLVED FIXED in mozilla1.9

Status

()

P2
critical
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: mcsmurf, Assigned: ajschult784)

Tracking

(Depends on: 1 bug, {crash, regression, testcase})

Trunk
mozilla1.9
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(8 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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?
(Reporter)

Comment 4

12 years ago
Correct, tried it, did not crash (with the "open new windows in tabs" option turned on).
Shouldn't be....

Comment 7

12 years ago
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]
Duplicate of this bug: 381257
(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 !
Status: NEW → ASSIGNED
Keywords: regression
I'm not too sure how I end up changing "Status  	NEW  	ASSIGNED" :-<
Whiteboard: [See Bug 381257 for steps]
Whiteboard: [See Bug 381257 for steps]
Oooops, usually mid-air collisions aren't so bad...
Whiteboard: [See Bug 381257 for steps]
(Assignee)

Comment 12

12 years ago
Created attachment 267092 [details]
testcase (part 4)

testcase part 4 -- closes the new tab
(Assignee)

Comment 13

12 years ago
Created attachment 267093 [details]
testcase (part 3)

a page to load in the iframe
(Assignee)

Comment 14

12 years ago
Created attachment 267094 [details]
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.
(Assignee)

Comment 15

12 years ago
Created attachment 267095 [details]
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
<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.
(Assignee)

Comment 17

12 years ago
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.
Keywords: testcase
Whiteboard: [See Bug 381257 for steps]
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)

Comment 28

12 years ago
Andrew, do you have time to work on this?
Assignee: nobody → ajschult
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 29

12 years ago
Created attachment 279225 [details] [diff] [review]
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.
Attachment #279225 - Flags: superreview?(bzbarsky)
Attachment #279225 - Flags: review?(bzbarsky)
(Assignee)

Comment 30

12 years ago
Created attachment 279226 [details] [diff] [review]
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.
Attachment #279226 - Flags: superreview?(neil)
Attachment #279226 - Flags: review?(neil)
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.
Attachment #279226 - Flags: superreview?(neil)
Attachment #279226 - Flags: superreview-
Attachment #279226 - Flags: review?(neil)
(Assignee)

Comment 32

12 years ago
Created attachment 279247 [details] [diff] [review]
rip it out

the previous patch did accomplish 99% of what anybody would want, but... ok
Attachment #279247 - Flags: superreview?(neil)
Attachment #279247 - Flags: review?(neil)
(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?
Attachment #279225 - Flags: superreview?(bzbarsky)
Attachment #279225 - Flags: superreview+
Attachment #279225 - Flags: review?(bzbarsky)
Attachment #279225 - Flags: review+
Attachment #279225 - Flags: approval1.9+
(Assignee)

Comment 36

12 years ago
Created attachment 279347 [details] [diff] [review]
rip it out (include pref)

Same as before, but also removes the pref and updates from browser notification bitrot.
Attachment #279247 - Attachment is obsolete: true
Attachment #279347 - Flags: superreview?(neil)
Attachment #279347 - Flags: review?(neil)
Attachment #279247 - Flags: superreview?(neil)
Attachment #279247 - Flags: review?(neil)
(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.
(Assignee)

Comment 39

12 years ago
>>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).
(Assignee)

Comment 40

12 years ago
Created attachment 280220 [details] [diff] [review]
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.
Attachment #280220 - Flags: review?(bzbarsky)
(Assignee)

Comment 41

12 years ago
Created attachment 280264 [details] [diff] [review]
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.
Attachment #280264 - Flags: superreview?(neil)
Attachment #280264 - Flags: review?(neil)

Updated

12 years ago
Attachment #280264 - Flags: superreview?(neil)
Attachment #280264 - Flags: superreview+
Attachment #280264 - Flags: review?(neil)
Attachment #280264 - Flags: review+
(Assignee)

Comment 42

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

this is checked in
Attachment #280264 - Attachment is obsolete: true
[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?
(Assignee)

Comment 45

12 years ago
> 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.
(Assignee)

Updated

12 years ago
Depends on: 396519
(Assignee)

Comment 47

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

(patch is in spinoff bug 396519)
Attachment #280220 - Flags: review?(bzbarsky)
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.
Attachment #279225 - Flags: approval1.9+
Comment on attachment 279225 [details] [diff] [review]
fix docshell
[Never landed]

Doesn't look like this ever landed. Re-requesting approval for after beta.
Attachment #279225 - Flags: approval1.9?
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.
Attachment #279225 - Flags: approval1.9? → approval1.9+

Updated

11 years ago
Priority: -- → P2

Comment 51

11 years ago
Reed, can you figure out what needs to be checked in here? It seems like this is getting neglected.
Whiteboard: [has-patch]
Keywords: checkin-needed
Whiteboard: [has-patch] → [c-n: "fix docshell"] [has-patch]
Attachment #280264 - Attachment description: avoid crash in tabbrowser → avoid crash in tabbrowser [Checkin: Comment 42]
Attachment #279226 - Attachment is obsolete: true
Attachment #280220 - Attachment description: encapsulate content viewer eviction → encapsulate content viewer eviction [Moved to bug 396519]
Attachment #280220 - Attachment is obsolete: true
Andrew, will you have time to check this in yourself, or do you want me to do it?
(Assignee)

Comment 53

11 years ago
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.
Keywords: checkin-needed, helpwanted
OS: Windows 2000 → All
Hardware: PC → All

Comment 54

11 years ago
renominate if there is a clear plan.
Flags: blocking1.9+ → blocking1.9-
Any update on which path to take?

Updated

11 years ago
Blocks: 398751
(Assignee)

Comment 56

11 years ago
Created attachment 307648 [details] [diff] [review]
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.
Attachment #307648 - Flags: superreview?(bzbarsky)
Attachment #307648 - Flags: review?(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.
Attachment #307648 - Flags: superreview?(bzbarsky)
Attachment #307648 - Flags: superreview+
Attachment #307648 - Flags: review?(bzbarsky)
Attachment #307648 - Flags: review+
Attachment #307648 - Flags: approval1.9?
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!
Attachment #307648 - Flags: approval1.9? → approval1.9+
(But yeah, mochis for this?  That would be super-sweet.)
(Assignee)

Comment 60

11 years ago
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"
(Assignee)

Comment 61

11 years ago
It should no longer be possible to crash in this way in EvictWindowContentViweers, so resolving FIXED.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Can you put the testcases in as crashtests or mochis, as appropriate?  That would be swell.  Thanks a bunch for the patch!
Flags: in-testsuite?
(Assignee)

Comment 63

11 years ago
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.
(Assignee)

Updated

11 years ago
Attachment #279347 - Flags: superreview?(neil)
Attachment #279347 - Flags: review?(neil)
Attachment #307648 - Attachment description: bail if passed indices are clearly bogus → bail if passed indices are clearly bogus [Checkin: Comment 60]
Attachment #279225 - Attachment description: fix docshell → fix docshell [Never landed]
Whiteboard: [c-n: "fix docshell"] [has-patch]
Target Milestone: --- → mozilla1.9
Attachment #280264 - Attachment is obsolete: false
Crash Signature: [@ nsSHistory::EvictWindowContentViewers]
You need to log in before you can comment on or make changes to this bug.