708280 - delay checking for update

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Doug Turner (:dougt)]

Right now, we check for an update in the startup path.  We should get this on the other side of gecko being started up. Currently we are using a delay of 50ms.  this adjusts it to 5s which is will outside the startup time in my profile.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

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

Why do we want to do this? Don't we need to handle this in the startup path at some point?

Comment 3

[Doug Turner (:dougt)]

> Why do we want to do this? Don't we need to handle this in the startup path at some point?

we shouldn't do this before gecko starts.  we shouldn't do this in the sub 500ms startup.  this is this is something that should happen well after the user starts up.  I picked 5s, but it could really be any time.  Suggestions?

Comment 4

[Doug Turner (:dougt)]

blassey suggested: http://developer.android.com/reference/android/os/MessageQueue.html#addIdleHandler%28android.os.MessageQueue.IdleHandler%29

Nice.

Comment 5

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

I can live with the idle handler. We could also wait for "Gecko:Ready"

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 7

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 8

[Doug Turner (:dougt)]

https://hg.mozilla.org/mozilla-central/rev/c65be44ac489

Comment 9

[Gian-Carlo Pascutto [:gcp]]

I'm a bit confused. After that patch, the following commit was done, which moves the Looper.myQueue().addIdleHandler outside the mMainHandler:

https://hg.mozilla.org/mozilla-central/rev/aebdec71790e

But I'm seeing:

E/GeckoApp(10059): Exception handling message "Gecko:Ready":
E/GeckoApp(10059): java.lang.NullPointerException
E/GeckoApp(10059): 	at android.os.Looper.myQueue(Looper.java:179)
E/GeckoApp(10059): 	at org.mozilla.gecko.GeckoApp.handleMessage(GeckoApp.java:905)
E/GeckoApp(10059): 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:1502)
E/GeckoApp(10059): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
E/GeckoApp(10059): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
E/GeckoApp(10059): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:448)
E/GeckoApp(10059): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:107)

Comment 10

[Doug Turner (:dougt)]

https://hg.mozilla.org/mozilla-central/rev/aebdec71790e

Comment 12

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

this broke updater, backing out

Comment 13

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

backout pushed https://hg.mozilla.org/mozilla-central/rev/02781c0867f5

Comment 14

[Doug Turner (:dougt)]

alex, do you have any cycles to look at this?

Comment 15

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

Comment on attachment 580754 [details] [diff] [review]
patch v.2

marking r- so no one confuses this patch as something we want to land again

Comment 16

[Alex Pakhotin (:alexp)]

Attachment: Patch v3

Use a specific intent action for restart on update, which allows to do a check for update file only when it's really needed.

Comment 17

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

Comment on attachment 591524 [details] [diff] [review]
Patch v3

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

r- for that comment, but re-request review with an explanation if I'm off base here

::: mobile/android/base/GeckoApp.java
@@ +1448,5 @@
>  
> +        if (ACTION_UPDATE.equals(intent.getAction()) || args != null && args.contains("-alert update-app")) {
> +            Log.i(LOGTAG,"onCreate: Update request");
> +            checkAndLaunchUpdate();
> +        }

this should probably return, right? My assumption is we don't want to continue with start up if we're gonna update

Comment 18

[Alex Pakhotin (:alexp)]

(In reply to Brad Lassey [:blassey] from comment #17)
> > +        if (ACTION_UPDATE.equals(intent.getAction()) || args != null && args.contains("-alert update-app")) {
> > +            Log.i(LOGTAG,"onCreate: Update request");
> > +            checkAndLaunchUpdate();
> > +        }
> 
> this should probably return, right? My assumption is we don't want to
> continue with start up if we're gonna update

In a normal update situation the checkAndLaunchUpdate() function will close Fennec by calling System.exit(), but if for some reason the downloaded update APK does not exist or is not accessible, I believe we should continue running normally, rather than silently stop.

Comment 19

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

Comment on attachment 591524 [details] [diff] [review]
Patch v3

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

(In reply to Alex Pakhotin (:alexp) from comment #18)
> 
> In a normal update situation the checkAndLaunchUpdate() function will close
> Fennec by calling System.exit(), but if for some reason the downloaded
> update APK does not exist or is not accessible, I believe we should continue
> running normally, rather than silently stop.

I don't know that that is right, but I don't want to hold up this patch fixing an issue that is only tangentially related. Please file a UX bug for madhava to specify what the behavior should be if the user clicks on the update notification bug we can't find an update to apply

::: mobile/android/base/GeckoApp.java
@@ +1448,5 @@
>  
> +        if (ACTION_UPDATE.equals(intent.getAction()) || args != null && args.contains("-alert update-app")) {
> +            Log.i(LOGTAG,"onCreate: Update request");
> +            checkAndLaunchUpdate();
> +        }

this should probably return, right? My assumption is we don't want to continue with start up if we're gonna update

Comment 20

[Alex Pakhotin (:alexp)]

(In reply to Brad Lassey [:blassey] from comment #19)
> 
> I don't know that that is right, but I don't want to hold up this patch
> fixing an issue that is only tangentially related. Please file a UX bug for
> madhava to specify what the behavior should be if the user clicks on the
> update notification bug we can't find an update to apply

FWIW, there is no change in behavior compared to the current implementation. Right now if we cannot find the update file on restart, we just continue. With this change the same check is just done earlier and only when really needed.

Comment 21

[Alex Pakhotin (:alexp)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f70a43cdccf5

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f70a43cdccf5

Comment 23

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

Comment on attachment 591524 [details] [diff] [review]
Patch v3

[Triage Comment]

Comment 24

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

https://hg.mozilla.org/releases/mozilla-beta/rev/bc296e68391d