Closed
Bug 541743
Opened 15 years ago
Closed 15 years ago
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() ]
Categories
(Core Graveyard :: Embedding: APIs, defect)
Tracking
(status1.9.2 .2-fixed, status1.9.1 .9-fixed)
RESOLVED
FIXED
People
(Reporter: armen52, Assigned: chris)
Details
(5 keywords)
Crash Data
Attachments
(1 file)
1.13 KB,
patch
|
bzbarsky
:
review+
beltzner
:
approval1.9.2.2+
beltzner
:
approval1.9.1.9+
beltzner
:
approval1.9.0.19+
|
Details | Diff | Splinter Review |
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?
Severity: major → critical
Keywords: crash,
crashreportid
Summary: Closing two tabs from yelp.com in rapid succession crashes Camino → Closing two tabs from yelp.com in rapid succession crashes Camino [@ nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&) | nsDocShell::FirePageHideNotification(int) ] [@ nsDocShell::FirePageHideNotification(int) ]
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•15 years ago
|
||
> Boris, do you have any insight here?
Not offhand, no. I assume this is only reproducible in Camino, not Firefox?
Comment 4•15 years ago
|
||
Does changing nsWebBrowser::SetDocShell to hold a strong ref to the docshell on the stack fix the crash for you?
Assignee | ||
Comment 5•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 7•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 9•15 years ago
|
||
The crashes are gone with this patch. I got a few leaks in initial testing, but it seems to be OK now.
Assignee | ||
Updated•15 years ago
|
Attachment #426378 -
Flags: review?(bzbarsky)
Comment 10•15 years ago
|
||
Comment on attachment 426378 [details] [diff] [review]
Hold on to the DocShell
r=bzbarsky
Attachment #426378 -
Flags: review?(bzbarsky) → review+
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.
Assignee: nobody → trendyhendy2000
Comment 12•15 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 15 years ago
Component: General → Embedding: APIs
Product: Camino → Core
QA Contact: general → apis
Resolution: --- → FIXED
Also showing up on crash-stats as [@ @0x0 | nsDocShell::Destroy() ]
Comment 15•15 years ago
|
||
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
Comment 17•15 years ago
|
||
> 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.)
Comment 19•15 years ago
|
||
> 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.
Attachment #426378 -
Flags: approval1.9.2.2?
Attachment #426378 -
Flags: approval1.9.1.9?
Attachment #426378 -
Flags: approval1.9.0.19?
Summary: Closing two tabs from yelp.com in rapid succession crashes Camino [@ nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&) | nsDocShell::FirePageHideNotification(int) ] [@ nsDocShell::FirePageHideNotification(int) ] → 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() ]
Comment 21•15 years ago
|
||
Comment on attachment 426378 [details] [diff] [review]
Hold on to the DocShell
a=beltzner for all branches
Attachment #426378 -
Flags: approval1.9.2.2?
Attachment #426378 -
Flags: approval1.9.2.2+
Attachment #426378 -
Flags: approval1.9.1.9?
Attachment #426378 -
Flags: approval1.9.1.9+
Attachment #426378 -
Flags: approval1.9.0.19?
Attachment #426378 -
Flags: approval1.9.0.19+
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.
Keywords: fixed1.9.0.19
Whiteboard: fixed1.9.0.19
Comment 24•15 years ago
|
||
Verified for 1.9.0 using the 3/10/2010 nightly Camino 2 build using the steps in comment 0. No crashing is occurring.
Keywords: fixed1.9.0.19 → verified1.9.0.19
Comment 25•15 years ago
|
||
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
Status: RESOLVED → VERIFIED
Comment 26•15 years ago
|
||
Changing this to resolved fixed since it wasn't verified on trunk and adding verified1.9.2 and verified1.9.1 keywords.
Status: VERIFIED → RESOLVED
Closed: 15 years ago → 15 years ago
Keywords: verified1.9.1,
verified1.9.2
Updated•14 years ago
|
Crash Signature: [@ nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&) | nsDocShell::FirePageHideNotification(int) ]
[@ nsDocShell::FirePageHideNotification(int) ]
[@ @0x0 | nsDocShell::Destroy() ]
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•