823285 - Private browsing downloads not cleared from Android notification bar

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Open a private tab and start a download
2) Close the private tab

The download remains in the Android notifications list, stuck at whatever % it was when the private tab was closed. Even worse, clicking it does nothing, and it stays in the notification bar without any way to get rid of it (even clearing the X to close all notifications doesn't work).

Comment 1

[Josh Matthews [:jdm]]

Hmm, my first thought was to have an observer of last-pb-context-exited that iterates through the activePrivateDownloads and clears corresponding notifications, but that won't work if the download manager gets the notification first. Perhaps it's easiest to just store a list of notifications that will need clearing and do so in last-pb-context-exited.

Comment 2

[Josh Matthews [:jdm]]

Alternatively, what happens with notifications for downloads that are cancelled partway through right now (or Fennec closes)? If those are properly handled, there shouldn't be any need for special private notification handling.

Comment 3

[Brian Nicholson (:bnicholson)]

(In reply to Josh Matthews [:jdm] from comment #2)
> Alternatively, what happens with notifications for downloads that are
> cancelled partway through right now (or Fennec closes)? If those are
> properly handled, there shouldn't be any need for special private
> notification handling.

If it's cancelled, Fennec deletes the file and the download is removed from the Android notification bar. But if Fennec is closed, the downloads hang around, stuck at whatever % they were on when Fennec was killed (as described in comment 0).

Comment 4

[Aaron Train [:aaronmt]]

I checked just now, this is still an issue (Nightly 02/08).

Comment 5

[Brian Nicholson (:bnicholson)]

Investigating this further, I think this bug may be impossible to fully fix this without some bigger changes. We could add a last-pb-context-exited listener that iterates through and clears an internal list of private downloads; however, this isn't the only situation where we need to be concerned about clearing downloads.

The other is if we're killed, which happens when swiping Fennec from the recent apps list in ICS. We've been promoting this as the standard way to quit Firefox (see bug 800071). Unfortunately, when we're killed, there's no guarantee that the onDestroy() callback will be fired, meaning there's simply no way to reliably ensure that private downloads are removed from the notification bar.

Downloads in general - not just private ones - are affected by this behavior. Any time Fennec is killed during an in-progress download, the download stays stuck in the notification bar. When this happens, the download can't even be removed from the list (by swiping it out or hitting the "clear all" button) since it's a progress notification. So if users expect that their private download notifications will be cleared when Fennec is closed, this behavior will obviously break that expectation.

For the short term, I can think of the following options:
1) We don't treat private browsing downloads as private. Even though we still suffer from stuck download bug described above, users will at least expect private downloads to behave the same as normal ones. This is the same behavior seen in private mode in Chrome and the stock browser (which simply use the Android download manager for all downloads).
2) We don't add private downloads to the notification bar. Users may feel something is broken since notifications for downloads is the standard Android behavior.
3) We don't support downloads at all in private browsing mode.

For the long term, we may want to consider creating our own download service to handle downloads. We would have more reliable pause/resume, we could continue downloads after Fennec is killed, and we may even be able to watch for Fennec being killed so we can remove any private download notifications.

Comment 6

[Brian Nicholson (:bnicholson)]

