1004495 - Tapping on a download notification doesn't do anything (if Firefox is in the background)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Download Manager, defect)

Description

[Marco Castelluccio [:marco]]

STR:
1) Download a file
2) Go to the homescreen
2) Tap a download completed notification

I expected the file to be opened or Firefox to be opened with a "about:downloads" tab (so that I could open the file), instead tapping on the notification just dismissed it.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - Is this fixed by the Downloads.jsm backout?

Comment 3

[Marco Castelluccio [:marco]]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Wes - Is this fixed by the Downloads.jsm backout?

I think this wasn't a regression but a pre-existing bug.

Comment 4

[Marco Castelluccio [:marco]]

I wrote the STR in comment 0.

1) Start a download
2) Go to the homescreen by pressing the home button
3) Wait the download to be finished
4) Tap on the notification that Firefox uses to notify a completed download

Firefox should open the downloaded file, instead the notification is dismissed and no action is performed.

Comment 5

[Wesley Johnston (:wesj)]

This was really a QA wanted to see if it was fixed by backout recently.

There is likely a different bug here though. We use Notifications.jsm for these notifications, and if Gecko is killed while the notification is showing, we don't have any callback to call anymore. Its sorta an inherent problem with this approach :(

We could move all this handling to Java and have an explicit listener for the intent there. (i.e. in OnNewIntent we'd have:

if (intent == download_intent) {
  handleDownloadIntent();
}

Alternatively... we could have these intents fire an nsIObserver notification when they come back that can be picked up by the sender. Receivers would just have to register themselves on startup so that they can pick up any new messages coming in...

Comment 6

[Aaron Train [:aaronmt]]

(In reply to Wesley Johnston (:wesj) from comment #5)
> This was really a QA wanted to see if it was fixed by backout recently.

This is still broken across channels. I tested with the Nightly APK on http://nightly.mozilla.org and backgrounded the browser. On tap of the download complete notification, it's dismissed and nothing happens.

Comment 7

[Wesley Johnston (:wesj)]

Strawman proposal here. We need a way for Notification listeners to listen across kills/restarts of Geckos. I'm going to propose something like:

Notifications.registerHandler(key, handler);

as a way to say "Give me all of the notification events from notifications with this key". When you want to show a notification,

// XXX - Do we need to pass the key here. Can we just make it one of aOptions and make this API semi-compat with the old one? Perhaps passing two parameters to create puts you in the v2 api...
var id = Notifications.create(key, aOptions);
Notifications.update(id, aOptions); // update it later
Notifications.cancel(id); // cancel it later

aOptions can/should contain a "cookie" parameter that will be passed to callbacks. handlers will implement an interface like:

{
  onClick(aCookie);
  onCancelled(aCookie);
  onButtonPressed(buttonNumber, aCookie);
}

At startup, we'll gather notifications we've heard from in Notifications.jsm and give extensions/JS that have registered for that key 10-20 seconds to register/respond to them. Only one listener will ever be notified about a particular extension.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Saving my place. This seems to work well. Also, I hate this notifications code.

There's a bit of an dance with these things in Java. In fact, Java tries kill notifications if the Activity dies, but keeps downloads around because they can live beyond the activity (all notifications COULD live beyond the activity, but we decided downloads were special). This allows everything to live even if the APPLICATION is killed until they're explicitly hidden by us or the user (at which point, Gecko is started and notified they're being clicked/cancelled. That's probably annoying if you select "Clear All" to remove something. Need to fix.)

Our Java frontend also throws away clicks/etc if Gecko isn't running or if it isn't aware of any notifications that have been shown. I removed a lot of that in here, but I'm worried it was put in for a reason. No notes in the code or bugs means it can't be important though, right? :)

Also, I'm going to see if I can write tests for this...

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

OK. Lets try this. This revamps our Notifications system a bit so that callers can pass up a "handlerId" and a "cookie" (we had a cookie before but we only held it in JS). When you tap the notification, regardless of if Fennec is running, we start Fennec and pass the handlerId and cookie back to JS, and then notify anyone registered for this handlerId (and pass them the cookie parameter).

Pinging brian for the review because he wrote this mReady stuff that I removed. TBH, I want to remove all the code that auto-clears notifications as well. I think adding it was a mistake. But I'll file a different bug for that.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This revamps downloads.js to use the new code.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8437893 [details] [diff] [review]
Patch 2/2

>diff --git a/mobile/android/chrome/content/downloads.js b/mobile/android/chrome/content/downloads.js
>+    Notifications.registerHandler(this._notificationKey, this);
>+  },

Add a comment before this section of methods to let people know that onClick and friends are part of the Notifications callback API

