Closed Bug 516124 Opened 12 years ago Closed 12 years ago

Crash [@ nsAlertsIconListener::SendClosed()]


(Core :: Widget: Gtk, defect, P2)




Tracking Status
status1.9.2 --- beta4-fixed
status1.9.1 --- unaffected


(Reporter: florian, Assigned: ventnor.bugzilla)


(Depends on 1 open bug)


(Whiteboard: topcrash)


(1 file, 1 obsolete file)

Attached 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
Closed: 12 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:

Backed out, and reopening.

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 look like a duplicate of this one?
(In reply to comment #8)

Yes, 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.
Attached 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]
Closed: 12 years ago12 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?
Uhm, sorry fo that.
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.