433644 - Crash [@ -[ChildView sendFocusEvent:]() ] when closing pop-up window in the background

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[u278247]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9pre) Gecko/2008051300 Camino/2.0a1pre (like Firefox/3.0pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9pre) Gecko/2008051300 Camino/2.0a1pre (like Firefox/3.0pre)

http://www.bt-chat.com/ has annoying popups that don't get caught by the popup blocker and open in the background (i.e. they're not focused and remaing behind the Camino window I used to open http://www.bt-chat.com/ in). Camino crashes every time I close such a popup by clicking on the x button of the popup window.

Reproducible: Always

Steps to Reproduce:
1. Navigate to http://www.bt-chat.com/.
2. Upon loading the page, the popup blocker informs me there was an attempt at opening a popup and asks me whether I want to allow it. The crash is there regardless of leaving the band with the information/question or dismissing it by clicking its x button.
3. Click on the search text entry in the vertical center of the page. It's the one after the buttons Torrent, Usenet, ED2K and before the link "Advanced Search".
4. A popup should appear in the background i.e. behind the Camino window.
5. Click the x button of the popup from step 4 without first focusing it.
Actual Results:  
Camino dies and Apple's Crash Reporter dialog appears offering me to send a report to Apple.

Expected Results:  
The popup from step 4 should be closed and the window in which http://www.bt-chat.com/ was initially opened in step 1 should continue to function.

I was able to reproduce it every time I tried it i.e. 5 times and the following excerpt from the Crash Reporter logs seems to be common:

Process:         Camino [15593]
Path:            /Users/Lubomir/Applications/Camino.app/Contents/MacOS/Camino
Identifier:      org.mozilla.camino
Version:         2.0a1pre (2008.05.13)
Code Type:       X86 (Native)
Parent Process:  launchd [99]

Date/Time:       2008-05-14 05:17:34.665 +0300
OS Version:      Mac OS X 10.5.2 (9C7010)
Report Version:  6

Exception Type:  EXC_CRASH (SIGABRT)
Exception Codes: 0x0000000000000000, 0x0000000000000000
Crashed Thread:  0

Thread 0 Crashed:
0   libSystem.B.dylib             	0x946200ea __kill + 10
1   libSystem.B.dylib             	0x946973f2 raise + 26
2   libSystem.B.dylib             	0x946a69af abort + 73
3   libstdc++.6.dylib             	0x92646005 0x925fe000 + 294917
4   libstdc++.6.dylib             	0x9264410c __gxx_personality_v0 + 1108
5   libstdc++.6.dylib             	0x9264414b std::terminate() + 29
6   libstdc++.6.dylib             	0x926446da std::type_info::~type_info() + 0
7   org.mozilla.camino            	0x006bb720 -[ChildView sendFocusEvent:] + 112
8   org.mozilla.camino            	0x006a9010 -[NSWindow(MethodSwizzling) nsCocoaWindow_NSWindow_sendEvent:] + 464
9   com.apple.AppKit              	0x9303e714 -[NSApplication sendEvent:] + 2780
10  com.apple.AppKit              	0x92f9c0f9 -[NSApplication run] + 847
11  com.apple.AppKit              	0x92f6930a NSApplicationMain + 574
12  org.mozilla.camino            	0x0000358c main + 196
13  org.mozilla.camino            	0x00002e5e _start + 216
14  org.mozilla.camino            	0x00002d85 start + 41

Console.app has matching entries for "pure virtual method called".

Comment 1

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

This sounds a lot like bug 432005 (see bug 432005 comment 6).

Lubomir, can you paste (or attach, if they are more than a few lines) the matching entries from Console.log you mentioned?

Comment 2

[u278247]

Attachment: Compressed CrashReporter .crash log

Console.app has a set of 3 lines for each crash (15585 bellow is the PID so it changes in the different triplets):

[0x0-0x10d10d].org.mozilla.camino[15585] pure virtual method called
[0x0-0x10d10d].org.mozilla.camino[15585] terminate called without an active exception
com.apple.launchd[99] ([0x0-0x10d10d].org.mozilla.camino[15585]) Exited abnormally: Abort trap

