516124 - Crash [@ nsAlertsIconListener::SendClosed()]

Status: RESOLVED FIXED

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

Description

[Florian Quèze [:florian]]

Attachment: Possible fix

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.

Comment 1

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

Comment on attachment 400234 [details] [diff] [review]
Possible fix

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

{} around the if body

Comment 2

[Florian Quèze [:florian]]

http://hg.mozilla.org/mozilla-central/rev/fa03a509c7a7

Comment 3

[Florian Quèze [:florian]]

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

Comment 4

[Damon Sicore (:damons)]

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?

Comment 5

[Matthew Cline]

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

Comment 6

[Karl Tomlinson (:karlt)]

topcrash on trunk.  One comment says "tootip from a statusbar icon".

Comment 8

[Matěj Cepl]

Does https://bugzilla.redhat.com/show_bug.cgi?id=530236 look like a duplicate of this one?

Comment 9

[Karl Tomlinson (:karlt)]

(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?).

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Reporter says he run FoxyProxy extension and it caused the crash.

Comment 11

[Michael Ventnor]

Can we simply take the nullchecks, and never mind the test if it won't cooperate?

Comment 13

[Joe Drew (not getting mail)]

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.

Comment 14

[Joe Drew (not getting mail)]

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?

Comment 15

[Joe Drew (not getting mail)]

Just ran this on a local VM - no timeout.

Comment 16

[Michael Ventnor]

I think the more stable we can make the original 3.6, the better the perception. I'll put this on the tryserver.

Comment 17

[Michael Ventnor]

Attachment: Patch 2

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.

Comment 18

[Florian Quèze [:florian]]

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.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/mozilla-central/rev/c40b30f6a611

Comment 20

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/73e510e78e17

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Asking for 1.9.1 inclusion...

Comment 23

[Karl Tomlinson (:karlt)]

Martin, please see comment 9 and
http://cvs.fedoraproject.org/viewvc/rpms/xulrunner/F-12/mozilla-libnotify.patch?view=markup

Comment 24

[Martin Stránský [:stransky] (ni? me)]

Uhm, sorry fo that.