delay checking for update

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dougt, Assigned: alexp)

Tracking

unspecified
Firefox 12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 579721 [details] [diff] [review]
patch v.1
Assignee: nobody → doug.turner
Attachment #579721 - Flags: review?
Why do we want to do this? Don't we need to handle this in the startup path at some point?
(Reporter)

Updated

6 years ago
Attachment #579721 - Flags: review? → review?(blassey.bugs)
(Reporter)

Updated

6 years ago
Attachment #579721 - Flags: review?(blassey.bugs) → review-
(Reporter)

Comment 3

6 years ago
> 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?
(Reporter)

Comment 4

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

Nice.
I can live with the idle handler. We could also wait for "Gecko:Ready"
Summary: delay checking checking for update → delay checking for update
Priority: -- → P3
(Reporter)

Comment 6

6 years ago
Created attachment 580750 [details] [diff] [review]
patch v.2
Attachment #579721 - Attachment is obsolete: true
Attachment #580750 - Flags: review?(mark.finkle)
(Reporter)

Comment 7

6 years ago
Created attachment 580754 [details] [diff] [review]
patch v.2
Attachment #580750 - Attachment is obsolete: true
Attachment #580750 - Flags: review?(mark.finkle)
Attachment #580754 - Flags: review?(mark.finkle)
Attachment #580754 - Flags: review?(mark.finkle) → review+
(Reporter)

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/c65be44ac489
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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)
(Reporter)

Comment 10

6 years ago
https://hg.mozilla.org/mozilla-central/rev/aebdec71790e

Updated

6 years ago
Duplicate of this bug: 710744
this broke updater, backing out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
backout pushed https://hg.mozilla.org/mozilla-central/rev/02781c0867f5
(Reporter)

Comment 14

6 years ago
alex, do you have any cycles to look at this?
Assignee: doug.turner → alexp
tracking-fennec: --- → 11+
Priority: P3 → P2
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
Attachment #580754 - Flags: review+ → review-
(Assignee)

Comment 16

6 years ago
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.
Attachment #580754 - Attachment is obsolete: true
Attachment #591524 - Flags: review?(blassey.bugs)
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
Attachment #591524 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 18

6 years ago
(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.
(Assignee)

Updated

6 years ago
Attachment #591524 - Flags: review- → review?(blassey.bugs)
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
Attachment #591524 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 20

6 years ago
(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.
(Assignee)

Comment 21

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f70a43cdccf5
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/f70a43cdccf5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Comment on attachment 591524 [details] [diff] [review]
Patch v3

[Triage Comment]
Attachment #591524 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/bc296e68391d
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.