Comment 3

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

I can repro this; it just took me a number of tries to get the pop-unders to actually pop under.  

Slightly different stack in my few-days-old debug build.  These frames

7   libwidget_mac.dylib           	0x114fbda5 nsCOMPtr<nsIWidget>::nsCOMPtr(nsIWidget*) + 45 (nsCOMPtr.h:628)
8   libwidget_mac.dylib           	0x114fbde2 nsCOMPtr<nsIWidget>::nsCOMPtr(nsIWidget*) + 24 (nsCOMPtr.h:629)
9   libwidget_mac.dylib           	0x11524c8a nsGUIEvent::nsGUIEvent(int, unsigned int, nsIWidget*) + 60 (nsGUIEvent.h:456)
10  libwidget_mac.dylib           	0x11524cbc nsGUIEvent::nsGUIEvent(int, unsigned int, nsIWidget*) + 38 (nsGUIEvent.h:458)

show up between the libstdc++.6.dylib frames and the -[ChildView sendFocusEvent:] frame.

Comment 4

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

Pretty much all of Camino's trunk Talkback is this bug and bug 433432.  

Kicking to Widget:Cocoa and pulling the blocking1.9? flag to pick up one of the wanted1.9-type flags (unless someone sees this in Firefox it likely doesn't need to block 1.9 proper, but we'll need it fixed before a Camino release off of 1.9).

Comment 5

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #3)

> I can repro this; it just took me a number of tries to get the
> pop-unders to actually pop under.

Smokey, please tell me more about what you did to make the pop-unders
happen.

The only "popups" I see aren't windows at all (but some sort of HTML
object).  I haven't (yet) unblocked conventional popups.

I tested in an opt build with debug symbols that I made yesterday
(with yesterday's code).

Comment 6

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

There's a full-screen window that opens behind the current window, and a small (postage-stamp-sized) window that opens in the upper left behind the full-screen one.  They get around the pop-up blocker (which does block some pop-ups/pop-unders).

As I mentioned earlier, it took me a couple of tries + some waiting to trigger it; perhaps the code that launches the pop-unders isn't ready to run initially (the page loads a lot of crap and takes about 5 minutes until I see no network activity) and waiting to focus the text field is more effective?

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Thanks, Smokey.

I can now reproduce this crash once in three or four tries.  I'm slowly tracking down its cause.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

> once in three or four tries

I get the little popunder window once in every three or four tries.  But I crash every time if I click on it without first focusing it.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

> But I crash every time if I click on it without first focusing it.

I crash every time I click on its close button without first focusing it.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

I've figured out what's causing this bug, and I'll post a patch
(actually two patches) in subsequent comments.  But first I want to
explain what's going on.

It's actually an old bug (or bugs).  It's just an accident that
they've suddenly become visible.

The basic problem is that an nsChildView object can be destroyed
without Destroy() ever having been called on it.  Destroy() calls
[mView widgetDestroyed] on the nsChildView's ChildView (native)
object, and widgetDestroyed is what NULLs out the ChildView object's
weak reference to its nsChildView object (mGeckoChild).  There
(obviously) will be problems if the ChildView object continues to use
its mGeckoChild after that object has been destroyed.  There are NULL
checks on mGeckoChild throughout the ChildView class.  But these won't
work if mGeckoChild doesn't get NULLed before the object it refers to
is destroyed.

Here's what happens specifically:

The crash occurs at the following line in nsChildView.mm:

http://mxr.mozilla.org/mozilla/source/widget/src/cocoa/nsChildView.mm#2540

The nsGUIEvent constructor gets called on an nsIWidget object
(mGeckoChild) that has just been destroyed, which (somehow) results in
a (spurious) error about a pure virtual method having been called on
that object.

The call to [ChildView sendFocusEvent:] that results in this crash is
made at the following line (in nsCocoaWindow.mm):

http://mxr.mozilla.org/mozilla/source/widget/src/cocoa/nsCocoaWindow.mm#2190

There are two possible approaches to this problem, both of which
should (I think) be taken:

1) Bullet-proof the nsChildView destructor, so that it does what's
   needed even if Destroy() wasn't called.

   The need for this was already forseen, and there's already code in
   the nsChildView destructor that tries to do this.  But it doesn't
   do everything that's needed.

