1075476 - removing download notification restarts Firefox

Status: RESOLVED FIXED

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

Description

[Christian Ascheberg]

I am using a Nexus 4, Android 4.4.4.

STR:
1. download a file
2. kill Nightly after download is finished
3. remove download notification

Result:
- Firefox shows up again

Expected:
- Firefox should stay closed

Comment 1

[Aaron Train [:aaronmt]]

I don't think this is a recent regression. For a while, tapping the download notification did absolutely nothing: bug 997756

Comment 2

[Wesley Johnston (:wesj)]

You don't need a window here. This is probably just from bug 1004495 (but before bug 1004495 we just did nothing in this case).

Comment 3

[Wesley Johnston (:wesj)]

I flipped us to the new downloads stuff in Firefox 32 (bug 1004495).

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch

This moves our notification handling to a broadcast receiver, at least for the initial handling. If the receiver sees that the notification is being cancelled, and knows that Gecko isn't running, it just throws away the event. 

If Gecko is running, it notifies Gecko (without bringing Fennec to the foreground) so that JS can do things if it wants. I had to pull out some code in NotifcationHelper to build a JSON response to do that.

If its not a cancel notification, we force the intent to be handled by GeckoApp.

I'm going to look into some tests for this code (by mocking out the NotificationManager...)

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8499199 [details] [diff] [review]
Patch

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

