Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 739146 - AlertFinished called too often
: AlertFinished called too often
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All Linux
: -- minor (vote)
: Thunderbird 14.0
Assigned To: Wolfgang Rosenauer [:wolfiR]
Depends on:
  Show dependency treegraph
Reported: 2012-03-26 00:19 PDT by Wolfgang Rosenauer [:wolfiR]
Modified: 2012-04-02 10:54 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (2.11 KB, patch)
2012-03-26 00:20 PDT, Wolfgang Rosenauer [:wolfiR]
no flags Details | Diff | Splinter Review
proposal (4.00 KB, patch)
2012-03-26 03:46 PDT, Wolfgang Rosenauer [:wolfiR]
irving: review-
Details | Diff | Splinter Review
patch #3 (3.45 KB, patch)
2012-03-26 14:28 PDT, Wolfgang Rosenauer [:wolfiR]
irving: review+
Details | Diff | Splinter Review

Description Wolfgang Rosenauer [:wolfiR] 2012-03-26 00:19:03 PDT
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] 2012-03-26 00:20:11 PDT
Created attachment 609234 [details] [diff] [review]
Comment 2 Wolfgang Rosenauer [:wolfiR] 2012-03-26 00:21:47 PDT
Comment on attachment 609234 [details] [diff] [review]

oops, sorry, this version is broken
Comment 3 Wolfgang Rosenauer [:wolfiR] 2012-03-26 03:46:24 PDT
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.
Comment 4 :Irving Reid (No longer working on Firefox) 2012-03-26 07:50:31 PDT
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)
Comment 5 Wolfgang Rosenauer [:wolfiR] 2012-03-26 14:28:42 PDT
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.
Comment 6 Wolfgang Rosenauer [:wolfiR] 2012-03-26 14:33:00 PDT
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) 2012-04-02 07:29:01 PDT
Comment on attachment 609491 [details] [diff] [review]
patch #3

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

Looks good.
Comment 8 Wolfgang Rosenauer [:wolfiR] 2012-04-02 10:54:22 PDT

Note You need to log in before you can comment on or make changes to this bug.