2) Find all the cases where an nsChildView object (or any nsIWidget
   object) can be destroyed without calling Destroy(), and fix them.

   This bug's specific case happens at the following line in
   nsWebBrowser.cpp, where nsWebBrowser::InternalDestroy() fails to
   call mInternalWidget->Destroy() at the same time it calls
   mInternalWidget->SetClientData(0).

http://mxr.mozilla.org/mozilla/source/embedding/browser/webBrowser/nsWebBrowser.cpp#136

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix (patch for nsChildView destructor)

Here's a patch for the nsChildView destructor.

I've added a debug warning for when the destructor is called without
Destroy() having been called.  I think this will help tracking down
other cases of this problem.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Attachment: Another fix (patch for nsWebBrowser::InternalDestroy())

Here's a patch for nsWebBrowser::InternalDestroy().

Josh or Stuart (or someone else), please suggest someone to review it.

By the way, each of these two patches fixes this bug's crashes by
itself.  But I still think both should be landed.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

As best I can tell this bug doesn't happen in Firefox.  But 1.9 branch
Camino would be seriously effected, and at least my patch for the
nsChildView destructor (attachment 321100 [details] [diff] [review]) is trivial, and therefore
no-risk.

The patch for nsWebBrowser::InternalDestroy() is slightly less
trivial, and needs testing on multiple platforms.

Comment 14

[Samuel Sidler (old account; do not CC)]

Comment on attachment 321101 [details] [diff] [review]
Another fix (patch for nsWebBrowser::InternalDestroy())

Boris is an embedding peer, so targeting him for review.

Comment 15

[Mike Schroepfer]

Anyone know the schedule for a camino off 1.9 release?  If it is even a month or so out that can pickup this fix in a 1.9.0.1...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 321101 [details] [diff] [review]
Another fix (patch for nsWebBrowser::InternalDestroy())

This looks ok, assuming we don't need to null out mInternalWidget (and specifically, assuming Destroy() on widgets is idempotent).

You probably want similar fixes in nsIView::CreateWidget and nsWebShellWindow::~nsWebShellWindow

Comment 17

[Samuel Sidler (old account; do not CC)]