What does Gecko do with the notifications if it's running? Either it's necessary (in which case this patch won't work) or it isn't (in which case dropping it might be better). See comment below.

::: mobile/android/base/NotificationHelper.java
@@ +1,1 @@
> +

Nit: remove newline

@@ +186,5 @@
>          notificationIntent.setData(dataUri);
>          notificationIntent.putExtra(HELPER_NOTIFICATION, true);
>          notificationIntent.putExtra(COOKIE_ATTR, message.optString(COOKIE_ATTR));
> +
> +        // All intents get routed through the notificationReceiver. That lets us bail if we don't want to start Gecko

If the download manager can remove downloads on its own, is it worth notifying Gecko at all? Aside from being explicitly killed, remember that Fennec can also be killed at any point while in the background -- but still alive from the user's perspective -- so it's inconsistent if Gecko sometimes receives notifications and sometimes doesn't. I'd argue that no notifications are better than flaky ones.

::: mobile/android/base/NotificationReceiver.java
@@ +1,1 @@
> +

Nit: remove newline

@@ +15,5 @@
> +import android.net.Uri;
> +import android.util.Log;
> +
> +/* Broadcast receiver for Notifications. Will forward them to GeckoApp (and start Gecko) if they're clicked.
> +   If they're being dismissed will not start Gecko, but may forward them to JS if Gecko is running. */

Nit: use JavaDoc style

/**
 * <comments>
 */

Comment 6

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #5)
> If the download manager can remove downloads on its own, is it worth
> notifying Gecko at all? Aside from being explicitly killed, remember that
> Fennec can also be killed at any point while in the background -- but still
> alive from the user's perspective -- so it's inconsistent if Gecko sometimes
> receives notifications and sometimes doesn't. I'd argue that no
> notifications are better than flaky ones.

I don't quite understand this comment. The API having onCancel is a bit of an artifact of the old API, which assumed (and still tries) to kill all notifications when Gecko dies. That obviously doesn't work when we're force killed.

You want to remove that API entirely? We could do that and not switch to a handler at all here I guess.

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 8499199 [details] [diff] [review]
Patch

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

(In reply to Wesley Johnston (:wesj) from comment #6)
> I don't quite understand this comment. The API having onCancel is a bit of
> an artifact of the old API, which assumed (and still tries) to kill all
> notifications when Gecko dies. That obviously doesn't work when we're force
> killed.

OK, so we just need to ping Gecko to make sure that stored notifications are cleaned up so we don't leak them? That's all I was wondering -- just wanted to make sure we aren't relying on this event to do important things that may or may not happen.

Comment 8

[Carsten Book [:Tomcat]]

Hi,

could you provide a try run, thanks!

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 11

[Aaron Train [:aaronmt]]

(In reply to Carsten Book [:Tomcat] from comment #8)
> Hi,
> 
> could you provide a try run, thanks!

Comment 12

[Wesley Johnston (:wesj)]

https://tbpl.mozilla.org/?tree=Try&rev=a6861d43ff75

Comment 14

[Chien Yu]

https://drive.google.com/file/d/0B2z0K-XuUT7KaUMtRWJPRHE1WHM/view?usp=sharing

Comment 15

[Chien Yu]

In the notification bar select the downloaded picture (file) will first start Firefox, and then start the picture viewer.

Comment 16

[Wesley Johnston (:wesj)]

I filed bug 1091718 for a way to improve that.

Comment 17

[Jan Manthay]

Is there any progress in this bug?

Comment 18

[Aaron Train [:aaronmt]]

This has a patch that never landed. It might be bitrot now. NI Brian since Wes is out.

Comment 19

[Brian Nicholson (:bnicholson)]

Rebased and pushed to try a few days ago, but it looks like some things have changed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=abb702035a8d. Probably not going to spend any more time on this for now since it's not a high priority.

Comment 21

[Robert (rrayborn, he/him)]

FYI, 10+ users have commented on this on Input.  I agree it's not a high priority, but it is being noticed.  https://input.mozilla.org/en-US/?q=clear+%2B+notification+%2B+%28open+reopen+launch%29&date_start=2015-02-11

Comment 23

[Aaron Train [:aaronmt]]

Re-triage this

Comment 25

[Nicolae Albu]

This might not be high priority, but it causes Firefox to crash (see bug 1167755).

Comment 26

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

Martyn - Can you pick this up? Chat with Wes and Margaret for background.

Comment 27

[gwaeraurond]

This issue is independent of the download manager. When I clear any group of notifications on either of my Android devices (phone or tablet) it will cause Firefox to launch, even if the notifications have nothing to do with Firefox or downloads of any kind.

Comment 28

[Nicolae Albu]

(In reply to gwaeraurond from comment #27)
> This issue is independent of the download manager. When I clear any group of
> notifications on either of my Android devices (phone or tablet) it will
> cause Firefox to launch, even if the notifications have nothing to do with
> Firefox or downloads of any kind.

Can you present some steps to consistently reproduce your issue?
Because for me, the only notifications that produce the issue are Firefox's...

Comment 29

[Yajo]

(In reply to Nicolae Albu from comment #28)
> for me, the only notifications that produce the issue are
> Firefox's...

For me too.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Renom for new assignee.

Comment 31

[Eugen Sawin [:esawin]]

This does not reproduce for me on Android M (Nexus 5), the download notification is automatically dismissed when Fennec is killed.

Comment 32

[Eugen Sawin [:esawin]]

Which might not be the expected behavior either.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Assigned to investigate (which versions does this affect?) and see how we can move this forward. Supposedly, the patch should work, provided it gets rebased.

Comment 34

[Jan Henning [:JanH]]

Actually killing Firefox is not necessary - simply switching back to the home screen or some other app suffices. Swiping away the download notification then causes Firefox to come back into the foreground.

I've tested on Android 4.1.2 (Galaxy S3 Mini) and the download notification persists if Firefox is backgrounded, killed by the low memory killer or even swiped away in the "Recent tasks" list. The notification only disappears if I use the "Force stop" option from Android's app manager.

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Jan Henning (:JanH) from comment #34)
> Actually killing Firefox is not necessary - simply switching back to the
> home screen or some other app suffices.

Depending on the device, it could be that backgrounding Firefox is getting it killed anyway (which is the same as swiping it away).

Comment 36

[Sebastian Kaspari (:sebastian; :pocmo)]

Just saw this on Reddit:
https://www.reddit.com/r/firefox/comments/437w9i/firefox_android_notifications/

And found this bug here. I never noticed that behavior but it's indeed weird that Firefox comes up after you dismiss the notification:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/NotificationHelper.java#302-303

Comment 37

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The code sebastian linked to in comment 36 shows we set the deleteIntent to the result of `PendingIntent.getActivity`, which is why Firefox is opening again.

It looks like we're doing this to generically remove our queue of notifications [1]:
124         if (CLEARED_EVENT.equals(notificationType)) {
125             mClearableNotifications.remove(id);
126             // If Gecko isn't running, we throw away events where the notification was cancelled.
127             // i.e. Don't bug the user if they're just closing a bunch of notifications.
128             if (!GeckoThread.isRunning()) {
129                 return;
130             }
131         }

The Activity, since it's a foreground UI Object, is not the place to do this. Maybe we could handle this by receiving the PendingIntent in a BroadcastReceiver/IntentService to update the state. However, the state is kept in memory, which is suspicious in and of itself – I filed bug 1244820.

It looks like mClearableNotifications is used as a list of open notifications that we'd want to close when the Fennec Activity is closed and the deleteIntent is used to clear notifications from the HashMap. It looks like some of this state is syncing information to Gecko, so that could be why this seems unnecessarily complicated.

I can't think of a quick fix off the top of my head, expect storing some of the notification state in memory so we can access it from a Service or BroadcastReceiver (which could handle clearing the mClearableNotifications state).

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/NotificationHelper.java#124

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

It looks like wes' WIP patch moved the receiving to a BroadcastReceiver which ensures the GeckoThread is started before starting the Activity. The one fallacy here is that the GeckoThread state isn't a perfect mirror of the Activity state and they can diverge more in the future.

That being said, it could be the quick fix we want.

Comment 39

[:Margaret Leibovic]

Let's try to get this fixed for 47.

Comment 40

[Kevin Brosnan [Ex-Mozilla]]

48 maybe?

Comment 42

[donrhummy]

(In reply to Kevin Brosnan [:kbrosnan] from comment #40)
> 48 maybe?

Is there anything users can do to help move this along?

Comment 43

[Kevin Brosnan [Ex-Mozilla]]

From Mike/Wes's investigation it is a thorny concurrency issue between the Java frontend and the NDK compiled Gecko.

Comment 44

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Jonathan was going to take a look into this one.

Comment 45

[Jonathan Almeida [:jonalmeida]]

Re-applied wesj's original patch with minor modifications where GeckoAppShell.createBroadcastEvent has been replaced with GeckoAppShell.notifyObservers.

I've done some simple tests to see if the notification goes way:

1. Open Fennec and download an image.
2. Click the recent apps button and swipe away Fennec.
3. Attempt to swipe away the download notification.
4. Observer that Fennec isn't opened again after swiping.

Is there anything else you think I should try to test out? This does seem like a quick win, but I'm wondering if there is some other behaviour I should try observing for.

Comment 46

[Jonathan Almeida [:jonalmeida]]

Attachment: Removing download notification restarts Firefox

Comment 47

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Jonathan Almeida (:jonalmeida) from comment #45)
> Is there anything else you think I should try to test out?

Spoke on irc & irl. It'd be good to get context on why we keep the notifications in memory to see what other states could be going on here.

That being said, we think we keep the notifications in memory to sync with Gecko so it might good to also test the state when Gecko is dead but Android is in memory (though, I'm not quite sure how to do that, or if we'd ever hit in the wild).

Comment 48

[Nicolae Albu]

(In reply to Jonathan Almeida (:jonalmeida) from comment #45)
> Is there anything else you think I should try to test out? This does seem
> like a quick win, but I'm wondering if there is some other behaviour I
> should try observing for.

Yes, please see bug 1167755.
Even if it doesn't crash (because it has sufficient memory, depending on the specs of the device), it makes Firefox hang a couple of seconds (directly proportional to the number of notifications) after the notifications are cleared during which rendering (and others) is halted - for example, if I have a tab with sufficient vertical scrolling (like a forum or even Bugzilla), if I scroll or zoom, it's jerky and the page remains blurry until all the notifications are "processed".

Comment 49

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8756553 [details] [diff] [review]
Removing download notification restarts Firefox

According to the comments in the code, we keep the in-memory map of notifications for those that aren't tied to Gecko being alive, i.e. notifications without the 'persistent' flag. We use this map when we call `destroy` in the `NotificationHelper`, which removes only those notifications using the id that we've saved. Now, the destroy call is made only in `GeckoApp.onDestroy` which will only get called when low on resources, and it's also tied to the lifetime of gecko so the notifications would be in-memory along with gecko.

If onDestroy is called, should we be removing all notifications or just the non-persistent ones? That seems to be the main reason to keep the notifications in-memory. So if this is the expected behaviour, then keeping it in-memory is useful.

With the new patch, we check within the BroadcastReceiver if Gecko is running first. If not, we don't send it onward to gecko, otherwise we send it on without checking the in-memory map. But in that situation where gecko isn't there, the map won't be there either, so there won't be anything to keep in sync.

Comment 50

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8756553 [details] [diff] [review]
Removing download notification restarts Firefox

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

This code is hard to follow because of the code duplication. Note that code duplication is generally a red flag – it's inevitable that the two code paths will diverge and most of the time, that's not intentional! Let's get rid of that and I'll have another review pass.

GeckoApp.initialize still calls into NotificationHelper.handleNotificationIntent –  do we still need that call?

Finally, this doesn't apply cleanly for me – be sure to rebase.

::: mobile/android/base/AndroidManifest.xml.in
@@ +239,5 @@
>              </intent-filter>
>          </receiver>
>  
> +        <receiver
> +            android:name="org.mozilla.gecko.NotificationReceiver">

add android:exported="false" - this is *very* important for security reasons.

::: mobile/android/base/java/org/mozilla/gecko/NotificationHelper.java
@@ +52,5 @@
>      private static final String ACTION_ICON_ATTR = "icon";
>      private static final String PERSISTENT_ATTR = "persistent";
>      private static final String HANDLER_ATTR = "handlerKey";
>      private static final String COOKIE_ATTR = "cookie";
> +    public static final String ORIGINAL_COMPONENT_EXTRA = "originalComponent";

nit: Should this be package private too?

@@ +126,5 @@
>                  args.put(ACTION_ID_ATTR, actionName);
>              }
>  
>              Log.i(LOGTAG, "Send " + args.toString());
>              GeckoAppShell.notifyObservers("Notification:Event", args.toString());

This method calls notifyObservers, as does the caller – we don't want to send the event twice, right?

@@ +133,5 @@
>          }
> +        return args;
> +    }
> +
> +    public void handleNotificationIntent(SafeIntent i) {

Why is the code here largely duplicated with the code in NotificationReceiver? Don't we already handle all of the edge cases there?

::: mobile/android/base/java/org/mozilla/gecko/NotificationReceiver.java
@@ +14,5 @@
> +import android.content.Intent;
> +import android.net.Uri;
> +import android.util.Log;
> +
> +/* Broadcast receiver for Notifications. Will forward them to GeckoApp (and start Gecko) if they're clicked.

javadoc is:

/**
 * <code comments here>
 * <line 2>
 */

/** is especially important so we could potentially generate API docs in the future.

@@ +15,5 @@
> +import android.net.Uri;
> +import android.util.Log;
> +
> +/* Broadcast receiver for Notifications. Will forward them to GeckoApp (and start Gecko) if they're clicked.
> +   If they're being dismissed will not start Gecko, but may forward them to JS if Gecko is running. */

If I understand the code correctly, you may want to add that this is the *only* entry point for Notification intents.

nit: "If they're being dismissed, <?> will not start Gecko, but <?>..."

@@ +17,5 @@
> +
> +/* Broadcast receiver for Notifications. Will forward them to GeckoApp (and start Gecko) if they're clicked.
> +   If they're being dismissed will not start Gecko, but may forward them to JS if Gecko is running. */
> +public class NotificationReceiver extends BroadcastReceiver {
> +    private static final String LOGTAG = "NotificationReceiver";

nit: By convention, we prepend the logtag with "Gecko". Also, for a little more safety:

`"Gecko" + NotificationReceiver.class.getSimpleName()`

@@ +42,5 @@
> +            }
> +            return;
> +        }
> +
> +        // Otherwise forward the event to Gecko

nit: Feel free to disagree with me, but one coding strategy I really like is if I'm thinking of writing a comment in-line, I consider abstracting it away to a helper method to help with readability & to make sure comments don't get out-of-date (because method names are less likely to go out-of-date than comments). For example:

public void onReceive(Context context, Intent intent) {
  // early return for errors ...

  if (NotificationHelper.CLEARED_EVENT.equals(notificationType)) {
    maybeNotifyNotificationClearedObservers();
    return;
  }

  forwardMessageToActivity();
}

@@ +43,5 @@
> +            return;
> +        }
> +
> +        // Otherwise forward the event to Gecko
> +        ComponentName name = intent.getExtras().getParcelable(NotificationHelper.ORIGINAL_COMPONENT_EXTRA);

nit: final

::: mobile/android/base/moz.build
@@ +469,5 @@
>      'MotionEventInterceptor.java',
>      'NotificationClient.java',
>      'NotificationHandler.java',
>      'NotificationHelper.java',
> +    'NotificationReceiver.java',

Not something you need to fix but I think it'd be cool to move these Notification* to a new package (like that notifications/ package right below here! :P But there may be a reason they're not in there already).

Comment 51

[Jonathan Almeida [:jonalmeida]]

Sorry this took me a long time to finally get back to.

(In reply to Michael Comella (:mcomella) from comment #50)
> Comment on attachment 8756553 [details] [diff] [review]
> Removing download notification restarts Firefox
> 
> Review of attachment 8756553 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This code is hard to follow because of the code duplication. Note that code
> duplication is generally a red flag – it's inevitable that the two code
> paths will diverge and most of the time, that's not intentional! Let's get
> rid of that and I'll have another review pass.

I'm not sure what I did wrong earlier but the patch I uploaded was merged with an older working patch. It explains why I wasn't seeing any merge conflicts locally but you were. I'll be sure to check carefully when I use `hg bzexport` again.

> GeckoApp.initialize still calls into
> NotificationHelper.handleNotificationIntent –  do we still need that call?

True, I didn't notice that earlier. We don't require it during initialize anymore.

> nit: Feel free to disagree with me, but one coding strategy I really like is
> if I'm thinking of writing a comment in-line, I consider abstracting it away
> to a helper method to help with readability & to make sure comments don't
> get out-of-date (because method names are less likely to go out-of-date than
> comments). For example:
> 
> public void onReceive(Context context, Intent intent) {
>   // early return for errors ...
> 
>   if (NotificationHelper.CLEARED_EVENT.equals(notificationType)) {
>     maybeNotifyNotificationClearedObservers();
>     return;
>   }
> 
>   forwardMessageToActivity();
> }
> 

I like this; using it. It feels a bit alien in our code base, but I'll try to use this style more.

> ::: mobile/android/base/moz.build
> @@ +469,5 @@
> >      'MotionEventInterceptor.java',
> >      'NotificationClient.java',
> >      'NotificationHandler.java',
> >      'NotificationHelper.java',
> > +    'NotificationReceiver.java',
> 
> Not something you need to fix but I think it'd be cool to move these
> Notification* to a new package (like that notifications/ package right below
> here! :P But there may be a reason they're not in there already).

Filed bug 1279301.

Fixed all other nits as well.

Comment 52

[Jonathan Almeida [:jonalmeida]]

Attachment: Removing download notification restarts Firefox

MozReview-Commit-ID: 35pn1nntcsV

Comment 53

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Margaret, could you take a look at this while I'm on PTO? Thanks!

Comment 54

[:Margaret Leibovic]

I think Sebastian is better qualified to review this...

Comment 55

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8761756 [details] [diff] [review]
Removing download notification restarts Firefox

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

::: mobile/android/base/java/org/mozilla/gecko/NotificationHelper.java
@@ +61,5 @@
>      private static final String CLOSED_EVENT = "notification-closed";
>  
> +    static final String EVENT_TYPE_ATTR = "eventType";
> +    static final String CLEARED_EVENT = "notification-cleared";
> +    static final String ORIGINAL_COMPONENT_EXTRA = "originalComponent";

nit: Why the reordering of EVENT_TYPE_ATTR and CLEARED_EVENT? Seems like previously events and attrs were grouped. The grouping might not be useful - but now it's mixed. :)

@@ -117,5 @@
> -        final String notificationType = data.getQueryParameter(EVENT_TYPE_ATTR);
> -        if (id == null || notificationType == null) {
> -            Log.e(LOGTAG, "handleNotificationEvent: invalid intent parameters");
> -            return;
> -        }

Is this check not needed anymore?

@@ -122,4 @@
>  
> -        // In case the user swiped out the notification, we empty the id set.
> -        if (CLEARED_EVENT.equals(notificationType)) {
> -            mClearableNotifications.remove(id);

We stop removing the notification here but we don't do it in NotificationReceiver now. Is this correct?

@@ +139,5 @@
> +        final Uri data = i.getData();
> +        final String notificationType = data.getQueryParameter(EVENT_TYPE_ATTR);
> +
> +        final JSONObject args = getArgsFromIntent(i);
> +        GeckoAppShell.notifyObservers("Notification:Event", args.toString());

The previous code would not send an event if a JSONException was thrown. Now getArgsFromIntent() will return an empty or partial JSONObject and we will send this event.

Comment 56

[Sebastian Kaspari (:sebastian; :pocmo)]

Reflag me for review if needed :)

Comment 57

[Jonathan Almeida [:jonalmeida]]

> ::: mobile/android/base/java/org/mozilla/gecko/NotificationHelper.java
> @@ +61,5 @@
> >      private static final String CLOSED_EVENT = "notification-closed";
> >  
> > +    static final String EVENT_TYPE_ATTR = "eventType";
> > +    static final String CLEARED_EVENT = "notification-cleared";
> > +    static final String ORIGINAL_COMPONENT_EXTRA = "originalComponent";
> 
> nit: Why the reordering of EVENT_TYPE_ATTR and CLEARED_EVENT? Seems like
> previously events and attrs were grouped. The grouping might not be useful -
> but now it's mixed. :)

The different indentation bothered me, I've put it back but at the end of each group.

> @@ -117,5 @@
> > -        final String notificationType = data.getQueryParameter(EVENT_TYPE_ATTR);
> > -        if (id == null || notificationType == null) {
> > -            Log.e(LOGTAG, "handleNotificationEvent: invalid intent parameters");
> > -            return;
> > -        }
> 
> Is this check not needed anymore?

Thanks for the catch, this should be there again.

> @@ -122,4 @@
> >  
> > -        // In case the user swiped out the notification, we empty the id set.
> > -        if (CLEARED_EVENT.equals(notificationType)) {
> > -            mClearableNotifications.remove(id);
> 
> We stop removing the notification here but we don't do it in
> NotificationReceiver now. Is this correct?

Created bug 1281822 to track removing of mClearableNotifications since it's hard to tell how useful it is.

> @@ +139,5 @@
> > +        final Uri data = i.getData();
> > +        final String notificationType = data.getQueryParameter(EVENT_TYPE_ATTR);
> > +
> > +        final JSONObject args = getArgsFromIntent(i);
> > +        GeckoAppShell.notifyObservers("Notification:Event", args.toString());
> 
> The previous code would not send an event if a JSONException was thrown. Now
> getArgsFromIntent() will return an empty or partial JSONObject and we will
> send this event.

Yes, this seems wrong. I've changed it so that we send the event within the try block itself and avoid returning anything. I've changed the `NotificationReceiver` to do this as well.

Comment 58

[Jonathan Almeida [:jonalmeida]]

Attachment: Bug 1075476 - Removing download notification restarts Firefox

Review commit: https://reviewboard.mozilla.org/r/60384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60384/

Comment 59

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8764661 [details]
Bug 1075476 - Removing download notification restarts Firefox

https://reviewboard.mozilla.org/r/60384/#review57396

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/3a6ed18fa2a9
Removing download notification restarts Firefox r=sebastian

Comment 61

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3a6ed18fa2a9

Comment 62

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8761756 [details] [diff] [review]
Removing download notification restarts Firefox

Thanks Sebastian!

Comment 63

[Teodora Vermesan (:TeoVermesan)]

Firefox does not open after downloading a file, closing Firefox and removing the download notification, so:
Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 50.0a1 (2016-06-29)