Closed Bug 1233086 Opened 8 years ago Closed 8 years ago

http icons do not work on linux native notifications

Categories

(Toolkit Graveyard :: Notifications and Alerts, defect)

46 Branch
defect
Not set
normal

Tracking

(firefox47 wontfix, firefox48 wontfix, firefox49 verified, firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- verified
firefox50 --- fixed

People

(Reporter: bkelly, Assigned: lina)

References

()

Details

(Keywords: regression)

Crash Data

Attachments

(4 files)

Got a bug report over twitter today that apparently only triggers on linux:

  https://twitter.com/gauntface/status/677160573260472320

Steps to reproduce:

1) Open https://tests.peter.sh/notification-generator/
2) Set a custom icon to http://l.yimg.com/a/i/brand/purplelogo//uh/us/news-wea.gif
3) Click display notification

Expected:

The yahoo weather icon is shown in the notification.

Actual:

On mac I get the expected icon, but the user reports it is not shown on ubuntu with firefox nightly 46.
jgraham tried to reproduce this for me, but he reports the icon shows correctly.  We may need the user's help to understand whats different about his configuration.
Recorded what I'm getting: https://drive.google.com/file/d/0B55F0MBQPca4cFZYWkxIYXV2azA/view?usp=sharing

I don't have any add-ons enabled.

I'm on Gnome Ubuntu 15.10.
Thanks Matt!

Can you paste the output from about:support here?

Also, do you see anything in the web-console or browser-console?  (I don't know the shortcut keys on mac, but you can open these from hamburger->developer->browser console, etc.)
Flags: needinfo?(matt)
I'll take a look. There are a few known UX paper cuts with notifications on Linux. :-/ (Another known issue: notifications with an image don't show up the first time). Thanks for the report!