(In reply to comment #15)
> Anyone know the schedule for a camino off 1.9 release?  If it is even a month
> or so out that can pickup this fix in a 1.9.0.1...

Camino 2 is definitely more than a month away, but an alpha might be before 1.9.0.1. I think this can wait.

Comment 18

[Stuart Morgan]

(In reply to comment #17)
> Camino 2 is definitely more than a month away, but an alpha might be before
> 1.9.0.1.

Agreed on both counts; I'd say there's a good chance 2.0a1 will be less than a month from now.

> I think this can wait.

From a ship schedule, perhaps, but my concern is less about that, and more about getting it landed to find out if there are other bugs it's masking (or, on the flip side, if this is responsible for some of the non-crash problems like bug 412223).

Comment 19

[Stuart Morgan]

I realized after posting that I wasn't very clear. If the tree will be opening for post-1.9 landings fairly quickly, so that we could get this in nightlies for testing, then I'm fine with taking this immediately after the tree re-opens for 1.9.0.x rather than now.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #16)

> (and specifically, assuming Destroy() on widgets is idempotent)

It definitely is in Cocoa widgets (and in nsBaseWidget), and pretty
clearly also is in GTK2 widgets.  And it probably is in all the others
... but it's a bit hard to tell just looking at the source.

> assuming we don't need to null out mInternalWidget

Yeah, I think we should.  Now I notice that the nsWebBrowser class has
all kinds of NULL tests on mInternalWidget.  I'll change this in my
next revision.

> You probably want similar fixes in nsIView::CreateWidget and
> nsWebShellWindow::~nsWebShellWindow

That makes sense to me.  I'll add these in my next revision.

This patch has gotten complicated enough that I think it should be
spun off in another bug.  I don't want to delay fixing this particular
bug ... which can be accomplished by the (trivial) patch for
nsChildView's destructor.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

>> (and specifically, assuming Destroy() on widgets is idempotent)
>
> It definitely is in Cocoa widgets (and in nsBaseWidget), and pretty
> clearly also is in GTK2 widgets.  And it probably is in all the
> others ... but it's a bit hard to tell just looking at the source.

By the way, nsIWidget::Destroy() may (as far as I know) still end up
being called more than once on a single widget (e.g in the nsView
destructor and in nsWebBrowser::InternalDestroy()) even if I change my
InternalDestroy() patch to NULL out mInternalWidget.

So (if I'm right) we may not want to proceed with the
InternalDestroy() patch (attachment 321101 [details] [diff] [review]) until we're sure
nsIWidget::Destroy() is idempotent on all kinds of widgets.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 321101 [details] [diff] [review]
Another fix (patch for nsWebBrowser::InternalDestroy())

I've spun the issues targeted by this patch off into a new bug (bug
434089 "Make sure Destroy() is called (at least once) before an
nsIWidget is destroyed").

Comment 23

[Mike Schroepfer]

(In reply to comment #19)
> I realized after posting that I wasn't very clear. If the tree will be opening
> for post-1.9 landings fairly quickly, so that we could get this in nightlies
> for testing, then I'm fine with taking this immediately after the tree re-opens
> for 1.9.0.x rather than now.
> 

Smokey this is totally clear.  We hope to keep the tree closed as shortly as possible as there are a number of fixes waiting for 3.0.1 for which it is better to get them into nighties sooner rather than later.  

Given the discussion here I'm going to take this off the 1.9 nom list and add it to the 1.9.0.x wanted list.

Comment 24

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

Since tryserver can't do Camino, here's a Camino build with the ChildView patch for Lubomir to try, since he's hitting this often.

http://www.ardisson.org/moz/CaminoTrunk-2008-05-17-bug433644.dmg

Comment 25

[u278247]

(In reply to comment #24)
> Since tryserver can't do Camino, here's a Camino build with the ChildView patch
> for Lubomir to try, since he's hitting this often.

Thank you all for the dedicated work! It was a great experience following the progress through the comments.

> http://www.ardisson.org/moz/CaminoTrunk-2008-05-17-bug433644.dmg

I get 404 at the specified URL.

Comment 26

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

> I get 404 at the specified URL.

Oops, apparently "copy path" doesn't work correctly in that app :(

http://www.ardisson.org/smokey/moz/CaminoTrunk-2008-05-17-bug433644.dmg

Comment 27

[u278247]

(In reply to comment #26)
> http://www.ardisson.org/smokey/moz/CaminoTrunk-2008-05-17-bug433644.dmg

The build works for me i.e. I cannot reproduce the crash with it. Thank you very much!

Comment 28

[Josh Aas]

Comment on attachment 321100 [details] [diff] [review]
Fix (patch for nsChildView destructor)

r+, but please add a comment about why you're not just calling ::Destroy in the destructor if it wasn't already called.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch for nsChildView destructor, rev1 (revise comment)

Comment 30

[Mike Shaver (:shaver emeritus)]

Patch not ready, work it for 3.0.1.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Roc, can you sr?

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 322703 [details] [diff] [review]
Patch for nsChildView destructor, rev1 (revise comment)

+  if (!mOnDestroyCalled)
+    NS_WARNING("nsChildView object destroyed without callling Destroy()");

Use NS_WARN_IF_FALSE

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Is it alright to land this now on mozilla-central (before it lands on
the 1.9.0 branch)?  That'd give it some time to bake there.

Comment 36

[Samuel Sidler (old account; do not CC)]

If it has reviews, you can land it on mozilla-central at any point, if the tree is green (then resolve this bug as FIXED). Just be sure to request approval1.9.0.1 on the patch after it has baked.

Comment 37

[Steven Michaud [:smichaud] (Retired)]

Landed on mozilla-central, with roc's change from comment #33.

Comment 38

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

This bug probably blocks Camino 2.0a1, but note especially the last paragraph of Stuart's comment 18: we'd really like to get this on 1.9.0 for a few weeks *in advance* of our alpha to make sure it's not masking, or responsible for, other problems.  Shipping a crashy milestone sucks.

Comment 39

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 322703 [details] [diff] [review]
Patch for nsChildView destructor, rev1 (revise comment)

Minimal risk, large benefit.

Comment 40

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

Comment on attachment 322703 [details] [diff] [review]
Patch for nsChildView destructor, rev1 (revise comment)

a=beltzner; can you please add a description of how QA can test this, or perhaps a description of a litmus test?

Comment 41

[Steven Michaud [:smichaud] (Retired)]

> can you please add a description of how QA can test this, or perhaps
> a description of a litmus test?

This bug's STR is quite difficult to reproduce (and then only in
Camino).  I suspect the only way we can be sure	it's been fixed is if
Talkback reports of this crash drop to zero :-)

But here's what I've been doing (I tested it again just now).  It
"works" one time in 4 or 5 (in recent Camino 1.9.0-branch builds).

1) Start a recent 1.9.0-branch Camino nightly and visit
   http://www.bt-chat.com/.

2) Wait until everything in the page has loaded (until the status bar
   is empty).

   On the first few tries this may take more than a minute.  And often
   an ad will "pop up" in some kind of HTML object, obscuring the rest
   of the page.	 If this happens before the status bar is empty, quit
   Camino and restart.

3) Click in the box (near middle of the page) to the left of "Advanced
   Search".

   If you get a	small pop-under in the	upper-left of the screen,
   proceed to step 4.  Otherwise quit Camino and restart.

4) Click on the pop-under's close button without giving it focus
   (i.e. without first clicking anywhere else in that window).

   If you've gotten this far, you should crash 100% of the time. 

