541743 - Closing two tabs on yelp.com in rapid succession crashes Camino [@ nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&) | nsDocShell::FirePageHideNotification(int) ] [@ nsDocShell::FirePageHideNotification(int) ] [@ @0x0 | nsDocShell::Destroy() ]

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Armen Abrahamian]

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.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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?

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Boris, do you have any insight here?

Not offhand, no.  I assume this is only reproducible in Camino, not Firefox?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Does changing nsWebBrowser::SetDocShell to hold a strong ref to the docshell on the stack fix the crash for you?

Comment 5

[Christopher Henderson]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Christopher Henderson]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Christopher Henderson]

Attachment: 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 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 426378 [details] [diff] [review]
Hold on to the DocShell

r=bzbarsky

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I think this patch is fine for all branches and doesn't need sr.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Also showing up on crash-stats as  [@ @0x0 | nsDocShell::Destroy() ]

Comment 15

[Boris Zbarsky [:bzbarsky]]

For which browser?

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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 21

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

Comment on attachment 426378 [details] [diff] [review]
Hold on to the DocShell

a=beltzner for all branches

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 23

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Bleh, put the keyword in the whiteboard by accident :-(  smokey 0, beltznermail 1.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0 using the 3/10/2010 nightly Camino 2 build using the steps in comment 0. No crashing is occurring.

Comment 25

[Aakash Desai [:aakashd]]

verified FIXED on builds using steps in comment 0 :

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.9) Gecko/20100315 Firefox/3.5.9

Comment 26

[Al Billings [:abillings - ex-MoCo]]

Changing this to resolved fixed since it wasn't verified on trunk and adding verified1.9.2 and verified1.9.1 keywords.