604491 - Tapping update notification while Fennec isn't running just starts fennec and doesn't offer to download

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dave Townsend [:mossop]]

Apparently it is possible for Fennec to detect an update and show the update available notification and then close down in the background leaving the notification around. The notification says to tap it to start downloading but when you do Fennec gets launched and nothing else happens

Comment 1

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

Mossop, do you still see this?

Comment 2

[Alex Pakhotin (:alexp)]

This is actually how notifications are currently implemented. They may be shown in the status bar regardless of Fennec being running. But if Fennec closed since it displayed the notification, the listener provided to the nsIAlertsService.showAlertNotification obviously gets lost, so when you click on the notification, Fennec activity starts, but there is no listener registered.
I don't think we can restore the listener on restart, so might want just to hide the notification when Fennec closes, and have it to re-check the update status on restart (which actually should happen automatically after the specified interval).

Comment 3

[Alex Pakhotin (:alexp)]

(In reply to comment #2)
> ...might want just to hide the notification when Fennec closes...

I meant when it gets closed completely by the system, if we can detect that at all.

Comment 4

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

(In reply to comment #2)

> I don't think we can restore the listener on restart, so might want just to
> hide the notification when Fennec closes, and have it to re-check the update
> status on restart (which actually should happen automatically after the
> specified interval).

This sounds like a good plan to me.

We can use the "xpcom-shutdown" or "quit-application" notifications

Comment 5

[Dave Townsend [:mossop]]

Can we have it flag the update at the next restart or something in that case?

Comment 6

[Madhava Enros [:madhava]]

An assumption: I don't think people on android have any real sense of whether a given app is open or not.  Partly as a result of this, I suspect that people think of notifications as being more a system thing than an app thing.

So, in my perfect world, if there's an update there, you could just make it do it's thing, regardless of whether Firefox is open or not.  I guess this will be situation for our updates once we're in the Market, correct?

Definitely, though, if it's going to result in a dead-end, better to not show it.

If you've downloaded it, so the notification you have is that you're ready to install, can that go ahead even if Firefox is closed?

Comment 7

[Alex Pakhotin (:alexp)]

It looks like we cannot do anything on shut down. Most of the time Fennec gets killed without onDestroy() being called.

It is possible to do something special when Fennec restarts on the notification click, but the problem here is that we don't want to do a lot of custom stuff in Java layer. Normally notifications just call the listener provided, so the notification handler code is pretty generic - it doesn't know if the notification was about update, or file download, or add-on installation. We may think about providing some special command-line parameters, which would let Fennec know on loading that it was started from the notification, and needs to do something about it.

Comment 8

[Alex Pakhotin (:alexp)]

(In reply to comment #6)
> So, in my perfect world, if there's an update there, you could just make it do
> it's thing, regardless of whether Firefox is open or not.  I guess this will be
> situation for our updates once we're in the Market, correct?

Yes, with Market-based update it will work like this. But updates for nightly/beta builds are performed by our own code, so the app needs to run at that moment.

> If you've downloaded it, so the notification you have is that you're ready to
> install, can that go ahead even if Firefox is closed?

Yes, if the update was downloaded and is ready to install, the installation will happen on the next start of Fennec. So the issue is just with the first notification about the update being ready for download.

Actually regular file downloads have the same issue - if Fennec was shut down, clicking on the notification will restart it, but will not open the download manager. It's less noticeable though, as when something is downloading, the browser is usually active at that moment.

Comment 9

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

Can we use the same solution as bug 605173?

Comment 10

[Alex Pakhotin (:alexp)]

(In reply to comment #9)
> Can we use the same solution as bug 605173?

I'll just copy that comment here where it belongs:
==================================================
Mark Finkle (:mfinkle) 2010-11-04 10:53:39 PDT

Can we get the Update notification to launch Fennec with a commandline param? 

fennec -alert <cookie>

so:
fennec -alert update

Then the commandline handler can run some code
==================================================

Yes, I think we can do this, have a generic alert command line parameter handler, which will receive the alert name+cookie, and let chrome decide what to do with that.

Comment 12

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

general consensus is to go with the command line flag approach. Let's get a patch together for that and see how it works.

Comment 13

[Alex Pakhotin (:alexp)]

Attachment: [WIP] Fix

Pass alert name and cookie as an -alert command line parameter.

Comment 14

[Alex Pakhotin (:alexp)]

Attachment: [WIP] Fix (mobile)

Check "-alert" command line flag in command line handler.
This is just a draft, which needs to be reworked. The issue with this approach is that update cannot proceed when called from there - it seems like this command line processing is happening at so early stages that some important modules are not yet initialized.
An attempt to do an update at that moment fails with an error "transfer error: Network is offline (go online), code: 2152398918". We probably need to postpone the actual handling of this command line parameter to some later stage. This needs further investigation.

Comment 15

[Alex Pakhotin (:alexp)]

Attachment: [WIP] Fix (mobile)

This command line handler now works with updates.
The only issue to be ironed out is with other notifications, like file downloads and addons. With the current implementation the download or addon panel shows empty, and needs to be manually refreshed to show the content.

Comment 16

[Alex Pakhotin (:alexp)]

Attachment: Fix

Updated to the latest code.

Comment 17

[Alex Pakhotin (:alexp)]

Attachment: Fix (mobile)

This version works with update, download and add-on notifications.

There was a concern about "UIReadyDelayed" event listener being in race condition with the browser panels initialization, but it seems to be fine. The listener in BrowserCLH is registered after the one in BrowserUI, so it's called after the downloads/addons panels have been initialized.

Comment 18

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

Comment on attachment 499468 [details] [diff] [review]
Fix (mobile)

You'll need to remove this:
http://mxr.mozilla.org/mobile-browser/source/components/BrowserCLH.js#209

I added it in the hopes that it would speed up Ts, but it didn't. Now, it will return early and skip your new code.

>     // First, get a browserDOMWindow object
>     while (!win.browserDOMWindow)
>       Services.tm.currentThread.processNextEvent(true);
> 
>     // Open any URIs into new tabs
>     for (let i = 0; i < uris.length; i++)
>       win.browserDOMWindow.openURI(uris[i], null, Ci.nsIBrowserDOMWindow.OPEN_NEWTAB,
>                                    Ci.nsIBrowserDOMWindow.OPEN_EXTERNAL);

>+      } else if (alertFlag == "addons") {
>+        showPanelWhenReady(win, "addons-container");
>+      }
>+    }
>+
>   },

Don't add the blank line

Comment 19

[Alex Pakhotin (:alexp)]

Attachment: Fix

Updated to the latest code. r=blassey

Comment 20

[Alex Pakhotin (:alexp)]

Attachment: Fix (mobile)

Fixed review comments. r=mfinkle

Comment 21

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/0c7acbb43cef
http://hg.mozilla.org/mozilla-central/rev/8f1c39add00f

Comment 22

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Android; Linux armv7l; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre Fennec/4.0b4pre ID:20110109042046

Steps I used to verify:
- go to about:firefox, tap on "Check for Updates"
- Update available notification appears in Android status bar
- Close Fennec by tapping on the close button of the last open tab
- Open Android status bar and tap on the Fennec update available entry

Expected result:
- Fennec starts up and shows the Update prompt

The expected result is what happens in the build that I used.

Comment 23

[Paul [pwd]]

I'm seeing this again on Fennec Native.