793432 - New nsIAlertsService implementation breaks feature detection

Status: RESOLVED INVALID

(Toolkit :: UI Widgets, defect)

Description

[Nils Maier [:nmaier]]

Either with the new Notification Center code or the Growl removal, it is now possible to .getService() the nsIAlertsService implementation successfully, but have showNotification() calls later fail as not implemented.

The previous behavior was that the getService() call would already fail. IIRC the nsIAlertsService interface was not even registered at all.

The late error only upon calling showNotification is incompatible with some add-ons I remember reviewing (and writing), and at least some legacy AddonSDK versions as well (newer AddonSDK versions aren't affected).

Furthermore this prevents feature detection, as before you could getService() and if that failed provide the user with different options.

I therefore would like to see the previous behavior of failing early on getService restored for maximum backwards compatibility, or alternatively another way to feature detect the actual availability of the service.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

Can you reproduce on 17? If that's the case, then bug 728106 would be the only possible culprit (if the milestones are correct). Tracking on 18 only for now.

Comment 2

[Josh Aas]

Nils - are you still having this problem with the latest Nightly?

The patch for bug 728106, which added notification center support, had a bug that would likely lead to the incorrect behavior you reported. Three weeks or so later I fixed that bug as part of the patch to remove Growl support.

Comment 3

[Nils Maier [:nmaier]]

Attachment: mochitest fix

Latest Nightly 18.0a1 (2012-09-25) fails on Lion. Cannot really test Aurora, because I have Growl installed and am not eager to mess up that installation in order to test.

Here is a patch that actually fixes the existing test so that it fails in the correct places, instead of todo()ing. 
With this applied, make TEST_PATH=toolkit/components/alerts/test mochitest-plain rightfully get an unexpected fail.

Comment 4

[Josh Aas]

Thanks for the patch to the tests, that was helpful.

It seems to me that we have always allowed service creation to succeed, even when showing an alert is not possible (e.g. Growl not installed). The nsMacAlertsService::Init() method, which determines whether or not we can actually show alerts, runs after service creation, and always has.

I did discover something that changed the behavior of your modified test: the commit for bug 776134, "UI Test App - Notifications don't work properly when run OOP":

http://hg.mozilla.org/mozilla-central/rev/164655cefe0b

This changed the return value from nsAlertsService::ShowAlertNotification. Prior to that commit we'd try to send a XUL notification if we failed to use a system service. The XUL notification would fail, but with NS_ERROR_FAILURE. That commit added the following code before trying to use XUL notifications, after system notification has failed:

#ifdef XP_MACOSX
  return NS_ERROR_NOT_IMPLEMENTED;
#endif

So we start returning NS_ERROR_NOT_IMPLEMENTED instead of NS_ERROR_FAILURE.

Does this explain what you're seeing? I think returning NS_ERROR_NOT_IMPLEMENTED is actually a better error.

Comment 5

[Nils Maier [:nmaier]]

No I feel stupid :p

I did some historic digging and the real culprit that changed the behavior actually is:
https://hg.mozilla.org/releases/mozilla-release/rev/ee9ab83779a9
This one removed the growl installation check in ::Init() so that getService would always succeed.

So while the behavior changed as I described, that change was introduced more than 10 months back :(
I only noticed just now because of the growl support removal, having had growl installed before so my own unit tests would succeed...
 
Still considering it a regression and just fixing it would be pointless and potentially harmful, right? It would regress code that was created in between.
However, the idea to provide a way to feature-detect the availability of a working service implementation as an enhancement still seems reasonable.

It seems reasonable if Jorge included notes about this in one of his blog post about addon compat.

Comment 6

[Josh Aas]

Thanks for the info. No need to feel stupid, I'm happy that we have people on the lookout for behavioral changes like this, particularly when they are willing and able to dig into some code.

We are planning to add XUL-based notification support for all platforms in the coming months, which will reduce the need for feature-detection.

Comment 7

[Jorge Villalobos [:jorgev] (he/him)]

When did this ship?

FWIW, the example code in the documentation works either way: https://developer.mozilla.org/en-US/docs/Code_snippets/Alerts_and_Notifications

Comment 8

[Nils Maier [:nmaier]]

(In reply to Jorge Villalobos [:jorgev] from comment #7)
> When did this ship?

Firefox 11 (was not backported to 10ESR, it seems)

> FWIW, the example code in the documentation works either way:
> https://developer.mozilla.org/en-US/docs/Code_snippets/
> Alerts_and_Notifications

Yeah, if people actually use that snippet. OTOH we well them not to .getService all over the place, but do it only one and cache that result...
Also it refers to Growl explicitly.

Randomly checking addons mxr shows people will basically use it in all kinds of ways: no error checking at all (broken), checking only getService (broken now), checking only showAlertNotification (broken before, incl. ESR), null checking (broken), checking both (works).
Seeing this mess, it might be worth considering adding an amo-validator warning (or info, at least) about error checking (try-catching) both .getService() and .showAlertNotification(). What do you think about that, Jorge?
nsIAlertsService is one of the few services that do not have a platform bit in the name (like nsIWinTaskbar) but are not necessarily available on all platforms and hence kinda violate expectations.

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Nils Maier [:nmaier] from comment #8)
> (In reply to Jorge Villalobos [:jorgev] from comment #7)
> Seeing this mess, it might be worth considering adding an amo-validator
> warning (or info, at least) about error checking (try-catching) both
> .getService() and .showAlertNotification(). What do you think about that,
> Jorge?

Adding Kris and Matt, who know the validator better than I do. We would want to warn about using nsIAlertsService without a try/catch block. Note that the try/catch should at least wrap the function calls to the service, not necessarily the referencing of the service.

Can we do this?

Comment 10

[Matt Basta [:basta]]

What do you mean by "use"?

Comment 11

[Kris Maglione [:kmag]]

It can be done. I don't think it can be done especially easily, but I'll leave that determination up to Matt.

I'm assuming we want to flag calls to getService(Ci.nsIAlertsService) and nsIAlertsService@showAlertNotification any time either happens outside of a try-catch block? I think that makes sense, as the alerts service on OS-X is a never ending source of headaches.

Comment 12

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Kris Maglione [:kmag] from comment #11)
> I'm assuming we want to flag calls to getService(Ci.nsIAlertsService) and
> nsIAlertsService@showAlertNotification any time either happens outside of a
> try-catch block? I think that makes sense, as the alerts service on OS-X is
> a never ending source of headaches.

We would want to flag only when the function calls happen outside of a try/catch block. As far as I know, the getService call should never fail, after Firefox 11.

Comment 13

[Matt Basta [:basta]]

Since the validator examines code lexically, the downside to that test is that the following code would generate spurious warnings:

function foo() { getService(Ci.nsIAlertsService }
try {foo()} catch(e){}

Comment 14

[Kris Maglione [:kmag]]

I wouldn't worry too much about that. That kind of code is pretty rare, and if it's meant to deal with the getService throwing where the alerts service is not available, the devs can always catch that error early and throw a more sensible JS error.