>+  onClick: function(aCookie) {
>+    Services.console.logStringMessage("Click " + aCookie);

>+  onCancel: function(aCookie) {
>+    Services.console.logStringMessage("Cancel " + aCookie);

>+  onButtonClick: function(aButtonId, aCookie) {
>+    Services.console.logStringMessage("ButtonClick " + aButtonId + ": " + aCookie);

Remove the debugging output before landing

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8437890 [details] [diff] [review]
Patch 1/2

Drive-by

>diff --git a/mobile/android/base/NotificationHelper.java b/mobile/android/base/NotificationHelper.java

>     private void hideNotification(JSONObject message) {

>             id = message.getString("id");
>+            handler  = message.optString("handlerKey");
>+            cookie  = message.optString("cookie");

Extra space after the '='

>diff --git a/mobile/android/modules/Notifications.jsm b/mobile/android/modules/Notifications.jsm

>+    if ("handlerKey" in aOptions && aOptions.handlerKey != null)
>+        this._handlerKey = aOptions.handlerKey;

Fix indent

>+    if (this._cookie)
>+        msg.cookie = JSON.stringify(this._cookie);

Fix indent

>   cancel: function() {
>     let msg = {
>         type: "Notification:Hide",
>-        id: this._id
>+        id: this._id,
>+        handlerKey: this._handlerKey,
>+        cookie: JSON.stringify(this._cookie),

The indent of this section is wrong too

>+  unregisterHandler: function(key, handler) {
>+    var i = _handlersMap[key].indexOf(handler);

var -> let

>+        if (handlerKey) {
>+            _handlersMap[handlerKey].forEach(function(handler) {
>+                handler.onClick(cookie);
>+            });

Fix indent

>+        if (handlerKey) {
>+            _handlersMap[handlerKey].forEach(function(handler) {
>+                handler.onButtonClick(data.buttonId, cookie);
>+            });
>+        }

Fix indent

>+        if (handlerKey) {
>+            _handlersMap[handlerKey].forEach(function(handler) {
>+                handler.onCancel(cookie);
>+            });

Fix indent

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 8437890 [details] [diff] [review]
Patch 1/2

Review of attachment 8437890 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApp.java
@@ +1324,5 @@
>              }
>          });
>  
>          GeckoAppShell.setNotificationClient(makeNotificationClient());
> +        NotificationHelper.getInstance(getApplicationContext());

It's tricky that you need this line here because of the GeckoThreadListener event registration side effect in the initialization. Since the ApplicationContext stays constant between activity recreations with DKA enabled, the NotificationHelper only needs to be initialized once, so it would be better to put this in GeckoApplication's onCreate.

To avoid the side effect trickery, I'd also recommend moving the registerGeckoThreadListener call out of the NotificationHelper constructor and into a separate init() function, like we do at [1]. A couple of advantages to this approach:

1) You can still pass the context to init(), just once, so that you don't need to pass it every time you call getInstance().
2) By having a separate init() function for initialization, it makes the intent clearer by removing side effects (that is, without looking deeper into NotificationHelper right now, it's not clear why you need to have this getInstance() call here at all). getInstance() is strictly used to get the instance; init() is used to do the initialization.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApplication.java#124

::: mobile/android/base/NotificationClient.java
@@ -19,5 @@
>  public abstract class NotificationClient {
>      private static final String LOGTAG = "GeckoNotificationClient";
>  
>      private volatile NotificationHandler mHandler;
> -    private boolean mReady;

This change seems unrelated to the rest of your patch -- but why do you want to remove it? mReady acts as a guard to prevent multiple bind() calls from occurring. Without it, we'll repeatedly create new notification handlers [1] and repeatedly bind the notification service [2].

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AppNotificationClient.java#23
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ServiceNotificationClient.java#33

::: mobile/android/base/NotificationHelper.java
@@ +71,1 @@
>      private static NotificationHelper mInstance;

While here, we should fix the naming (mInstance -> sInstance).

@@ +71,4 @@
>      private static NotificationHelper mInstance;
>  
> +    private NotificationHelper(Context context) {
> +        mContext = context;

To be safe, this should be 'mContext = context.getApplicationContext()' to prevent accidentally passing a non-application context.

@@ +122,3 @@
>          }
>  
> +        JSONObject args = new JSONObject();

Nit: final

@@ +139,5 @@
>              }
> +
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Notification:Event", args.toString()));
> +        } catch (JSONException e) {
> +            Log.w(LOGTAG, "Error building JSON notification arguments.", e);

Nit: s/Log.w/Log.e/ since this is an error.

@@ +165,5 @@
> +
> +        try {
> +            final String id = message.getString(HANDLER_ATTR);
> +            b.appendQueryParameter(HANDLER_ATTR, id);
> +        } catch (JSONException ex) {

Nit: Log an error here too to avoid eating the exception.

::: mobile/android/modules/Notifications.jsm
@@ +177,5 @@
> +
> +  unregisterHandler: function(key, handler) {
> +    var i = _handlersMap[key].indexOf(handler);
> +    if (i > -1) {
> +        _handlersMap.slice(i, 1);

s/slice/splice/

@@ +220,3 @@
>            notification._onClick(id, notification._cookie);
> +
> +        if (handlerKey) {

Should this be 'handlerKey && cookie'? In your second patch, you're executing callbacks on Downloads without checking whether aCookie is null.

@@ +228,3 @@
>          break;
>        case "notification-button-clicked": {
> +        if (handlerKey) {

Same question here.

@@ +243,5 @@
>          }
>          break;
>        case "notification-cleared":
>        case "notification-closed":
> +        if (handlerKey) {

And here.

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

(In reply to Brian Nicholson (:bnicholson) from comment #13)
> would be better to put this in GeckoApplication's onCreate.

I'm fine with that.

> To avoid the side effect trickery, I'd also recommend moving the
> registerGeckoThreadListener call out of the NotificationHelper constructor
> and into a separate init() function, like we do at [1]. A couple of

I did this mostly for convenience. We DO need an instance of the NotificationHelper (at startup) when we have an intent coming in from a tapped Notification. I didn't want to rely on making sure an init() call had happened before any of those instances. To provide the same security with an init function, the other callers would have to do something like:

i = nh.getInstance();
if (i == null) {
  nh.init(context);
  i = nh.getInstance();
}

Or we'd have to force these to do initialization by default and pray no one ever forgot to do it:
nh.init(context); // no-op if already initialized
i = nh.getInstance();

or maybe?
nh.getInstance(context).init(); // This just seems silly... if every getInstance needs an init() call, getInstance should just call init...

Having a little odd-looking nh.getInstance(context) call in the middle of nowhere seemed like an prettier/safer solution. Curious if you have any other ideas.

> @@ +165,5 @@
> > +
> > +        try {
> > +            final String id = message.getString(HANDLER_ATTR);
> > +            b.appendQueryParameter(HANDLER_ATTR, id);
> > +        } catch (JSONException ex) {
> 
> Nit: Log an error here too to avoid eating the exception.

I intentionally left some of these out. We've got two API's here now (one for backwards compat). One of them doesn't need the handler, so the lack of it doesn't really mean much. That said, I'm fine logging something.
 
> Should this be 'handlerKey && cookie'? In your second patch, you're
> executing callbacks on Downloads without checking whether aCookie is null.

I didn't do this check because cookie isn't required (in either the old or new api).

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

Mostly cleanup.

Comment 17

[Brian Nicholson (:bnicholson)]

Comment on attachment 8443094 [details] [diff] [review]
Patch 1/2

Review of attachment 8443094 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly looks good, with a few questions about the optional cookie.

(In reply to Wesley Johnston (:wesj) from comment #14)
> > Should this be 'handlerKey && cookie'? In your second patch, you're
> > executing callbacks on Downloads without checking whether aCookie is null.
> 
> I didn't do this check because cookie isn't required (in either the old or
> new api).

The cookie not being required is why you *should* have a null check here, right? Otherwise, your callback in patch 2 will end up here with a null download ID: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/downloads.js#96.

(In reply to Wesley Johnston (:wesj) from comment #14)
> I did this mostly for convenience. We DO need an instance of the
> NotificationHelper (at startup) when we have an intent coming in from a
> tapped Notification. I didn't want to rely on making sure an init() call had
> happened before any of those instances.

That's the nice thing about putting this in Application -- there's no race to worry about. All activities/intent handlers/etc. are dependent on the Application, so it's guaranteed your init() will be called since the Application must be created first.

::: mobile/android/base/NotificationClient.java
@@ +128,5 @@
>              return;
>          }
>  
>          synchronized (this) {
> +            if (!mReady) {

Accidental change?

I think these changes got undone in patch 2, so you may want to move those changes back to here (or fold 1+2 when landing).

@@ +173,5 @@
>          mReady = true;
>      }
>  
>      protected void unbind() {
> +        mUpdatesMap.clear();

Nit: revert this change

::: mobile/android/base/NotificationHelper.java
@@ +64,5 @@
>      private static final String CLEARED_EVENT = "notification-cleared";
>      private static final String CLOSED_EVENT = "notification-closed";
>  
> +    private Context mContext;
> +    // Holds a list of notifications that should be cleared if the Fennec Activity is shut down.

Nit: space above comment

::: mobile/android/modules/Notifications.jsm
@@ +210,5 @@
>    observe: function notif_observe(aSubject, aTopic, aData) {
>      let data = JSON.parse(aData);
>      let id = data.id;
> +    let handlerKey = data.handlerKey;
> +    let cookie = JSON.parse(data.cookie);

If cookie is optional, won't this fail?

@@ +220,4 @@
>            notification._onClick(id, notification._cookie);
> +
> +        if (handlerKey) {
> +            _handlersMap[handlerKey].forEach(function(handler) {

Nit: indentation is still off here (4->2 spaces)

@@ +228,4 @@
>          break;
>        case "notification-button-clicked": {
> +        if (handlerKey) {
> +            _handlersMap[handlerKey].forEach(function(handler) {

Nit: indentation is still off here (4->2 spaces)

@@ +244,5 @@
>          break;
>        case "notification-cleared":
>        case "notification-closed":
> +        if (handlerKey) {
> +            _handlersMap[handlerKey].forEach(function(handler) {

Nit: indentation is still off here (4->2 spaces)

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

I fixed up that one cookie check. If downloads doesn't get a cookie, something has gone wrong. I think we want to fail badly there.

I also made an init function, but made getInstance() throw if we have an instance that wasn't initialized lying around. Does that sound ok? We could just log something instead. Heck, we could force the initialization to happen in that case, but for now it means something is wrong.

Comment 19

[Brian Nicholson (:bnicholson)]

Comment on attachment 8447391 [details] [diff] [review]
Patch 1/2

Review of attachment 8447391 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Wesley Johnston (:wesj) from comment #18) 
> I didn't do this check because cookie isn't required (in either the old or
> new api)

> I fixed up that one cookie check. If downloads doesn't get a cookie,
> something has gone wrong. I think we want to fail badly there.

I'm still confused. Here's my interpretation of what's happening:
1) The cookie given to handleNotificationIntent() is optional as you say in the comments
2) handleNotificationIntent() fires Notification:Event with the given cookie (which may be null since cookie is optional)
3) You end up calling 'handler.onClick(cookie);' in the "notification-clicked" case
4) That takes us to patch 2/2, where you forward this null cookie to 'Downloads.clickCallback(aCookie);'
5) Finally, we end up here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/downloads.js#96

At that point, "downloads doesn't get a cookie, something has gone wrong" as you said. So is cookie not really optional? Or should we not be doing this callback if cookie is null? Or am I misunderstanding what's happening?

> I also made an init function, but made getInstance() throw if we have an
> instance that wasn't initialized lying around. Does that sound ok? We could
> just log something instead. Heck, we could force the initialization to
> happen in that case, but for now it means something is wrong.

Since you're already forcing init() to be called before anything else, can you move the Context argument from getInstance() to init()? It only gets used the first time getInstance() is called anyway, so it seems pointless that the other callers have to pass in a Context when it's not even being used in those cases. mInitialized also seems like a wasted boolean.

Just trying to figure out why you dislike the pattern we use at [1]. From comment 14, it sounds like you're concerned about the order of execution, but the fact that an Application must exist before an Activity or BroadcastReceiver will guarantee that the singleton is initialized. Are you not convinced the Application constructor will happen first, or are you worried about something else?

Would something like this make you happier?:

public static void init(Context context) {
  if (sInstance != null) throw new Exception(...);

  // constructor can initialize mClearableNotifications, register listeners, etc.
  sInstance = new NotificationHelper(context);
}

public static NotificationHelper getInstance() {
  if (sInstance == null) throw new Exception(...);
  return sInstance;
}

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApplication.java#124

::: mobile/android/base/GeckoApplication.java
@@ +121,5 @@
>          Clipboard.init(getApplicationContext());
>          FilePicker.init(getApplicationContext());
>          GeckoLoader.loadMozGlue();
>          HomePanelsManager.getInstance().init(getApplicationContext());
> +        // This getInstance call will force initializatino of the NotificationHelper, but does nothing with the result

s/initializatino/initialization/

Not sure this comment is even necessary though -- it's just calling an init() like all the lines before it.

Comment 20

[Brian Nicholson (:bnicholson)]

Comment on attachment 8447391 [details] [diff] [review]
Patch 1/2

Review of attachment 8447391 [details] [diff] [review]:
-----------------------------------------------------------------

I think this makes a bit more sense to me now: cookies are optional for notifications in general, but not for downloads. Maybe it would make sense to throw in Patch 2 if aCookie is null before we invoke the Downloads service to prevent any bugs from propagating too far?

r=me, though I'd still prefer the Context argument be moved to init() instead of getInstance() as mentioned above. I guess I'm OK with this approach if you feel strongly about it, though.

Comment 21

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/6ee3c3ba17b5

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ee3c3ba17b5