Open these two URLs in two tabs in the same window. in Camino 2.0.1: http://www.yelp.com/biz/armandinos-salumi-seattle http://www.yelp.com/biz/caffe-umbria-seattle Then press Cmd-W twice in rapid succession to close both tabs. Camino will crash every time. Was told this is probably a core bug and I attempted to repro it in the equivalent version of Firefox (3.0.17?) but I could not do it because Firefox does not accept the second Cmd-W keystroke command so soon after the first one. You can press it but nothing happens if you do it too quickly. By waiting about a second between closing tabs, the crash is avoided in Camino as well.
Armen's crash reports mostly have garbage frame 0s (e.g. bp-a6d9bf44-a385-42c8-b003-ca8ab2100123), but bp-98b83361-98fb-47fc-9d8e-395fe2100121 and bp-aa5a1cb0-065e-416d-94d6-f6a872100123 both look better, with perhaps the latter missing frames 0 and 1 from the former. I can reproduce the crash with the URLs Armen provided in comment 0 (I get a garbage frame 0 in my Cm2.1a1pre debug build, too, but otherwise things line up starting with nsDocShell::Destroy() as frame 1). Boris, do you have any insight here?
It looks like this crash could also show up with the top of the stack messed up down to nsDocShell::Destroy (which is what I said in my comment in comment 1, but not what I meant to say), e.g. bp-93152da1-c22f-40f4-a821-cfa7b2100123 [@ @0x0 | nsDocShell::Destroy() ] (this one was Armen again) and bp-cebcb1a9-7056-4462-8dfe-199ef2100119 [@ nsDocShell::Destroy() ]
> Boris, do you have any insight here? Not offhand, no. I assume this is only reproducible in Camino, not Firefox?
Does changing nsWebBrowser::SetDocShell to hold a strong ref to the docshell on the stack fix the crash for you?
Yes, adding NS_ADDREF(aDocShell); at nsWebBrowser.cpp:1592 does fix the crash. It leaks, though, so I guess we need to release it in nsWebBrowser::InternalDestroy()?
I really did mean to hold the ref on the stack. As in, use an nsCOMPtr on the stack to hold the ref just for the duration of that method call.
I put nsCOMPtr<nsIDocShell> kungFuDeathGrip(aDocShell); at nsWebBrowser.cpp:1589, but it still crashed: #0 0x002d6404 in nsVoidArray::Count at nsVoidArray.h:63 #1 0x16bba663 in nsDocShell::FirePageHideNotification at nsDocShell.cpp:1024 #2 0x16bb5da0 in nsDocShell::Destroy at nsDocShell.cpp:3683 #3 0x19251973 in nsWebBrowser::SetDocShell at nsWebBrowser.cpp:1618 #4 0x1924b1c8 in nsWebBrowser::InternalDestroy at nsWebBrowser.cpp:138 #5 0x1924aabd in nsWebBrowser::Destroy at nsWebBrowser.cpp:1229 #6 0x0003d9d4 in -[CHBrowserView destroyWebBrowser] at CHBrowserView.mm:321 #7 0x00003af8 in -[BrowserWrapper browserClosed] at BrowserWrapper.mm:298
nsWebBrowser.cpp:1589 on which branch? 1.9.0? If so, why are you holding a ref to aDocShell (which already has a ref in the caller, presumably) and not mDocShell? mDocShell is what can go away from under you during teardown.
Created attachment 426378 [details] [diff] [review] Hold on to the DocShell The crashes are gone with this patch. I got a few leaks in initial testing, but it seems to be OK now.
Comment on attachment 426378 [details] [diff] [review] Hold on to the DocShell r=bzbarsky
Boris, since hendy's patch was against 1.9.0, I just wanted to double-check that you were also OK with the patch for mozilla-central (and ultimately the other Hg branches) and that it doesn't require sr. Camino doesn't build on m-c yet, but the patch applies there (just an offset), and we do have experimental Camino builds for 1.9.1 and 1.9.2, where the crash does still exist and the patch does prevent it--so it seems we should land the patch on m-c and then request approvals for the various branches.
I think this patch is fine for all branches and doesn't need sr.
http://hg.mozilla.org/mozilla-central/rev/1b7c9f8d88d2 Moving to Core to (I hope) the right component to help get attention when it comes time to ask for branch reviews.
Also showing up on crash-stats as [@ @0x0 | nsDocShell::Destroy() ]
For which browser?
Camino 2.0.1 (and 2.0.2, but those were just me using comment 0 to test some l10n updates in our Breakpad client before release; that is, though, how I found this crash had yet-another-signature. Although apparently I already knew that in comment 2; there just weren't very many with that signature at that time). Since you asked, though, I looked and there are about 40 [@ @0x0 | nsDocShell::Destroy() ] crashes across all Firefox versions in the last 4 weeks, but they all have one or more different stacks below nsDocShell::Destroy than the Camino crashes do: https://crash-stats.mozilla.com/report/list?product=Firefox&build_id=&query_search=signature&query_type=contains&query=0x0%20|%20nsDocShell%3A%3ADestroy()&date=&range_value=4&range_unit=weeks&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=%400x0%20|%20nsDocShell%3A%3ADestroy()&missing_sig=&page=1
> but they all have one or more different stacks below nsDocShell::Destroy > than the Camino crashes do Right. I was just asking because Firefox doesn't use the code patched in this bug... so if you were just looking at Firefox crash stats, they were about something else. ;)
Boris, thanks for all the additional clarifications, both here and in bug 547613 :) One more question: does this code get used in XULRunner apps or Fennec, or is Camino the only active app that exercises this path? (If it's only Camino, I may as well start the branch approval process now rather than wait for baking that's not going to detect anything, since we don't yet build against m-c.)
> does this code get used in XULRunner apps or Fennec Not at the moment. It's only used in native-code embeddings of various sorts.
Comment on attachment 426378 [details] [diff] [review] Hold on to the DocShell Requesting branch approvals for this patch, which fixes a Camino 2.0.x (1.9.0) topcrasher that persists in newer branches. The code involved here is not used by Firefox or other XUL apps.
Comment on attachment 426378 [details] [diff] [review] Hold on to the DocShell a=beltzner for all branches
Landed on all branches: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/18ea3b9ac710 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/41454a888b0d Checking in embedding/browser/webBrowser/nsWebBrowser.cpp; /cvsroot/mozilla/embedding/browser/webBrowser/nsWebBrowser.cpp,v <-- nsWebBrowser.cpp new revision: 1.170; previous revision: 1.169 done Note to anyone trying to verify this: this can only be verified in a Camino build, not Firefox, and there are no official Camino builds yet on 1.9.x x>0. You can verify with Camino on 1.9.0.x by absence of a crash using the steps in comment 0, though.
Bleh, put the keyword in the whiteboard by accident :-( smokey 0, beltznermail 1.
Verified for 1.9.0 using the 3/10/2010 nightly Camino 2 build using the steps in comment 0. No crashing is occurring.
verified FIXED on builds using steps in comment 0 : Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:188.8.131.52) Gecko/20100316 Firefox/3.6.2 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:184.108.40.206) Gecko/20100315 Firefox/3.5.9
Changing this to resolved fixed since it wasn't verified on trunk and adding verified1.9.2 and verified1.9.1 keywords.