167233 - Trunk M120A Mozilla crashed after failure to connect to "Out-of-Service" IMAP server [@ nsWindowWatcher::GetActiveWindow]

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Greg K.]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.2a) Gecko/20020906
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.2a) Gecko/20020906

For some reason, the iTools IMAP server at mail.mac.com is out of service at the
moment. After failing to connect to my account there, Mozilla alerted me that
the IMAP server is out of service with two consecutive alerts, then crashed.

Reproducible: Always

Steps to Reproduce:
1. Configure an IMAP account for a server that is out of service
2. Attempt to connect to that server.

Actual Results:  
Mozilla displayed two "out of service" alerts, then crashed.

Expected Results:  
Mozilla should not have crashed. I would say it should only have shown one
alert, but I'm not certain why two were shown. That's probably a separate issue.

Stack forthcoming.

Comment 1

[Greg K.]

Attachment: Crash report generated by FizzillaCFM/2002090508 on 10.1.5 showing crash after nsWindowWatcher::GetActiveWindow

Comment 2

[Greg K.]

Experienced this crash again using FizzillaCFM/2002090608 on 10.1.5.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is a topcrash that started showing up in 2002-09-06 04:00 builds.  Based on
a checkin window of
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2002-09-05+08%3A00&maxdate=2002-09-06+04%3A30&cvsroot=%2Fcvsroot
I strongly suspect bryner's checkin for bug 141295.  Note that at least some of
the stacks in talkback are different, and many of the steps to reproduce seem
different.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: sumarry of talkback information for nsWindowWatcher::GetActiveWindow

Comment 5

[Brian Ryner (not reading)]

The only way I see to crash in GetActiveWindow is if mActiveWindow is an invalid
pointer.  I'll investigate.  Also, changing Platform/OS to All/All per talkback
info.

Comment 6

[Brian Ryner (not reading)]

These are actually two distinct crashes, although they might have the same
cause.  I don't think we've gotten any talkback reports with the stack in this
bug.  We have several that appear to be a crash on showing a Navigator window
(from Startup in navigator.js).  I haven't been able to reproduce either one. 
Probably a timing issue that I haven't been able to hit yet.

I think we could bulletproof this by using an nsWeakPtr for mActiveWindow.  I'll
come up with a patch for that if I continue to not be able to reproduce this crash.

Comment 7

[Brian Ryner (not reading)]

Attachment: bulletproof against dangling pointer

This is about the best I can do without a good way to reproduce the problem.

Comment 8

[Dan M]

Comment on attachment 98568 [details] [diff] [review]
bulletproof against dangling pointer

This shouldn't be necessary; the system is (supposed to be) carefully
calibrated to not need this kind of second-tier protection. It's spackle on top
of the real problem, which is something else completely, and we're not even
sure it'll help. But I understand that, unable to reproduce the problem, it's a
shot.

Please add a big-ol' comment in the vicinity of the weakref declaration that
goes something like that.

I'd also like this to be a temporary patch: add some assertions to try to catch
the dangling pointer in action. And be prepared to remove the patch if it turns
out not to help. It assumes that the nsIDOMWindow we're handed supports
weakrefs. Ours do of course and it's unlikely we'll ever be handed one that
doesn't, but it is a bothersome assumption.

However I think your patch needs some null checks. mActiveWindow can be null,
so mActiveWindow.get() is problematic. Even then, CallQueryReferent (you know
this will be only the third place in the code where it's used, outside some
test suites!) doesn't like null parameters. I think you need to go with

if (mActiveWindow && mActiveWindow.get())
  CallQueryReferent.

(do_QueryReferent and do_GetWeakReference I believe are safe with nulls.)

r=me after addressing those issues.

Comment 9

[Asa Dotzler [:asa]]

not gonna hold 1.2a for this.

Comment 10

[greer]

Brian, are you still working on Dan's additions? Do you have a patch for sr=?

Comment 11

[Brian Ryner (not reading)]

I was hoping to get more specific info on how to reproduce this, instead of
papering over the problem.

Comment 12

[Brian Ryner (not reading)]

I found a case where we incorrectly tell WindowWatcher that a window is active
_after_ it has been unregistered.  This can happen if we receive a deactivate
after the window has been unregistered -- as part of deactivating the focus
controller, we go tell the window watcher that what used to be the focus
controller's active window is the current active window.  That's just wrong; we
should not be updating the active window from the focus controller when the
focus controller is deactivated.

The steps I used to reproduce this were to run mozilla /turbo from a command
prompt and refocus the command prompt window as turbo was starting up, then
running mozilla again (without /turbo) from a second command prompt.  Obviously
not typical user behavior, but I think there are likely other ways to trigger
the same situation.

Comment 13

[Brian Ryner (not reading)]

Attachment: prevent spurious window watcher notifications

Comment 14

[Jan Carpenter]

Attachment: Attaching stack trace and comments from M120A

This stack is currently the #1 topcrash on M120A and #3 topcrash on the trunk

Comment 15

[Jan Carpenter]

Attachment: Attaching stack and comments from the trunk

Comment 16

[Dan M]

Comment on attachment 100085 [details] [diff] [review]
prevent spurious window watcher notifications

r=danm

Comment 17

[Alec Flett]

Comment on attachment 100085 [details] [diff] [review]
prevent spurious window watcher notifications

sr=alecf

Comment 18

[Brian Ryner (not reading)]

checked in.

Comment 19

[yulian chang]

20020926 trunk builds on Winds and Mac.
 Configure an IMAP account for a server that is 'out of service' by killing the
mail server process. When attempting to connect to that server, an alert appears
describing that 'Could not connect to mail server xxx.mcom.com; the connection
was refused'. No crash.

 Brian also verified the fix.

Marking bug as verified.

Comment 20

[Jay Patel [:jay]]

Adding 0x00000001 to summary for future reference since this crash was being
reported under that stack signature by Talkback.

Comment 21

[Olivier Cahagne]

*** Bug 170693 has been marked as a duplicate of this bug. ***

Comment 22

[Greg K.]

*** Bug 183846 has been marked as a duplicate of this bug. ***

Comment 23

[Greg K.]

See bug 170693, comment 20 for a current crash stack involving
nsWindowWatcher::GetActiveWindow.