AlertFinished called too often

RESOLVED FIXED in Thunderbird 14.0


MailNews Core
6 years ago
6 years ago


(Reporter: wolfiR, Assigned: wolfiR)


Thunderbird 14.0

Firefox Tracking Flags

(Not tracked)



(1 attachment, 2 obsolete attachments)



6 years ago
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

6 years ago
Created attachment 609234 [details] [diff] [review]
Assignee: nobody → mozilla
Attachment #609234 - Flags: review?


6 years ago
Attachment #609234 - Flags: review? → review?(irving)

Comment 2

6 years ago
Comment on attachment 609234 [details] [diff] [review]

oops, sorry, this version is broken
Attachment #609234 - Flags: review?(irving)

Comment 3

6 years ago
Created attachment 609277 [details] [diff] [review]

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.
Attachment #609234 - Attachment is obsolete: true


6 years ago
Attachment #609277 - Flags: review?(irving)
Comment on attachment 609277 [details] [diff] [review]

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)
Attachment #609277 - Flags: review?(irving) → review-

Comment 5

6 years ago
Created attachment 609491 [details] [diff] [review]
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.
Attachment #609277 - Attachment is obsolete: true


6 years ago
Attachment #609491 - Flags: review?(irving)

Comment 6

6 years ago
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 on attachment 609491 [details] [diff] [review]
patch #3

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

Looks good.
Attachment #609491 - Flags: review?(irving) → review+

Comment 8

6 years ago
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
You need to log in before you can comment on or make changes to this bug.