443775 - Growl 1.1.4 breaks nsIAlertsService initialization and 'before-growl-registration'

Status: RESOLVED WORKSFORME

(Toolkit :: UI Widgets, defect)

Description

[David Humphrey (:humph)]

As per bug 308552#c92, attempting to register a growl notification at "before-growl-registration" doesn't work with Growl latest (1.1.4).  The initialization timings seem to have changed such that you never get notified.

Comment 1

[Shawn Wilsher :sdwilsh]

Might not be growl related:
-1604706400[609ce0]: WARNING: Cannot create startup observer : service,@mozilla.org/alerts-service;1: file /Code/moz/central/mozilla/embedding/components/appstartup/src/nsAppStartupNotifier.cpp, line 113

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sorry, but I'm not able to find a similar output for hg. So giving a cvs blame link for the place where we probably fail:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/embedding/components/appstartup/src/nsAppStartupNotifier.cpp&rev=1.13&mark=93-96&#93

Comment 3

[Shawn Wilsher :sdwilsh]

That code hasn't changed in a very long time.  Don't think that's it.

Comment 4

[Shawn Wilsher :sdwilsh]

Oh - duh.  That makes sense since that machine doesn't have growl installed on it...

Comment 5

[Shawn Wilsher :sdwilsh]

Just to check - are you still seeing your application register with growl (remove it first, then close the preference window)

Comment 6

[David Humphrey (:humph)]