Ian/Madhava, any opinions given comment 5? Knowing Ehsan (who's currently on PTO), I think he'd probably opt for #2, perhaps with some alternate, less persistent UI for in-progress private downloads.

Comment 7

[Brian Nicholson (:bnicholson)]

Just so UX is aware, it's also worth mentioning that we don't (and can't) clear the private download files themselves when private tabs are closed. This is consistent with desktop's behavior when private windows are closed, but it's less consistent with our non-private downloads on Android (whose files are actually deleted when they're removed from the download manager).

Comment 8

[(no longer active)]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> Ian/Madhava, any opinions given comment 5? Knowing Ehsan (who's currently on
> PTO), I think he'd probably opt for #2, perhaps with some alternate, less
> persistent UI for in-progress private downloads.

I do prefer #2.  :-)

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Brian: if going ahead with #2 what's the LOE here?  Is this something you can prepare in the next couple of weeks to uplift before Beta 4?  That's the last beta we take speculative fixes on so we've got two week here before this is more likely to move to tracking for FF21.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Also - if #2 is the option to go with, we can warn SUMO ahead of time to have an article ready with the shipping of FF20 to explain new download behaviour in PB mode.

Comment 11

[Brian Nicholson (:bnicholson)]

In IRC, Ian said he felt option #1 was the best of the three. Ehsan, I think you've rejected this before, but is that an option you think we can consider?

If not, I'll need some UX direction to know how to proceed with #2.

Comment 12

[Brian Nicholson (:bnicholson)]

(In reply to Lukas Blakk [:lsblakk] from comment #9)
> Brian: if going ahead with #2 what's the LOE here?  Is this something you
> can prepare in the next couple of weeks to uplift before Beta 4?  That's the
> last beta we take speculative fixes on so we've got two week here before
> this is more likely to move to tracking for FF21.

I'm also curious to hear Ehsan's take on this. Is this something we'd have to block PB on in Fx20? In its current state on Beta, it's possible for private downloads to be stuck in the global notification bar when the last private tab is closed (in fact, even when Fennec is closed entirely).

I don't think we can ship PB in Fx20 if we have any holes in our implementation due to security concerns. But this bug seems to be more of a grey area.

Comment 13

[(no longer active)]

Hmm, if we ship Fx20 with (In reply to Brian Nicholson (:bnicholson) from comment #11)
> In IRC, Ian said he felt option #1 was the best of the three. Ehsan, I think
> you've rejected this before, but is that an option you think we can consider?

For the record, I'm really really not comfortable with this, but this is a UX call, so I won't object to shipping Fx20 with option #1, but we should do our best to move away from option #1 for Fx21.

(In reply to Brian Nicholson (:bnicholson) from comment #12)
> (In reply to Lukas Blakk [:lsblakk] from comment #9)
> > Brian: if going ahead with #2 what's the LOE here?  Is this something you
> > can prepare in the next couple of weeks to uplift before Beta 4?  That's the
> > last beta we take speculative fixes on so we've got two week here before
> > this is more likely to move to tracking for FF21.
> 
> I'm also curious to hear Ehsan's take on this. Is this something we'd have
> to block PB on in Fx20? In its current state on Beta, it's possible for
> private downloads to be stuck in the global notification bar when the last
> private tab is closed (in fact, even when Fennec is closed entirely).

I _think_ it's enough to change all callers of nsIDownloadManager::AddDownload, but Paolo/Marco should be able to confirm this for a fact.


> I don't think we can ship PB in Fx20 if we have any holes in our
> implementation due to security concerns. But this bug seems to be more of a
> grey area.

FWIW, I'm worried about privacy here.  There is no security concern with this bug.

Comment 14

[Ian Barlow (:ibarlow)]

> For the record, I'm really really not comfortable with this, but this is a
> UX call, so I won't object to shipping Fx20 with option #1, but we should do
> our best to move away from option #1 for Fx21.

I agree that it isn't the right long term strategy, and that we can improve the experience from a privacy perspective in the long run. My concern about option 2 (not showing the download in the notification bar) is that without the notification, the user has no direct path to the file once it's downloaded. So they either think that the download failed or disappeared, or in the best case they manage to find their way to the Downloads screen which can be perceived as a rather disconnected experience. 

The rationale for starting with option 1 is that from Firefox's perspective, while we are protecting the user by not recording the places they visit on the internet, if they consciously make the decision to download something to their phone, it seems reasonable to assume that Firefox may not be able to hide that from the view of others anymore. 

We even say that on about:privatebrowsing: "Files you download and bookmarks you make will be kept."

If others are opposed to this rationale, though, I am open to this blocking the PB release and Brian and I can work on a more privacy-forward UX. Note that it will undoubtedly require some additional UI and strings, which will likely push us out a release or so.

Comment 15

[:Paolo Amadini]

(In reply to :Ehsan Akhgari from comment #13)
> (In reply to Brian Nicholson (:bnicholson) from comment #12)
> > (In reply to Lukas Blakk [:lsblakk] from comment #9)
> > > Brian: if going ahead with #2 what's the LOE here?  Is this something you
> > > can prepare in the next couple of weeks to uplift before Beta 4?  That's the
> > > last beta we take speculative fixes on so we've got two week here before
> > > this is more likely to move to tracking for FF21.
> > 
> > I'm also curious to hear Ehsan's take on this. Is this something we'd have
> > to block PB on in Fx20? In its current state on Beta, it's possible for
> > private downloads to be stuck in the global notification bar when the last
> > private tab is closed (in fact, even when Fennec is closed entirely).
> 
> I _think_ it's enough to change all callers of
> nsIDownloadManager::AddDownload, but Paolo/Marco should be able to confirm
> this for a fact.

What's the question exactly? If you're asking about not adding private downloads
to the notification bar, I suppose this would only require changing the download
listeners, in mobile-specific code.

If you're asking about not clearing private downloads when the last private
browsing tab is closed, we can add a hook for that in DownloadManager.cpp,
but we still need the correct privacy flag on the download, for example to use
the correct cookies on resume. Private downloads handled in this way would
still be slightly different in behavior from public downloads, in that
they're not resumed or preserved when the session restarts. Changing this
behavior would be a much more invasive change than just the hook.

Comment 16

[Brian Nicholson (:bnicholson)]

Rather than implementing a full-blown download service, we might be able to implement a service that just handles the notifications and keeps Fennec alive in the background during downloads by using http://developer.android.com/reference/android/app/Service.html#startForeground(int,%20android.app.Notification). Hopefully, we could hook into the Service's onDestroy() callback to do any cleanup. I'm testing this out now, so I'll have more information by tomorrow.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Brian Nicholson (:bnicholson) from comment #16)
> I'm testing
> this out now, so I'll have more information by tomorrow.

Brian - how did the testing go?  We're looking to have the temporary (or permanent if possible) fix for this landed on trunk, verified, then nominated for uplift before Tues Mar 12th when we go to build with FF20 beta 4.  Can you meet that timeline?

Comment 18

[Brian Nicholson (:bnicholson)]

(In reply to Lukas Blakk [:lsblakk] from comment #17)
> (In reply to Brian Nicholson (:bnicholson) from comment #16)
> > I'm testing
> > this out now, so I'll have more information by tomorrow.
> 
> Brian - how did the testing go?  We're looking to have the temporary (or
> permanent if possible) fix for this landed on trunk, verified, then
> nominated for uplift before Tues Mar 12th when we go to build with FF20 beta
> 4.  Can you meet that timeline?

After some testing, it looks like it should fix most of our download manager UX problems. I was wrong about the onDestroy() callback in comment 16; by having a foreground service, we will never get killed in the first place; that is, even if the user swipes Fennec from the recent apps list, Gecko (and the download) stay alive, and the same session is still open when the user clicks Fennec again.

There's just a few tweaks that need to be made, so hopefully I can have a working patch by this weekend.

Comment 19

[Brian Nicholson (:bnicholson)]

Attachment: Use foreground notification service for downloads

This patch uses a foreground service for in-progress downloads, forcing Fennec to stay alive. This means that we won't be killed from OOM or swiping Fennec from the recent apps list, so downloads should never get to a frozen, stuck state where they can't be removed from the notification bar. It's still possible to interrupt the download by force closing Fennec, but this will get rid of all notifications.

One problem with this approach is that when a download completes, we remove the notification showing its progress, and then we create a new static notification showing that the download is complete. This patch destroys the service whenever all notifications are removed, so between the download notification being removed and the complete notification being shown, the service will be destroyed and immediately recreated. Doing some brief benchmarking, this only seems to be a performance hit of a few tens of ms at most, so it hopefully isn't a critical problem we need to address with this patch - but it's something to be aware of so we can fix it in the future.

Another change worth pointing out is that this removes the createShutdownEvent when GeckoApp is being destroyed. Since downloads keep Fennec (and Gecko) alive when the GeckoApp activity is destroyed, this was causing the download to abort and the notification to freeze. In the past, I've noticed that onDestroy() isn't guaranteed to be called anyway, and the same behavior is described here: http://developer.android.com/reference/android/app/Activity.html#onDestroy%28%29. And since we can't rely it, I'd argue there's no point in having it at all.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 723250 [details] [diff] [review]
Use foreground notification service for downloads

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

Found some issues and a bunch of readability cleanup but overall it looks pretty good. The changes to GeckoApp worry me the most, I don't know what kind of effects removing that shutdown event will have.

::: mobile/android/base/GeckoApp.java
@@ +1733,5 @@
>                  onSearchRequested();
>              }
>          }
>  
> +        processNonUriIntent(intent);

processNonUriIntent calls addTab() if the action is ACTION_WIDGET. Seems like this will now happen twice because the code block just above here might also call addTab()

@@ -2015,5 @@
> -        // Tell Gecko to shutting down; we'll end up calling System.exit()
> -        // in onXreExit.
> -        if (isFinishing())
> -            GeckoAppShell.sendEventToGecko(GeckoEvent.createShutdownEvent());
> -

Hmm.. I'm not sure that removing this is safe. It seems like it does some important things. You may be right that it doesn't get called when Fennec is swipe-killed but then that seems like something we should fix.

::: mobile/android/base/GeckoAppShell.java
@@ +1186,5 @@
>  
> +    public static void setNotificationClient(NotificationServiceClient client) {
> +        if (sNotificationClient == null) {
> +            sNotificationClient = client;
> +        }

Add an else clause here which logs a warning. Just in case.

@@ +1268,5 @@
>      }
>  
>      private static void removeNotification(int notificationID) {
> +        if (sNotificationClient != null) {
> +            sNotificationClient.remove(notificationID);

Why would sNotificationClient be null here, but not in say alertsProgressListener_OnProgress?

::: mobile/android/base/GeckoApplication.java
@@ +30,5 @@
>          GeckoBatteryManager.getInstance().start();
>          GeckoNetworkManager.getInstance().init(getApplicationContext());
>          MemoryMonitor.getInstance().init(getApplicationContext());
> +
> +        GeckoAppShell.setNotificationClient(new NotificationServiceClient(this));

I'd prefer using "getApplicationContext()" instead of "this" here. Also remove the blank line above it.

::: mobile/android/base/NotificationService.java
@@ +116,5 @@
> +     */
> +    public void remove(int notificationID) {
> +        final AlertNotification notification = mAlertNotifications.get(notificationID);
> +        if (notification != null) {
> +            mAlertNotifications.remove(notification.getId());

You can replace the above three lines with:
final AlertNotification notification = mAlertNotifications.remove(notificationID);
if (notification != null) {

@@ +145,5 @@
> +        final AlertNotification notification = mAlertNotifications.get(notificationID);
> +        return notification != null && notification.isProgressStyle();
> +    }
> +
> +    private synchronized void addNotification(AlertNotification notification) {

I'm not sure you need the synchronization on these methods. I think all of the code in these methods will always be running on the same thread anyway. I'm a bit unclear on how Services work - the calls to add/update/remove are going to be dispatched using some binder magic by Android, right? Do they still run on the same thread as the caller code in NotificationServiceClient?

@@ +173,5 @@
> +            if (foregroundNotification == null) {
> +                mForegroundNotification = null;
> +                stopForeground(true);
> +            } else {
> +                tryToStartForeground(foregroundNotification);

The call to tryToStartForeground here won't have the desired effect, because mForegroundNotification will not be null.

@@ +177,5 @@
> +                tryToStartForeground(foregroundNotification);
> +            }
> +        }
> +    }
> +};

No need for this semicolon

::: mobile/android/base/NotificationServiceClient.java
@@ +22,5 @@
> +public class NotificationServiceClient {
> +    private static final String LOGTAG = "GeckoNotificationServiceClient";
> +
> +    private volatile NotificationService mService;
> +    private boolean mBinding;

I think this variable should be renamed to mBound

@@ +25,5 @@
> +    private volatile NotificationService mService;
> +    private boolean mBinding;
> +    private final Context mContext;
> +    private final LinkedList<Runnable> mTaskQueue = new LinkedList<Runnable>();
> +    private final ConcurrentHashMap<Integer, UpdateRunnable> updatesMap =

mUpdatesMap

@@ +76,5 @@
> +
> +        if (runnable == null) {
> +            runnable = new UpdateRunnable() {
> +                @Override
> +                public void run() {

Move this run method into the definition of UpdateRunnable above, and make that non-abstract. It doesn't make sense to have an abstract UpdateRunnable class with a single anonymous subclass which is the only subclass ever created of it.

@@ +105,5 @@
> +            }
> +
> +            runnable.progress = aProgress;
> +            runnable.progressMax = aProgressMax;
> +            runnable.alertText = aAlertText;

For better encapsulation I would prefer moving this comparison/overriding code into a method in UpdateRunnable itself. You can even make the member variables regular old mProgress, mProgressMax, and mAlertText private members!

synchronized boolean updateProgress(long, long, String) {
   // if the arguments are the same as this.xxx, return false
   // else set this.xxx to the arguments and return true
}

@@ +121,5 @@
> +        addToTaskQueueIfBinding(new Runnable() {
> +            @Override
> +            public void run() {
> +                mService.remove(notificationID);
> +                updatesMap.remove(notificationID);

You said you might get an update after a remove, right? In which case removing the UpdateRunnable from updatesMap here means that if an update comes in just after this remove() call but before the end of run(), a new UpdateRunnable will be created and leaked. I think, to be safe, you should also do an updatesMap.clear() in unbind().

@@ +158,5 @@
> +     * @param runnable the task to add
> +     */
> +    private synchronized void addToTaskQueueIfBinding(Runnable runnable) {
> +        if (mBinding) {
> +            addToTaskQueue(runnable);

Overall I think the code would be more readable if you inlined both addToTaskQueueIfBinding and addToTaskQueue directly into add/update/remove. I think they're small enough that pulling them out doesn't make that much sense given their particular synchronization requirements. It seems weird to me to have a function like addToTaskQueue which does a notify() without explicitly having a synchronized block (even though the code is technically correct) and it also seems weird to me that addToTaskQueueIfBinding is synchronized whereas addToTaskQueue is not (again, even though the code is technically correct).

@@ +174,5 @@
> +        mTaskQueue.add(runnable);
> +        notify();
> +    }
> +
> +    private final ServiceConnection mConnection = new ServiceConnection() {

Turn this into a regular inner class declaration and move the member variable up to the top of the outer class. i.e.:

private final NotificationServiceConnection mConnection = new NotificationServiceConnection();

...

class NotificationServiceConnection implements ServiceConnection {
   ...
}

That way this isn't some anonymous inner class that's hard to find in crash stacks.

@@ +183,5 @@
> +            mService = binder.getService();
> +
> +            final Thread t = new Thread() {
> +                @Override
> +                public void run() {

Even better, make it

class NotificationServiceConnection implements ServiceConnection, Runnable {
   ...
}

and then you can do new Thread(this).start(), moving the run method to be a two levels less indented.

@@ +224,5 @@
> +            // unexpectedly disconnected -- that is, its process crashed.
> +            // Because it is running in our same process, we should never
> +            // see this happen, and the correctness of this class relies on
> +            // this never happening.
> +            Log.e(LOGTAG, "Notification service disconnected");

I would also add a "new Exception()" as a third argument to Log.e, so we get a full stack of where this is getting called from.

Comment 21

[Brian Nicholson (:bnicholson)]

Attachment: Use foreground notification service for downloads, v2

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #20)
> Comment on attachment 723250 [details] [diff] [review]
> Use foreground notification service for downloads
> 
> Review of attachment 723250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ -2015,5 @@
> > -        // Tell Gecko to shutting down; we'll end up calling System.exit()
> > -        // in onXreExit.
> > -        if (isFinishing())
> > -            GeckoAppShell.sendEventToGecko(GeckoEvent.createShutdownEvent());
> > -
> 
> Hmm.. I'm not sure that removing this is safe. It seems like it does some
> important things. You may be right that it doesn't get called when Fennec is
> swipe-killed but then that seems like something we should fix.

The problem is that we're trying to force a simple open/close desktop application model onto Android, but we simply have no reliable way to know when we're closing. I think the way to fix this would be to not rely on executing a shutdown event at all, but to instead make sure that all necessary data is saved when we receive onPause() (which would likely overlap the part of shutdown logic that persists data, but not the part that finishes Gecko). Several critical I/O components - session restore, disk cache, and prefs - already do this. I don't know if there are other essential components that need to flush to disk as well, but if there are, they aren't working now anyway.

Note that it's not just swipe-killing Fennec that doesn't call onDestroy(); OOM kills also don't execute onDestroy(). OOM kills are by far the most common way Fennec gets killed, so I'm fairly confident that we aren't missing anything critical (since Gecko is killed uncleanly, without onDestroy, every time). I do think it's a good idea to investigate this further and reevaluate the assumptions we're making about shutdown, but I'm hoping we can address this in another bug.

> 
> @@ +1268,5 @@
> >      }
> >  
> >      private static void removeNotification(int notificationID) {
> > +        if (sNotificationClient != null) {
> > +            sNotificationClient.remove(notificationID);
> 
> Why would sNotificationClient be null here, but not in say
> alertsProgressListener_OnProgress?

Oops - that was from a previous version of the patch that I forgot to take out.

> 
> @@ +145,5 @@
> > +        final AlertNotification notification = mAlertNotifications.get(notificationID);
> > +        return notification != null && notification.isProgressStyle();
> > +    }
> > +
> > +    private synchronized void addNotification(AlertNotification notification) {
> 
> I'm not sure you need the synchronization on these methods. I think all of
> the code in these methods will always be running on the same thread anyway.
> I'm a bit unclear on how Services work - the calls to add/update/remove are
> going to be dispatched using some binder magic by Android, right? Do they
> still run on the same thread as the caller code in NotificationServiceClient?

Yeah, these methods run on the same thread created in the client. I had synchronization here to make this class thread-safe from the start in case our design changed, but I guess I can remove them since we don't need it.

> 
> ::: mobile/android/base/NotificationServiceClient.java
> @@ +22,5 @@
> > +public class NotificationServiceClient {
> > +    private static final String LOGTAG = "GeckoNotificationServiceClient";
> > +
> > +    private volatile NotificationService mService;
> > +    private boolean mBinding;
> 
> I think this variable should be renamed to mBound

Yeah, I've had trouble trying to name this. I had mBound at first, but it seemed wrong since the service isn't technically bound until the onServiceConnected callback executes. But I guess mBinding isn't accurate either since the service isn't still binding once it's bound, so I'm fine with mBound if you think that makes sense.

> 
> @@ +183,5 @@
> > +            mService = binder.getService();
> > +
> > +            final Thread t = new Thread() {
> > +                @Override
> > +                public void run() {
> 
> Even better, make it
> 
> class NotificationServiceConnection implements ServiceConnection, Runnable {
>    ...
> }
> 
> and then you can do new Thread(this).start(), moving the run method to be a
> two levels less indented.

I agree that the lack of deep nesting looks better with this change, but I'm a bit torn about the design changes as it violates encapsulation: since these overridden methods are public, the callbacks onServiceConnected/onServiceDisconnected/run methods are now part of the NotificationServiceClient API.

I went with a hybrid approach where I created a named inner class like your first suggestion, and I had that also implement the Runnable. Let me know if you don't like it.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 723355 [details] [diff] [review]
Use foreground notification service for downloads, v2

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

(In reply to Brian Nicholson (:bnicholson) from comment #21)
> Note that it's not just swipe-killing Fennec that doesn't call onDestroy();
> OOM kills also don't execute onDestroy(). OOM kills are by far the most
> common way Fennec gets killed, so I'm fairly confident that we aren't
> missing anything critical (since Gecko is killed uncleanly, without
> onDestroy, every time). I do think it's a good idea to investigate this
> further and reevaluate the assumptions we're making about shutdown, but I'm
> hoping we can address this in another bug.

Ok, that's fair. Please file a follow-up bug for that.

> Yeah, I've had trouble trying to name this. I had mBound at first, but it
> seemed wrong since the service isn't technically bound until the
> onServiceConnected callback executes. But I guess mBinding isn't accurate
> either since the service isn't still binding once it's bound, so I'm fine
> with mBound if you think that makes sense.

Yeah, I prefer mBound because it's closer to the truth (i.e. there is only a small period of time when mBound is true and it's not actually bound, but a long period of time when it is bound and mBinding would be true).

> I went with a hybrid approach where I created a named inner class like your
> first suggestion, and I had that also implement the Runnable. Let me know if
> you don't like it.

What you did is actually exactly what I was suggesting :) Sorry if that was unclear. Overall this patch is much better, but r- for the processNonUriIntent thing that I think is still not fixed and also the inverted condition I mention below.

::: mobile/android/base/GeckoApp.java
@@ +1731,5 @@
>                  onSearchRequested();
>              }
>          }
>  
> +        processNonUriIntent(intent);

I think this is still busted. In this second patch you moved the other call to processNonUriIntent but not this one (at the bottom of initialize()). Just above this call to processNonUriIntent, probably outside the splinter context, there is block that goes like this:

if (ACTION_WIDGET.equals(action)) {
  ...
   addTab();
  ...
}

And then the call to processNonUriIntent(intent) does something very similar. So it seems to me that if GeckoApp is initialized with this ACTION_WIDGET intent then the addTab() will get called twice instead of once as it was previously. I think a better fix here is to rename processNonUriIntent to processAlertCallback and just have it contain the one if condition for ACTION_ALERT_CALLBACK, and put the ACTION_WIDGET case back in onNewIntent.

::: mobile/android/base/NotificationService.java
@@ +166,5 @@
> +            }
> +
> +            if (foregroundNotification == null) {
> +                mForegroundNotification = null;
> +                stopForeground(true);

If you want you could roll the above code into setForegroundNotification, so that the function reads like this:

private voi dsetForegroundNotification(AlertNotification notification) {
    mForegroundNotification = notification;
    if (notification != null) {
        startForeground(...);
    } else {
        stopForeground(...);
    }
}

and then here you could replace this if/else block with just a single call to setForegroundNotification. Feels like it would be a bit cleaner but I'll leave it up to you.

::: mobile/android/base/NotificationServiceClient.java
@@ +42,5 @@
> +    private class UpdateRunnable implements Runnable {
> +        private long mProgress;
> +        private long mProgressMax;
> +        private String mAlertText;
> +        private int mNotificationID;

You can make mNotificationID final

@@ +51,5 @@
> +
> +        public synchronized boolean updateProgress(long progress, long progressMax, String alertText) {
> +            if (progress == mProgress
> +                    && mProgressMax == progressMax
> +                    && mAlertText.equals(alertText)) {

mAlertText is initialized to null, so this could potentially throw an exception. The short-circuiting of previous && clauses is the only thing preventing that, which seems a bit brittle. Maybe use TextUtils.equals here instead which is null-safe.

@@ +116,5 @@
> +
> +        // If we've already posted an update with these values, there's no
> +        // need to do it again.
> +        if (runnable.updateProgress(aProgress, aProgressMax, aAlertText)) {
> +            return;

This condition is inverted. If it returns false you want to return, and if it returns true you want to re-post the runnable. Or you could flip the return values of updateProgress.

@@ +133,5 @@
> +     *
> +     * @see NotificationService#remove(int)
> +     */
> +    public synchronized void remove(final int notificationID) {
> +        if (mBound) {

Doing | if (!mBound) return | here would reduce the indentation of the rest of the method by 1 level. They're both equally readable to me, your call.

@@ +215,5 @@
> +                        }
> +                    }
> +                    r.run();
> +                }
> +            } catch (final InterruptedException e) {

Out of curiousity, did Eclipse tell you to make this final? I guess there's nothing wrong with it, just never seen anybody do that before.

Comment 23

[Brian Nicholson (:bnicholson)]

Attachment: Use foreground notification service for downloads, v3

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> Comment on attachment 723355 [details] [diff] [review]
> Use foreground notification service for downloads, v2
> 
> Review of attachment 723355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Brian Nicholson (:bnicholson) from comment #21)
> > Note that it's not just swipe-killing Fennec that doesn't call onDestroy();
> > OOM kills also don't execute onDestroy(). OOM kills are by far the most
> > common way Fennec gets killed, so I'm fairly confident that we aren't
> > missing anything critical (since Gecko is killed uncleanly, without
> > onDestroy, every time). I do think it's a good idea to investigate this
> > further and reevaluate the assumptions we're making about shutdown, but I'm
> > hoping we can address this in another bug.
> 
> Ok, that's fair. Please file a follow-up bug for that.

Filed bug 849901.

> > I went with a hybrid approach where I created a named inner class like your
> > first suggestion, and I had that also implement the Runnable. Let me know if
> > you don't like it.
> 
> What you did is actually exactly what I was suggesting :) Sorry if that was
> unclear.

Oh...for some reason I interpreted your comment as having the outer class implement the interfaces. Glad we're on the same page then!

> ::: mobile/android/base/GeckoApp.java
> @@ +1731,5 @@
> >                  onSearchRequested();
> >              }
> >          }
> >  
> > +        processNonUriIntent(intent);
> 
> I think this is still busted. In this second patch you moved the other call
> to processNonUriIntent but not this one (at the bottom of initialize()).
> Just above this call to processNonUriIntent, probably outside the splinter
> context, there is block that goes like this:
> 
> if (ACTION_WIDGET.equals(action)) {
>   ...
>    addTab();
>   ...
> }
> 
> And then the call to processNonUriIntent(intent) does something very
> similar. So it seems to me that if GeckoApp is initialized with this
> ACTION_WIDGET intent then the addTab() will get called twice instead of once
> as it was previously. I think a better fix here is to rename
> processNonUriIntent to processAlertCallback and just have it contain the one
> if condition for ACTION_ALERT_CALLBACK, and put the ACTION_WIDGET case back
> in onNewIntent.

Oops, I didn't even notice that ACTION_WIDGET part in initialize. I thought you were referring to the fact that that I called processNonUriIntent() before the setIntent()/return in onNewIntent(), which would have fired the same code twice if we called both onNewIntent() and initialize() with the same intent (which happens when launching via an intent after Android has OOM killed us). So that bug was fixed in the last patch, and the one you were talking about should be fixed in this one.

> @@ +116,5 @@
> > +
> > +        // If we've already posted an update with these values, there's no
> > +        // need to do it again.
> > +        if (runnable.updateProgress(aProgress, aProgressMax, aAlertText)) {
> > +            return;
> 
> This condition is inverted. If it returns false you want to return, and if
> it returns true you want to re-post the runnable. Or you could flip the
> return values of updateProgress.

Ack, thanks.

> @@ +215,5 @@
> > +                        }
> > +                    }
> > +                    r.run();
> > +                }
> > +            } catch (final InterruptedException e) {
> 
> Out of curiousity, did Eclipse tell you to make this final? I guess there's
> nothing wrong with it, just never seen anybody do that before.

Eclipse has a cleanup tool that that you tell to use the "final" modifier where possible; you can set it for private fields, parameters, and local variables. I had it set to private fields and local variables, and apparently the exception in the catch block counts as a local variable. I agree that it seems weird...too bad there isn't more fine-grained control for that.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 723553 [details] [diff] [review]
Use foreground notification service for downloads, v3

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

Looks good, thanks!

Comment 25

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80d1a17de628

Comment 26

[Brian Nicholson (:bnicholson)]

Comment on attachment 723553 [details] [diff] [review]
Use foreground notification service for downloads, v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: downloads can get stuck in notification bar if Fennec is killed while in the background, which is especially bad if the user was doing a private browsing download
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): Medium - this patch affects Fennec's lifetime when downloads are active, and it also removes shutdown code that was usually not called (which should be safe, but could still have unforeseen consequences).
String or UUID changes made by this patch: none

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Pulling in Axel - we're wondering if it would be lower risk - for Beta only - to obfuscate the downloaded file name in the notification bar with just the word 'download' (which should already have l10n available) and that will give us time to properly bake/test the true fix. Our concern is the risk of breaking anything with Fennec shutdown.

Comment 28

[Axel Hecht [:Pike]]

Sorry, I don't understand the context of this question. I don't see any l10n impact in the patch landed, nor in the approval request?

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Axel Hecht [:Pike] from comment #28)
> Sorry, I don't understand the context of this question. I don't see any l10n
> impact in the patch landed, nor in the approval request?

Yes, that's because we're asking about another option (no patch yet) that would be a temporary lower risk fix for Beta only and want to get your input before putting together that patch.  Would using the string 'download' as the file name for private file transfers be a string that is already localized and thus be usable for 6 weeks on FF20 until this fix has had time to bake more on FF21?

Comment 30

[Axel Hecht [:Pike]]

Doesn't look like it, and it's also a word that's apparently hard to re-use as it's used both as verb and noun.

More constraints: the answer would differ between android and gecko, in theory, if it wasn't "no" for both.

Also, if it's really just a file name only showing up for private browsing, I'd not be overly concerned about hardcoding it in English.

Comment 31

[Axel Hecht [:Pike]]

PS: Based on a transvision query, http://transvision.mozfr.org/?sourcelocale=en-US&locale=de&repo=beta&search_type=strings&perfect_match=perfect_match&recherche=download

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Axel Hecht [:Pike] from comment #30)
> Also, if it's really just a file name only showing up for private browsing,
> I'd not be overly concerned about hardcoding it in English.

In that case, let's go ahead with a patch for this - it's only for 6 weeks and fixes the problem without putting Fennec shutdown at risk of regression. Brian can you do this today so we can approve the uplift in time for today's beta?

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80d1a17de628

Comment 34

[Brian Nicholson (:bnicholson)]

Attachment: Show all private downloads as "download" in the notification bar

Beta-only patch for showing private downloads as "download".

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: private downloads can be stuck and exposed
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: hardcodes "download" string for private downloads

Comment 35

[Brian Nicholson (:bnicholson)]

Also, I just realized that this fixes the harder problem where Fennec could be closed during a private download, but doesn't address the original STR filed when the last private context is closed (see comment 0)! The fix for that should be much simpler - I filed bug 850424.

Comment 36

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

In reply to Brian Nicholson (:bnicholson) from comment #34)
> Created attachment 724146 [details] [diff] [review]
> Show all private downloads as "download" in the notification bar
> 
> Beta-only patch for showing private downloads as "download".
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): none
> User impact if declined: private downloads can be stuck and exposed
> Testing completed (on m-c, etc.): locally
> Risk to taking this patch (and alternatives if risky): very low risk
> String or UUID changes made by this patch: hardcodes "download" string for
> private downloads

I am confused :-) !

And I need to be un-confused because I am writing the SUMO KB article about Android private browsing (Help wanted BTW, love to have folks on this bug proofread my SUMO article when I write it on March 14! If you want to help with this article, please email me or ping me on IRC in #sumo :rolandtanglao)

Questions
Is Brian's patch option 1 or option 2 from comment 5?

Regardless, I will test Brian's patch when the next FF20 beta comes out

Comment 37

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1d319fd009d8

Comment 38

[Brian Nicholson (:bnicholson)]

(In reply to Roland Tanglao :rolandtanglao from comment #36)
> I am confused :-) !
> 
> And I need to be un-confused because I am writing the SUMO KB article about
> Android private browsing (Help wanted BTW, love to have folks on this bug
> proofread my SUMO article when I write it on March 14! If you want to help
> with this article, please email me or ping me on IRC in #sumo :rolandtanglao)
> 
> Questions
> Is Brian's patch option 1 or option 2 from comment 5?

Neither - I found the foreground service option after I posted that comment. The first patch I have here (which will only be on Fx21 and greater) keeps Fennec alive in the background during downloads, so it should never be possible that we're abruptly killed and the notification gets stuck. There's also the problem where notifications get stuck after closing the last private tab, and that will be fixed by bug 850424 (which, unlike this bug, will hopefully make it to Fx20).

For this bug on Fx20, we're not fixing the problem of stuck downloads when Fennec gets killed in the background. It's definitely a UX problem, but the risks are too high since we're so close to release. Instead, we're just naming all private downloads "download" (see comment 27). So they can still get stuck, but it won't be a privacy issue since we can't see what the actual file is from the notification.

Comment 39

[bhavana bajaj [:bajaj]]

Comment on attachment 723553 [details] [diff] [review]
Use foreground notification service for downloads, v3

Approving the fwd fix on aurora & request to keep an eye out for any new regressions as the patch may have unforeseen consequences per comment# 26.

Also requesting exploratory testing and verification on this bug to QA once the patch lands on aurora.

Comment 40

[Adrian Tamas (:AdrianT)]

The notification can now be dismissed but the user can't actually cancel a download if Firefox is closed with the download still active. Filed new bug 851056. Will mark this fixed on Beta where all private downloads are displayed as "Downlaod" and Nightly as the download notification is not stuck anymore.

Verified on:
Firefox Mobile 20 beta 5 build 1/Nightly 22.0a1 2013-03-14
Samsung Galaxy Tab 2 7.0 (Android 4.1.1)

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/acbb5f8e4cfd

Comment 43

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
-build: Firefox for Android 21 beta1 build 1
-device: Samsung Galaxy Nexus
-OS: Android 4.1.1