Last Comment Bug 708280 - delay checking for update
: delay checking for update
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: P2 normal (vote)
: Firefox 12
Assigned To: Alex Pakhotin (:alexp)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-07 09:27 PST by Doug Turner (:dougt)
Modified: 2012-02-06 13:04 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
patch v.1 (5.60 KB, patch)
2011-12-07 09:29 PST, Doug Turner (:dougt)
dougt: review-
Details | Diff | Review
patch v.2 (2.50 KB, patch)
2011-12-11 08:52 PST, Doug Turner (:dougt)
no flags Details | Diff | Review
patch v.2 (4.21 KB, patch)
2011-12-11 08:56 PST, Doug Turner (:dougt)
blassey.bugs: review-
Details | Diff | Review
Patch v3 (10.90 KB, patch)
2012-01-25 10:05 PST, Alex Pakhotin (:alexp)
blassey.bugs: review+
blassey.bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Doug Turner (:dougt) 2011-12-07 09:27:55 PST
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) 2011-12-07 09:29:26 PST
Created attachment 579721 [details] [diff] [review]
patch v.1
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-07 10:37:26 PST
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) 2011-12-07 11:04:02 PST
> 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 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-07 11:23:53 PST
I can live with the idle handler. We could also wait for "Gecko:Ready"
Comment 6 Doug Turner (:dougt) 2011-12-11 08:52:12 PST
Created attachment 580750 [details] [diff] [review]
patch v.2
Comment 7 Doug Turner (:dougt) 2011-12-11 08:56:47 PST
Created attachment 580754 [details] [diff] [review]
patch v.2
Comment 8 Doug Turner (:dougt) 2011-12-12 02:07:33 PST
https://hg.mozilla.org/mozilla-central/rev/c65be44ac489
Comment 9 Gian-Carlo Pascutto [:gcp] 2011-12-13 09:18:34 PST
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) 2011-12-14 01:57:22 PST
https://hg.mozilla.org/mozilla-central/rev/aebdec71790e
Comment 11 Aaron Train [:aaronmt] 2011-12-14 09:25:43 PST
*** Bug 710744 has been marked as a duplicate of this bug. ***
Comment 12 Brad Lassey [:blassey] (use needinfo?) 2011-12-14 09:31:53 PST
this broke updater, backing out
Comment 13 Brad Lassey [:blassey] (use needinfo?) 2011-12-14 11:34:31 PST
backout pushed https://hg.mozilla.org/mozilla-central/rev/02781c0867f5
Comment 14 Doug Turner (:dougt) 2011-12-22 10:25:10 PST
alex, do you have any cycles to look at this?
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2012-01-23 12:58:00 PST
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) 2012-01-25 10:05:35 PST
Created attachment 591524 [details] [diff] [review]
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?) 2012-01-25 14:42:12 PST
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) 2012-01-25 16:05:52 PST
(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?) 2012-01-25 17:55:45 PST
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) 2012-01-25 18:01:39 PST
(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 22 Ed Morley [:emorley] 2012-01-26 04:27:43 PST
https://hg.mozilla.org/mozilla-central/rev/f70a43cdccf5
Comment 23 Brad Lassey [:blassey] (use needinfo?) 2012-02-06 11:00:55 PST
Comment on attachment 591524 [details] [diff] [review]
Patch v3

[Triage Comment]
Comment 24 Brad Lassey [:blassey] (use needinfo?) 2012-02-06 13:04:51 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/bc296e68391d

Note You need to log in before you can comment on or make changes to this bug.