(In reply to comment #5)
> Just to check - are you still seeing your application register with growl
> (remove it first, then close the preference window)

I'm glad you asked this, because after manually removing Thunderbird from the list of Applications Growl has registered, closing the pref window, and starting my patched tb build again, it worked (i.e., I get my growl new mail notification as expected).  After doing this, I go back to the Growl pref window and Thunderbird is there.  So WFM?  Any thoughts on why this would be, Shawn?

Mark, what about you, can you reproduce this, given that you never got it to work before with my patch from bug 308552?

Comment 7

[Phil Ringnalda (:philor)]

Removing Tb from Growl's list doesn't make any difference for me, still no notification at all.

Comment 8

[Shawn Wilsher :sdwilsh]

I guess I'll ping a growl developer about this tomorrow.

Comment 9

[David Humphrey (:humph)]

I couldn't understand how I was seeing this and Phil/Mark weren't, so I did some more testing.  I've been able to confirm that my stuff works in debug builds only.  To test I'm removing the Thunderbird entry from the growl pref dialog before running each.  I have no idea why, as there is nothing ifdef'ed for debug in nsMessengerOSXIntegration.

Comment 10

[Karsten Düsterloh]

Debug builds of mailnews are using shared libraries instead of static ones, see bug 329021.
Does compiling with --enable-static-mail break Growl for your debug build?

Comment 11

[David Humphrey (:humph)]

A debug build with --enable-static-mail breaks growl, yes.

Comment 12

[Karsten Düsterloh]

I don't think it's Growl's fault.
I do get Biff notifications through Growl with my SM debug build, but they are just not registered as "New Mail" notifications, because at the time when nsAlertsService.mm does before-growl-registration, nsMessengerOSXIntegration hasn't even been created/initialized and thus can't respond. 

Comment 13

[Shawn Wilsher :sdwilsh]

well...that's an interesting problem to have

When does nsMessengerOSXIntegration get initialized?

Comment 14

[Karsten Düsterloh]

nsAlertsService::Init is first, called by nsAppStartupNotifier::Observe, adding the nsAlertsService observer.
Shortly after, nsAlertsService::Observe is called, registrating Growl.
Then SM starts up, in my case with messenger.xul as the main window.
messenger.xul's onload handler is called, jumping back into the backend to nsMsgAccountManager::GetAllIdentities and nsMsgAccountManager::LoadAccounts.
And there the messenger OS integration is initialized:
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/src/nsMsgAccountManager.cpp&rev=1.339&mark=1155-1159#1130>
 

Comment 15

[David Humphrey (:humph)]

What remains to be answered here is why in a non-static debug build this works, but in a static debug/opt build it doesn't.  And to clarify (Karsten and I spoke on IRC), what I mean by "work" is not the named notification init, but the Biff new mail notification when mail comes in.

Shawn, as a secondary point, Karsten and I were also discussing whether it's possible at all to register a named notification.  There doesn't seem to be another user of before-growl-registration in the tree, and as was discussed above (see comment #12) mailnews doesn't have a chance to get this.  Karsten suggested that AlertsService might need an Observer to force it to do late registrations.  You were keen to have me do a named notification, and then to localize its name, but it seems this is all for not, given that the fallback is 'general notification' in nsAlertsService.mm.

So two issues: static vs. non-static and named notification registration.

Comment 16

[Shawn Wilsher :sdwilsh]

Sadly, we have to know what notifications we are have when we register with growl.

Is it possible to init that service earlier in order to register?  Anything before "final-ui-startup" is good.

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #15)
> What remains to be answered here is why in a non-static debug build this works,
> but in a static debug/opt build it doesn't.  And to clarify (Karsten and I
> spoke on IRC), what I mean by "work" is not the named notification init, but
> the Biff new mail notification when mail comes in.

I just took a brief look at the build code that provides the module definitions - these are both the same for static and non-static builds, i.e. the Init() function should be called when the module is loaded. This therefore implies that in the non-static case the libmailbase.dylib is loading "at the right time", whereas in the static case, libmail.dylib is being loaded only at final-ui-startup time.

So I think we need to take a look at what is causing libmailbase.dylib (I think I've got the name right) to be loaded, and what is the difference for when libmail.dylib is loaded. I may get time to look at this on Monday if no-one else does.

The other thought I've just had, in the static case, if you supply an argument, e.g. -addressbook does the registration work?

Comment 18

[David Humphrey (:humph)]

> libmail.dylib is loaded. I may get time to look at this on Monday if no-one
> else does.

That's great, Mark, thanks so much.  I know I won't be able to do it by then.

> The other thought I've just had, in the static case, if you supply an argument,
> e.g. -addressbook does the registration work?

I tried this, no difference.

Comment 19

[Karsten Düsterloh]

So I did an optimized build of SM and ran it.
And got a Growl notification for new mail!
I then did remove SM's registration in Growl, tried again and again got a Growl notification for new mail...

When testing, I used an IMAP account (login at startup enabled) which got passed new messages from a different computer. Note that these messages are only marked as new wrt Biff on the very first time they're copied to the IMAP account and must be unread while doing so - iow, for each test, you need an unread message never seen by the IMAP account before!

So I don't think that Biff-over-Growl is broken at all.

The issue of the before-growl-registration init still remains, though.
The order of calls in my comment #14 still holds.

Comment 20

[David Humphrey (:humph)]

(In reply to comment #19)
> So I did an optimized build of SM and ran it.
> And got a Growl notification for new mail!
> I then did remove SM's registration in Growl, tried again and again got a Growl
> notification for new mail...

So forgive my ignorance, but I cannot replicate your findings with Thunderbird.  Is it possible that what you're seeing with SM is different?  Don't get me wrong, I'm glad it's working :)

> So I don't think that Biff-over-Growl is broken at all.

Agreed.

Comment 21

[Karsten Düsterloh]

> So forgive my ignorance, but I cannot replicate your findings with
> Thunderbird. Is it possible that what you're seeing with SM is different?

Possible: yes.
Likely: I don't think so.
But I'll do the same checks with TB tonight, just forgive my slow G4. ;-)

WRT before-growl-registration: I don't think it will "always" work in SM, as long as nsMessengerOSXIntegration isn't part of SM's "main core", because starting up SM generally does not mean starting up MailNews and thus would mean failure in registering the "New Mail" notification. But that shouldn't be a problem for TB, if we can figure out where to put nsMessengerOSXIntegration's init to make it seen on app startup...

Comment 22

[Karsten Düsterloh]

So I did debugging why it does work for my optimized SM and not indeed for an optimized TB and found this:
=> SM is doubly broken and thus works
=> TB is only broken once and hence doesn't work

En detail:
nsMessengerOSCXIntegration::ShowAlertMessage tries to grab the growlNotification string from messenger.properties - which fails for SM, because the patches in 308552 are broken. :-P 
This succeeds for TB.

Since SM doesn't pass an alert name to nsAlertsService::ShowAlertNotification, the service will create and display a general notification.
TB, otoh, tries to show a notification which isn't registered with Growl (=> this bug's topic) and thus fails.

Comment 23

[David Humphrey (:humph)]

After some research today, and discussions with shaver/ted/mfinkle, it looks like the proper solution here is to use a category in order to have all observers initialized from nsAlertsService when before-growl-registration happens, thus fixing our timing issue: 

in nsAlertsService.mm

#include "nsCategoryManagerUtils.h"
...(in ::Observe())
NS_CreateServicesFromCategory("before-growl-registration",
                               static_cast<nsISupports *>(static_cast<void *>(this)),
                               "before-growl-registration");

I tried a quick fix, but fell into linkage hell, since the mac alerts stuff is FORCE_SHARED_LIB instead of LIBXUL_LIBRARY (see bug #378785).  I don't have time to mess with this any more for a few weeks, so if someone else wants to try, that would be great.  nsComponentManagerUtils.h is in the glue, so maybe going at it from that direction is the best idea, and adding a new file to link to here?

I'm going to try and have my vacation now, which hasn't been going too well so far :)

Comment 24

[Shawn Wilsher :sdwilsh]

So, is it my understanding that this is not a bug in the growl alerts code?

Comment 25

[David Humphrey (:humph)]

I'd say it is a bug in (Mac) alerts, yes, since the fix is needed in nsAlertsService.mm.  Once I'm officially back from vacation (start of Sept) I'll continue with this.

Comment 26

[Shawn Wilsher :sdwilsh]

Would it help if we initialized services that registered in the category manager for before-growl-registration?

Comment 27

[Mark Banner (:standard8)]

(In reply to comment #26)
> Would it help if we initialized services that registered in the category
> manager for before-growl-registration?

Finally found some time to look at this, and the answer is yes. I'll attach a patch to bug 308552 and explain it there, but essentially this bug isn't a bug, and we can close it down.