739146 - AlertFinished called too often

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

This is a minor glitch I found while working on bug 737646.

In case ShowAlertNotification (or ShowNewAlertNotification) succeeds it will take care about the AlertFinished callback.
If not we have to make sure AlertFinished() gets called explicitely.

When rv = alertsService->ShowAlertNotification failed AlertFinished is called in any case although the notification is still in progress and will proceed with ShowNewAlertNotification. In addition when ShowAlertNotification failed we always called AlertFinished explicitely while ShowNewAlertNotification could have succeeded and would call it again.

All this should only have very minimal impact introducing very small time slots for another alert to begin before the previous has been finished but it is superfluous.

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch

Comment 2

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 609234 [details] [diff] [review]
patch

oops, sorry, this version is broken

Comment 3

[Wolfgang Rosenauer [:wolfiR]]

Attachment: proposal

A bit of optimized status tracking.

I still think there is one cycle too much but that's the price for keeping ShowNewAlertNotification self-contained.

Comment 4

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 609277 [details] [diff] [review]
proposal

Review of attachment 609277 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/src/nsMessengerUnixIntegration.cpp
@@ +394,5 @@
>                                                  aAlertText,
>                                                  false,
>                                                  NS_ConvertASCIItoUTF16(aFolderURI),
>                                                  this,
>                                                  EmptyString());

I'm a bit nervous that there may be situations where the alert service never calls our Observe() with "alertfinished", which would leave us stuck with mAlertInProgress = true. I read through that code quickly, and it looks to me like we depend on the notification daemon for that.

I'm OK with this for now, but we should keep an eye on whether we need to implement some sort of way to decide whether to give up on waiting for an alertfinished notification.

@@ +446,3 @@
>      nsCOMPtr<nsIMutableArray> argsArray = do_CreateInstance(NS_ARRAY_CONTRACTID);
>      if (!argsArray)
>        return NS_ERROR_FAILURE;

This failure return will leave us stuck with mAlertInProgress == true

@@ +448,5 @@
>        return NS_ERROR_FAILURE;
>  
>      // pass in the array of folders with unread messages
>      nsCOMPtr<nsISupportsInterfacePointer> ifptr = do_CreateInstance(NS_SUPPORTS_INTERFACE_POINTER_CONTRACTID, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);

This failure return will leave us stuck with mAlertInProgress == true

I recommend moving the mAlertInProgress = true assignment down after we've done all the preparation for the actual alert call (but before the call, unlike in the pre-patch code)

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch #3

You are right with the early errors leaving the flag in wrong state.
Moved the assignment down and made a few changes.

This one
-    ShowNewAlertNotification(false);
+    rv = ShowNewAlertNotification(false);

is the real relevant change only now as previously we called AlertFinished() a second time when ShowAlertNotification failed but ShowNewAlertNotification probably was still successfully running.

Comment 6

[Wolfgang Rosenauer [:wolfiR]]

During testing of that change together with the toolkit change I pushed today I found that it works stable for me (having no notification-daemon) now.

Please note that I also noticed that not every mail account is notifying about changes for me what I do not understand at the moment. For one of my accounts it stopped playing the event sound and showing notifications at some point but this issue seems to happen much earlier in the chain as ShowAlertNotification is never called at all in these cases. (And notification sound also not playing.)
I just mention this for completeness. I'll try to dig deeper into it and file another bug once I get a bit more information.

Comment 7

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 609491 [details] [diff] [review]
patch #3

Review of attachment 609491 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 8

[Wolfgang Rosenauer [:wolfiR]]

http://hg.mozilla.org/comm-central/rev/793feead7dcb