Crash [@ nsAlertsIconListener::SendClosed()]

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
P2
critical
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: florian, Assigned: ventnor.bugzilla)

Tracking

(Depends on 1 bug)

Trunk
mozilla1.9.3a1
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta4-fixed, status1.9.1 unaffected)

Details

(Whiteboard: topcrash)

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch Possible fix (obsolete) — Splinter Review
Since bug 469880, when using showAlertNotification with null as the last parameter (the observer), the application crashes when the notification disappears.

There doesn't seem to be any usecase for this in the Firefox code base, but some extensions are likely to use this, so I think this should block 1.9.2.

The attached patch just adds nullchecks...

I'm not really satisfied with the way I modified the test because I found no way to report success of displaying the notification without observer, but at least as I display it before the one with an observer, it will be closed before too, so the crash (if one occurs) would be during the execution of the test, not after it as if I added this notification after the existing one.
Flags: blocking1.9.2?
Attachment #400234 - Flags: superreview?(roc)
Attachment #400234 - Flags: review?(roc)
Assignee: nobody → florian
Attachment #400234 - Flags: superreview?(roc)
Attachment #400234 - Flags: superreview+
Attachment #400234 - Flags: review?(roc)
Attachment #400234 - Flags: review+
Comment on attachment 400234 [details] [diff] [review]
Possible fix

+  if (mAlertListener)
+    mAlertListener->Observe(NULL, "alertclickcallback", mAlertCookie.get());

{} around the if body
http://hg.mozilla.org/mozilla-central/rev/fa03a509c7a7
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #400234 - Flags: approval1.9.2?
The test change makes the linux unit test box timeout: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1253696211.1253699009.9857.gz

Backed out, and reopening.

Backout: http://hg.mozilla.org/mozilla-central/rev/b3f3249ceb96
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Attachment #400234 - Flags: approval1.9.2? → approval1.9.2-
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Do we know if this is a top crash?  If there's no use case for it, do we know if any popular extensions actually use it?  Just wondering why we are blocking on  this?
The "vacuum places improved" extension crash when you close its "done" notification; don't know how popular that extension is.

An example crash: bp-dbed52cc-8aa3-4ee0-862a-bf3fd2091002
topcrash on trunk.  One comment says "tootip from a statusbar icon".
Summary: Crash [@ nsAlertsIconListener::SendClosed] → Crash [@ nsAlertsIconListener::SendClosed()]
Whiteboard: topcrash
Duplicate of this bug: 496678
Does https://bugzilla.redhat.com/show_bug.cgi?id=530236 look like a duplicate of this one?
(In reply to comment #8)

Yes, https://bugzilla.redhat.com/attachment.cgi?id=366186 looks like this, but there is something else in the equation because Firefox/3.5.3 doesn't have this code (unless there are local patches, a binary extension, or something?).
Reporter says he run FoxyProxy extension and it caused the crash.
Can we simply take the nullchecks, and never mind the test if it won't cooperate?
Duplicate of this bug: 528930
Florian, this is a 1.9.2 blocker, and we're going to freeze 1.9.2 on Wednesday night, Mountain View time. Will you be able to fix this before that time? If not, we'll have to find someone else to work on this bug.
I don't really see a reason this would cause a true timeout in the test. Maybe a crash masquerading as a timeout? Has anyone debugged it? I am not enthusiastic about checking this in without its testcase if the testcase is showing that there are still bugs latent in this code.

I am also seriously wondering whether we should block on this bug. Seems like 3.6.1 fodder to me?
Just ran this on a local VM - no timeout.
I think the more stable we can make the original 3.6, the better the perception. I'll put this on the tryserver.
Posted patch Patch 2Splinter Review
I figured out the problem. The alerts are being "queued", with only one being shown at a time. Two alerts are due to show and the second one is the one with the observer, but it takes too long to receive the callback and just times out.

The fix is to move the no observer test to its own file and make sure it runs after the synchronous test.
Attachment #400234 - Attachment is obsolete: true
Attachment #412822 - Flags: review?(roc)
Michael, thanks for figuring out the timeout issue!

I'll have very little time with my Linux machine available before the 1.9.2 freeze, so it would be great if you could take assignment of this bug.
Assignee: florian → nobody
Whiteboard: topcrash → topcrash [needs landing]
http://hg.mozilla.org/mozilla-central/rev/c40b30f6a611
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 529664
Assignee: nobody → ventnor.bugzilla
Duplicate of this bug: 530921
Depends on: 530921
Attachment #412822 - Flags: approval1.9.2?
Attachment #412822 - Flags: approval1.9.2?
Asking for 1.9.1 inclusion...
blocking1.9.1: --- → ?
Attachment #412822 - Flags: approval1.9.1.10?
blocking1.9.1: ? → ---
Depends on: 589471
Depends on: 609772
No longer depends on: 589471
Depends on: 609784
You need to log in before you can comment on or make changes to this bug.