I can reproduce this on Fedora 23, which also uses the libnotify alerts backend. Unity on Ubuntu uses the XUL backend, so it's unaffected. Same for OS X.
Component: DOM: Push Notifications → Notifications and Alerts
Product: Core → Toolkit
Any reason we aren't just using XUL notifications everywhere?
And I think Kit identified the issue, so we don't need the support info I requested above.
Flags: needinfo?(matt)
(In reply to Ben Kelly [:bkelly] from comment #5)
> Any reason we aren't just using XUL notifications everywhere?

That's a good question! We went back and forth on this a few times, and I think custom action buttons will spur another round of discussion. TL;DR: a lot of it came down to system integration.

Since system notifications are used by other apps (at least on OS X and Gnome; we're thinking about adding Windows 10, too), users are accustomed to interacting with them. There are global settings for "do not disturb" and alert styles, which we'd duplicate. These platforms also have a notification tray for unread alerts, and automatically handle stacking, sorting, collapsing, multiple monitors, idling, and timestamps. We improved XUL notifications in 45, but a lot of these UX questions remain. System notifications give them to us for "free."

That said, there are cases where we can't use the system backend. In particular, Unity's notifications are ephemeral: you can't trigger an action by clicking on them, they fade out automatically, and there are no buttons at all. In that case, we fall back to XUL, even though native notifications are available.
Summary: http icons do not work on linux notifications → http icons do not work on linux native notifications
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Depends on: 1227300
Attachment #8699300 - Attachment is obsolete: true
Comment on attachment 8699300 [details]
Bug 1233086, Part 1 - Unify alert image loading.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28231/diff/1-2/
Attachment #8699300 - Attachment description: MozReview Request: Bug 1233086 - Refactor alert image loading. → MozReview Request: Bug 1233086, Part 1 - Unify alert image loading. r?seth
Attachment #8699300 - Attachment is obsolete: false
Attachment #8699300 - Flags: review?(seth)
Comment on attachment 8700177 [details]
Bug 1233086, Part 3 - Use unified image loading for OS X alerts.

https://reviewboard.mozilla.org/r/28607/#review25513
Attachment #8700177 - Flags: review?(mstange) → review+
Blocks: 1220640
Based on the window in https://bugzilla.mozilla.org/show_bug.cgi?id=1220640#c2
bug 1082837 is the suspected trigger.
Keywords: regression
Looks like changing the policy type to `nsIContentPolicy::TYPE_INTERNAL_IMAGE` is the salient part. I've been wanting to consolidate the image loading code, so I did that as part of the ticket. Happy to split it out into a separate patch, though.
Comment on attachment 8699300 [details]
Bug 1233086, Part 1 - Unify alert image loading.

This is going to need a few tweaks based on bug 1227300. Clearing r? for now.
Attachment #8699300 - Flags: review?(seth)
Attachment #8699300 - Flags: review?(seth)
Comment on attachment 8699300 [details]
Bug 1233086, Part 1 - Unify alert image loading.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28231/diff/2-3/
Comment on attachment 8700176 [details]
Bug 1233086, Part 2 - Use unified image loading for libnotify alerts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28605/diff/1-2/
Comment on attachment 8700177 [details]
Bug 1233086, Part 3 - Use unified image loading for OS X alerts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28607/diff/1-2/
Attachment #8700177 - Attachment description: MozReview Request: Bug 1233086, Part 3 - Use unified image loading for OS X alerts. r?mstange → MozReview Request: Bug 1233086, Part 3 - Use unified image loading for OS X alerts. r=mstange
Blocks: 1082837
https://reviewboard.mozilla.org/r/28231/#review31815

Unifying this code is good idea thanks.  ISTR the Mac code being noticeably different and I've only compared this against the Gnome code.
I don't know whether Markus compared the Mac code being removed with this.

::: toolkit/components/alerts/AlertNotification.cpp:19
(Diff revision 3)
> -NS_IMPL_CYCLE_COLLECTION(AlertNotification, mPrincipal)
> +NS_IMPL_CYCLE_COLLECTION(AlertNotification, mPrincipal, mImageRequest,
> +                         mImageTimer, mImageListener, mImageContext)

The nsIAlertNotificationImageListener implementations are not cycle collection
participants and so do not need to be traversed.

Similarly with nsIPrincipal:

[11:12] <karlt> why does nsGlobalWindow traverse its principal but nsNodeInfoManager not?
[11:12] <karlt> is there a simple way to know whether any nsIPrincipal (for example) implementations are cycle collection classes?
[11:13] <khuey> no
[11:13] <khuey> there is not
[11:14] <karlt> khuey: thanks; how does one decide whether to traverse nsISupports objects then?; just do it if you are sure?
[11:14] <karlt> not sure
[11:14] <khuey> karlt: in general some parts of Gecko are cycle collected and some aren't
[11:14] <khuey> principals are part of the infrastructure that falls in the latter
[11:15] <khuey> so you wouldn't normally bother traversing it
[11:15] <mccr8> I think the window ones aren't really needed but somebody decided them to throw it at the CC just in case
[11:15] <khuey> yeah
[11:15] <karlt> khuey: does that mean you know that principals don't hold references to cycle collection participants?
[11:15] <mccr8> the CC will QI to nsICycleCollectionPArticipant and it will return null and then the CC will ignore it
[11:15] <khuey> karlt: in general I know that things that are farther away from the DOM don't hold references to cycle collection participants
[11:16] <khuey> and nsIPrincipal is pretty far from the DOM
[11:16] <karlt> ok, thanks

Similarly, I assume the other references are held only as long as the request
and decode process, and so there is no need to traverse those references as
they will be removed anyway.

I don't think AlertNotification needs to be a cycle collection participant.

::: toolkit/components/alerts/AlertNotification.cpp:41
(Diff revision 3)
>  AlertNotification::~AlertNotification()
> -{}
> +{
> +  CancelImageRequest(true);

loadImageXPCOM() says "ImageLib does NOT keep a strong ref to the observer",
but if that is true, then what keeps the AlertNotification alive during image decoding?

::: toolkit/components/alerts/AlertNotification.cpp:168
(Diff revision 3)
> +  if (mImageRequest || mImageTimer) {

Is it necessary to test mImageTimer here?
I expect there won't be a timer if there is no request.

::: toolkit/components/alerts/AlertNotification.cpp:282
(Diff revision 3)
> +  if (mImageTimer) {
> +    mImageTimer->Cancel();
> +  }

Can mImageTimer be released here?

::: toolkit/components/alerts/AlertNotification.cpp:303
(Diff revision 3)
> +  if (NS_WARN_IF(aRequest != mImageRequest)) {
> +    return NS_OK;
> +  }

Is this possible?
If so, please add a comment to explain.
If not, please make this an assertion.

::: toolkit/components/alerts/AlertNotification.cpp:321
(Diff revision 3)
> +  if (NS_WARN_IF(aTimer != mImageTimer)) {

Is this possible?
If so, please add a comment to explain.
If not, please make this an assertion.

::: toolkit/components/alerts/nsIAlertsService.idl:8
(Diff revision 3)
> +#include "imgIRequest.idl"

Is "interface imgIRequest;" sufficient here?
Comment on attachment 8700176 [details]
Bug 1233086, Part 2 - Use unified image loading for libnotify alerts.

https://reviewboard.mozilla.org/r/28605/#review31827

If you are happy with no timeout, then I think this part is good.
It would be helpful to understand what keeps the AlertNotification alive.

::: toolkit/system/gnome/nsAlertsIconListener.h:62
(Diff revision 2)
>    bool mLoadedFrame;

I assume this can be removed now.

::: toolkit/system/gnome/nsAlertsIconListener.cpp:283
(Diff revision 2)
> -  nsAutoString imageUrl;
> -  rv = aAlert->GetImageURL(imageUrl);
> +  // Wait six seconds for the image to load.
> +  return aAlert->GetImage(6000, this, nullptr);

Six seconds is not long for a network request.

Are network timeouts so long that we need an additional timer here?
If not I'd prefer to pass zero for no timeout here.

There may be a an issue with decode errors not being detected after load, but
I'd prefer to address them properly rather than catching them with a timeout.
See the LOAD_COMPLETE/DECODE_COMPLETE comment at
https://bugzilla.mozilla.org/show_bug.cgi?id=858919#c28
Attachment #8700176 - Flags: review?(karlt)
Comment on attachment 8700176 [details]
Bug 1233086, Part 2 - Use unified image loading for libnotify alerts.

https://reviewboard.mozilla.org/r/28605/#review31831
Attachment #8700176 - Flags: review+
(In reply to Karl Tomlinson (ni?:karlt) from comment #20)
> loadImageXPCOM() says "ImageLib does NOT keep a strong ref to the observer",
> but if that is true, then what keeps the AlertNotification alive during
> image decoding?

(Not having looked at the patch yet...) ImageLib keeps a reference to *an* observer for the image, but not the one it returns to you. It also could release that reference at any time, because what holds it is the image cache, and the image cache may drop references for reasons of both time and space.
Comment on attachment 8699300 [details]
Bug 1233086, Part 1 - Unify alert image loading.

https://reviewboard.mozilla.org/r/28231/#review33269

The overall design looks fine but there are lots of little issues; see below.

::: toolkit/components/alerts/AlertNotification.cpp:38
(Diff revision 3)
> +  , mImageContext(nullptr)

Do not initialize nsCOMPtr's with nullptr. Their default constructor does the job just fine.

::: toolkit/components/alerts/AlertNotification.cpp:43
(Diff revision 3)
> +  CancelImageRequest(true);

The reader doesn't know what "true" means here, and it's pretty important to understand. Please do something like this:

CancelImageRequest(/* aForgetObserver = */ true);

Similar for any other occurences of CancelImageRequest().

Also, why are you nulling out mImageTimer here? Its destructor is just about to be called.

::: toolkit/components/alerts/AlertNotification.cpp:161
(Diff revision 3)
> +AlertNotification::GetImage(uint32_t aTimeout,

Why is this called "GetImage()"? That sounds like a getter, which this is definitely not as far as I can tell. Rename.

::: toolkit/components/alerts/AlertNotification.cpp:224
(Diff revision 3)
> +      ((imgStatus & imgIRequest::STATUS_ERROR) && !mLoadedFrame)) {

Why are you checking mLoadedFrame here? If STATUS_ERROR is set, we could not even decode the header of the image. This check is not needed, as far as I can tell.

::: toolkit/components/alerts/AlertNotification.cpp:239
(Diff revision 3)
> +  image->RequestDecodeForSize(nsIntSize(width, height), imgIContainer::FLAG_NONE);

s/nsIntSize/IntSize

::: toolkit/components/alerts/AlertNotification.cpp:248
(Diff revision 3)
> +    return NS_OK; // only use one frame

You may as well just do this stuff in OnLoadComplete(), which is guaranteed to be called only once. You won't get this notification until after OnLoadComplete() runs, anyway, because until that point you haven't requested a decode, and this is a decode-related notification. I don't think you really need this method at all, nor do you need the mLoadedFrame variable.

::: toolkit/components/alerts/AlertNotification.cpp:288
(Diff revision 3)
> +      mImageRequest->CancelAndForgetObserver(NS_BINDING_ABORTED);

You know where it says "please don't use this method in any new code" in imgIRequest.idl? Please don't use this method, which is to say, please don't cancel the image request from your destructor. Instead ensure that the lifetime of AlertNotification is such that it doesn't get destroyed until you receive the imgINotificationObserver::LOAD_COMPLETE notification. If you actually can't do this, let me know, and we can reconsider, but CancelAndForgetObserver is a notorious source of crash bugs and I'd really prefer that you avoid using it.

::: toolkit/components/alerts/nsIAlertsService.idl:22
(Diff revision 3)
> +   * supported by any backend.

Get rid of the text about animated images; what you're doing does not prevent animated images from loading. All you're really preventing are subsequent frames of multipart/x-mixed-replace images. Just say "since at this point we've already loaded enough data" or something like that.

::: toolkit/components/alerts/nsIAlertsService.idl:27
(Diff revision 3)
> +  void onImageReady(in nsISupports context, in imgIRequest request);

Gecko style requires arguments be named |aFoo|. Please fix this everywhere.

::: toolkit/components/alerts/nsIAlertsService.idl:33
(Diff revision 3)
> +   * @param context The context passed to |getImage|.

What's the point of this? Just to keep something alive while you're waiting for the request to load?
Attachment #8699300 - Flags: review?(seth)
Blocks: 1237035
https://reviewboard.mozilla.org/r/28231/#review31815

> The nsIAlertNotificationImageListener implementations are not cycle collection
> participants and so do not need to be traversed.
> 
> Similarly with nsIPrincipal:
> 
> [11:12] <karlt> why does nsGlobalWindow traverse its principal but nsNodeInfoManager not?
> [11:12] <karlt> is there a simple way to know whether any nsIPrincipal (for example) implementations are cycle collection classes?
> [11:13] <khuey> no
> [11:13] <khuey> there is not
> [11:14] <karlt> khuey: thanks; how does one decide whether to traverse nsISupports objects then?; just do it if you are sure?
> [11:14] <karlt> not sure
> [11:14] <khuey> karlt: in general some parts of Gecko are cycle collected and some aren't
> [11:14] <khuey> principals are part of the infrastructure that falls in the latter
> [11:15] <khuey> so you wouldn't normally bother traversing it
> [11:15] <mccr8> I think the window ones aren't really needed but somebody decided them to throw it at the CC just in case
> [11:15] <khuey> yeah
> [11:15] <karlt> khuey: does that mean you know that principals don't hold references to cycle collection participants?
> [11:15] <mccr8> the CC will QI to nsICycleCollectionPArticipant and it will return null and then the CC will ignore it
> [11:15] <khuey> karlt: in general I know that things that are farther away from the DOM don't hold references to cycle collection participants
> [11:16] <khuey> and nsIPrincipal is pretty far from the DOM
> [11:16] <karlt> ok, thanks
> 
> Similarly, I assume the other references are held only as long as the request
> and decode process, and so there is no need to traverse those references as
> they will be removed anyway.
> 
> I don't think AlertNotification needs to be a cycle collection participant.

Great. Thank you for the clarification! I wasn't sure if `AlertNotification` had to be a participant, since it's exposed to JS. But, if none of its members need to be cycle collected, I'll just remove it entirely.

> Is this possible?
> If so, please add a comment to explain.
> If not, please make this an assertion.

I think this may have been possible before, if the request was cancelled and restarted. I refactored the code so that this should no longer be an issue, and added assertions for both cases.
https://reviewboard.mozilla.org/r/28231/#review33269

> You may as well just do this stuff in OnLoadComplete(), which is guaranteed to be called only once. You won't get this notification until after OnLoadComplete() runs, anyway, because until that point you haven't requested a decode, and this is a decode-related notification. I don't think you really need this method at all, nor do you need the mLoadedFrame variable.

Cool. Removed both of those.

> You know where it says "please don't use this method in any new code" in imgIRequest.idl? Please don't use this method, which is to say, please don't cancel the image request from your destructor. Instead ensure that the lifetime of AlertNotification is such that it doesn't get destroyed until you receive the imgINotificationObserver::LOAD_COMPLETE notification. If you actually can't do this, let me know, and we can reconsider, but CancelAndForgetObserver is a notorious source of crash bugs and I'd really prefer that you avoid using it.

I broke out the observer into a separate class to make the code a little easier to follow. It takes an owning reference when the request starts, and removes it on complete. Please take a look; I think I'm getting the hang of lifetimes and ownership, but still new at it. Thank you for the thorough review!

> Get rid of the text about animated images; what you're doing does not prevent animated images from loading. All you're really preventing are subsequent frames of multipart/x-mixed-replace images. Just say "since at this point we've already loaded enough data" or something like that.

I confused multipart and animated images. Updated comment.

> Gecko style requires arguments be named |aFoo|. Please fix this everywhere.

Fixed in a follow-up commit, along with unnecessary `nsCOMPtr` initializations. Did not update the test because I'm told we no longer use aArgs in new toolkit JS code.

> What's the point of this? Just to keep something alive while you're waiting for the request to load?

Yes. It's not used for libnotify, but the OS X backend uses it to pass the pending `OSXNotificationInfo` object along. Clarified in comment.
Comment on attachment 8699300 [details]
Bug 1233086, Part 1 - Unify alert image loading.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28231/diff/3-4/
Attachment #8699300 - Flags: review?(seth)
Comment on attachment 8700176 [details]
Bug 1233086, Part 2 - Use unified image loading for libnotify alerts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28605/diff/2-3/
Attachment #8700176 - Attachment description: MozReview Request: Bug 1233086, Part 2 - Use unified image loading for libnotify alerts. r?karlt → MozReview Request: Bug 1233086, Part 2 - Use unified image loading for libnotify alerts. r=karlt
Comment on attachment 8700177 [details]
Bug 1233086, Part 3 - Use unified image loading for OS X alerts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28607/diff/2-3/
https://reviewboard.mozilla.org/r/28605/#review31827

> Six seconds is not long for a network request.
> 
> Are network timeouts so long that we need an additional timer here?
> If not I'd prefer to pass zero for no timeout here.
> 
> There may be a an issue with decode errors not being detected after load, but
> I'd prefer to address them properly rather than catching them with a timeout.
> See the LOAD_COMPLETE/DECODE_COMPLETE comment at
> https://bugzilla.mozilla.org/show_bug.cgi?id=858919#c28

That's fair. I arbitrarily picked six seconds because that's what OS X uses. I think it might be helpful to have an app-level timeout, so that we're not waiting indefinitely on slow servers...but I don't know what a good value would be. Markus, do you remember the rationale for OS X?
https://reviewboard.mozilla.org/r/28231/#review34735

::: toolkit/components/alerts/AlertNotification.cpp:292
(Diff revision 4)
> +  return NotifyMissing();

Is it safe for `NotifyImageMissing()` to release the ref here, even though `mRequest->Cancel()` is async?

I noticed we may not get a `LOAD_COMPLETE` notification if we cancel before the request loads...which makes sense, except I'm not sure how to detect "request cancelled" otherwise. Or is this OK?
Comment on attachment 8699300 [details]
Bug 1233086, Part 1 - Unify alert image loading.

https://reviewboard.mozilla.org/r/28231/#review35535

::: toolkit/components/alerts/AlertNotification.h:75
(Diff revision 4)
> +  WeakPtr<AlertImageRequest> mImageRequest;

I'm *really* sorry to do this to you, but...

I think your concern about Cancel() is absolutely correct. We broke this at some point without noticing. The fix may be trivial - in theory we can just fire the LOAD_COMPLETE notification all the time - but it may not be, because that may turn out to break something somewhere, and I don't want to block your patch on it.

For now you are better off using CancelAndForgetObserver(). I'd remove the WeakPtr stuff and just call CancelAndForgetObserver() in the destructor of AlertImageRequest.

Again, sorry about this.
Attachment #8699300 - Flags: review?(seth)
Comment on attachment 8726859 [details]
Bug 1233086, Part 4 - Gecko style fixes.

https://reviewboard.mozilla.org/r/38283/#review35537

LGTM.
Attachment #8726859 - Flags: review?(seth) → review+
If I remove the call to `mRequest->Cancel()` and just call `mRequest->CancelAndForgetObserver()` in the destructor, I get this warning the first time the image is loaded: https://dxr.mozilla.org/mozilla-central/rev/af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86/image/ProgressTracker.h#88

I guess that's because I'm releasing the ref and triggering the destructor from within `Notify()`? The warning goes away if I call `mRequest->Cancel()` and null out `mRequest`, as in the current patch. So maybe my concern about receiving notifications after calling `Cancel()` is unfounded.

FWIW, the notifications arrive in an odd order. For the initial request, I get {SIZE_AVAILABLE, LOAD_COMPLETE}. For subsequent ones, the order is {SIZE_AVAILABLE, FRAME_UPDATE, FRAME_COMPLETE, DECODE_COMPLETE, LOAD_COMPLETE}. Some time after (presumably when the decoded version is discarded), I see the initial ordering again, followed by a "forced to copy" warning.

I'm also wondering if I need to wait until DECODE_COMPLETE before calling `NotifyComplete()`, since I'm requesting a decode. The image shows up fine the first time, so probably not...and, if it's cached, LOAD_COMPLETE fires after DECODE_COMPLETE, anyway.

Sorry this is so complicated. Would love some help untangling it. :-) Thanks for your patience.
Flags: needinfo?(seth)
(In reply to Kit Cambridge [:kitcambridge] from comment #35)
> I guess that's because I'm releasing the ref and triggering the destructor
> from within `Notify()`? The warning goes away if I call `mRequest->Cancel()`
> and null out `mRequest`, as in the current patch. So maybe my concern about
> receiving notifications after calling `Cancel()` is unfounded.

Yep, that's why. That warning is not really a big deal when there's only one observer.

> FWIW, the notifications arrive in an odd order. For the initial request, I
> get {SIZE_AVAILABLE, LOAD_COMPLETE}. For subsequent ones, the order is
> {SIZE_AVAILABLE, FRAME_UPDATE, FRAME_COMPLETE, DECODE_COMPLETE,
> LOAD_COMPLETE}. Some time after (presumably when the decoded version is
> discarded), I see the initial ordering again, followed by a "forced to copy"
> warning.

There's nothing odd about that notification order; it's exactly what ProgressTracker is intended to do:

https://dxr.mozilla.org/mozilla-central/source/image/ProgressTracker.cpp#304

I'm not precisely sure what you mean by initial request vs. subsequent requests. Are these requests for the same or different images? A second request for the same image will replay all the notifications that were sent for that image immediately, so you'll get the notifications in the order you describe. The first order you specified is also the expected order, except that nothing has triggered a decode for that image yet.

> 
> I'm also wondering if I need to wait until DECODE_COMPLETE before calling
> `NotifyComplete()`, since I'm requesting a decode. The image shows up fine
> the first time, so probably not...and, if it's cached, LOAD_COMPLETE fires
> after DECODE_COMPLETE, anyway.

It'd probably be better to wait until FRAME_COMPLETE (not DECODE_COMPLETE, which would wait for e.g. every frame of an animated GIF to decode even though you only use the first), yes. It will work either way because FLAG_SYNC_DECODE is being passed here:

https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaUtils.mm#498

But if you call NotifyComplete() when only waiting for LOAD_COMPLETE, it's definitely the case that you'll sometimes end up performing some of the decoding on the main thread.

> Sorry this is so complicated. Would love some help untangling it. :-) Thanks
> for your patience.

It's mostly not that bad. Don't worry about the notification order, just trust that it's right. There are an absurd amount of assertions make sure that's the case in ImageLib. You also don't need to worry about that warning in this situation. (Though you've diagnosed the issue correctly.) The only real issue is the awkwardness of managing lifetimes for these objects; we really should just give in and start cycle collecting these things. I do certainly agree, as mentioned above, that calling NotifyComplete() in response to FRAME_COMPLETE would be a better choice.
Flags: needinfo?(seth)
Thanks for the explanation, Seth! I've been busy with other things, but I hope to circle back to this toward the end of next week.
Comment on attachment 8699300 [details]
Bug 1233086, Part 1 - Unify alert image loading.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28231/diff/4-5/
Attachment #8699300 - Attachment description: MozReview Request: Bug 1233086, Part 1 - Unify alert image loading. r?seth → Bug 1233086, Part 1 - Unify alert image loading.
Attachment #8700176 - Attachment description: MozReview Request: Bug 1233086, Part 2 - Use unified image loading for libnotify alerts. r=karlt → Bug 1233086, Part 2 - Use unified image loading for libnotify alerts.
Attachment #8700177 - Attachment description: MozReview Request: Bug 1233086, Part 3 - Use unified image loading for OS X alerts. r=mstange → Bug 1233086, Part 3 - Use unified image loading for OS X alerts.
Attachment #8726859 - Attachment description: MozReview Request: Bug 1233086, Part 4 - Gecko style fixes. r?seth → Bug 1233086, Part 4 - Gecko style fixes.
Attachment #8699300 - Flags: review?(seth)
Comment on attachment 8700176 [details]
Bug 1233086, Part 2 - Use unified image loading for libnotify alerts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28605/diff/3-4/
Comment on attachment 8700177 [details]
Bug 1233086, Part 3 - Use unified image loading for OS X alerts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28607/diff/3-4/
Comment on attachment 8726859 [details]
Bug 1233086, Part 4 - Gecko style fixes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38283/diff/1-2/
OK, I think this should do it. The first time the image is loaded, the code waits for LOAD_COMPLETE, requests a decode, and waits for FRAME_COMPLETE. For subsequent requests, when the decoded version is available, FRAME_COMPLETE fires first, and we ignore LOAD_COMPLETE since we've already decoded the image.

Karl, Markus: could you please take another look at libnotify and OS X? Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cd8c2d91ddb
Attachment #8700176 - Flags: review+ → review?(karlt)
Attachment #8700177 - Flags: review+ → review?(mstange)
Comment on attachment 8700177 [details]
Bug 1233086, Part 3 - Use unified image loading for OS X alerts.

https://reviewboard.mozilla.org/r/28607/#review56586
Attachment #8700177 - Flags: review?(mstange) → review+
Attachment #8699300 - Flags: review?(seth) → review+
Comment on attachment 8699300 [details]
Bug 1233086, Part 1 - Unify alert image loading.

https://reviewboard.mozilla.org/r/28231/#review56832

Looks good. I'm sorry about the complexity of this stuff; it's a legacy design from over a decade ago, and it's confusing even for me. Thanks for sticking with it.
Attachment #8700176 - Flags: review?(karlt) → review+
Comment on attachment 8699300 [details]
Bug 1233086, Part 1 - Unify alert image loading.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28231/diff/5-6/
Comment on attachment 8700176 [details]
Bug 1233086, Part 2 - Use unified image loading for libnotify alerts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28605/diff/4-5/
Comment on attachment 8700177 [details]
Bug 1233086, Part 3 - Use unified image loading for OS X alerts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28607/diff/4-5/
Comment on attachment 8726859 [details]
Bug 1233086, Part 4 - Gecko style fixes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38283/diff/2-3/
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6fc6996103a
Part 1 - Unify alert image loading. r=seth
https://hg.mozilla.org/integration/mozilla-inbound/rev/21f3d3f6baa2
Part 2 - Use unified image loading for libnotify alerts. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/14e487fc2bad
Part 3 - Use unified image loading for OS X alerts. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/a21989d264ee
Part 4 - Gecko style fixes. r=seth
No longer blocks: 1215861
Kit, do you think we could uplift this to Aurora or beta?
Flags: needinfo?(kcambridge)
Aurora, definitely, though I want to give it some time to bake and make sure we didn't regress anything. Probably too risky for beta.
The underlying reason that the icon loads were failing was that nothing was keeping the listener alive, as described in bug 1266993 comment 9.

This fix also fixed the leak in bug 1266856, since system notifications failing to complete and notify the DOM notification code causes the DOM notification code to leak, as also described in the same comment.

Is it worth backporting the smaller patch in bug 1266993 to beta?
I haven't seen any notification-related regressions for the past week (*me tempts fate*), so I think it's safe to request uplift to Aurora. Thanks for all the reviews and guidance!

(In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #56)
> Is it worth backporting the smaller patch in bug 1266993 to beta?

Sounds great to me. Also, if this patch is deemed too risky for Aurora, let's uplift your patch to both.
Flags: needinfo?(kcambridge)
Comment on attachment 8699300 [details]
Bug 1233086, Part 1 - Unify alert image loading.

Approval Request Comment
[Feature/regressing bug #]: libnotify notifications on Linux.
[User impact if declined]: Notifications with an image won't show up the first time (bug 1215861, bug 1279233), and will leak windows (bug 1266993).
[Describe test coverage new/current, TreeHerder]: Includes tests for new image loading code. https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e1223a519bdf
[Risks and why]: Medium risk. Most of this patch is moving code around, but it rewrites image loading for OS X and Linux. If this is too risky, we can uplift the patch for bug 1266993 instead.
[String/UUID change made/needed]: No string changes. Adds a new interface (`nsIAlertNotificationImageListener`), and a method to an existing interface (`nsIAlertNotification::loadImage`).
Attachment #8699300 - Flags: approval-mozilla-aurora?
Attachment #8700176 - Flags: approval-mozilla-aurora?
Attachment #8700177 - Flags: approval-mozilla-aurora?
Comment on attachment 8726859 [details]
Bug 1233086, Part 4 - Gecko style fixes.

For completeness.
Attachment #8726859 - Flags: approval-mozilla-aurora?
Comment on attachment 8699300 [details]
Bug 1233086, Part 1 - Unify alert image loading.

Let's try this on aurora, and if it causes a lot of regressions, crashes, or doesn't work as you expect maybe you could back it out and try the simpler patch from the other bug.
Attachment #8699300 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8700176 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8700177 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8726859 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Cornel, can your team verify the fix on aurora? Sounds like we have STR for linux, maybe not for mac though.
Flags: needinfo?(cornel.ionce)
Hi guys,
I was unable to reproduce this issue on 
- Ubuntu x64 14.04
- Ubuntu x64 16.04 
- Mac OSx 10.11.1
using Nightly 46.0a1 (20151216030229). 

We, unfortunately, don't have an Ubuntu 15.10 OS on any of our computers and couldn't test on that. 
Every notification sent from the site provided in the STR is displayed, even using images from other sites than http://l.yimg.com/a/i/brand/purplelogo//uh/us/news-wea.gif.

Can anyone else repro this?
No worries, Ubuntu 14 and 16 should be enough. Are you testing in GNOME, by chance? Unity, the default Ubuntu window manager, isn't affected.

OS X wasn't affected by this bug, either, but this patch changed how it loads notification images. Could you please verify that the same notifications tested on Linux also show up on OS X, to make sure we didn't regress? Thanks!
Hi! I can test on Fedora 24 on GNOME and MATE, if needed?
Resolved in GNOME 3.20 on Arch.
Version: unspecified → 46 Branch
Hi Kit and sorry for the late response,
I have retested the issue in Gnome on Ubuntu 16.04 and the issue is not reproducible there either and the notifications on OSx show up just as on Ubuntu. I have some screenshots in the following link with the notifications for every OS. Hope this helps.

http://imgur.com/a/2c0O7
Flags: needinfo?(cornel.ionce)
Crash Signature: [@ nsAlertsIconListener::OnLoadComplete]
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.