Comment 42

[Steven Michaud [:smichaud] (Retired)]

Landed on the 1.9.0 branch (in cvs).

Comment 43

[Samuel Sidler (old account; do not CC)]

Steven, please be sure to use the fixed1.9.0.1 keyword when landing things on the 1.9 branch, pre-1.9.0.1. (Afterwards, there will of course be fixed1.9.0.x keywords.)

Comment 44

[Steven Michaud [:smichaud] (Retired)]

Thanks, Sam.  I'll eventually learn :-)

Comment 45

[Marcia Knous [:marcia]]

This bug is a bit difficult to verify, even using the STR in Comment 41. As I click around in the screen I am now getting the "Attack Site" warning for www.advertyz.com. But I haven't yet been able to crash using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1pre) Gecko/2008070104 GranParadiso/3.0.1pre.

Comment 46

[Samuel Sidler (old account; do not CC)]

Please note that comment 41 states this is only reproducible with Camino.

Comment 47

[Steven Michaud [:smichaud] (Retired)]

And this problem is fixed in today's Camino nightlies (since my patch was landed on the 1.9.0 branch yesterday).

Comment 48

[Tony Chung [:tchung]]

(In reply to comment #46)
> Please note that comment 41 states this is only reproducible with Camino.
> 
We need a testcase for 3.0.1.  can you provide one, since the testcase in comment 41 doesnt apply?

Comment 49

[Samuel Sidler (old account; do not CC)]

(In reply to comment #48)
> We need a testcase for 3.0.1.  can you provide one, since the testcase in
> comment 41 doesnt apply?

It applies to Camino. You can verify this for 1.9.0.1 using a Camino 1.9 nightly. There is no testcase for Firefox.

Comment 50

[Marcia Knous [:marcia]]

Using Version 2.0a1pre (1.9.0.2pre 2008071400) of Camino, I was not able to crash using the STR in Comment 41 - I tried several times. Adding verified keyword.