Closed Bug 708280 Opened 8 years ago Closed 8 years ago

delay checking for update

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
Firefox 12
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: dougt, Assigned: alexp)

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch patch v.1 (obsolete) — Splinter Review
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?
Attachment #579721 - Flags: review? → review?(blassey.bugs)
Attachment #579721 - Flags: review?(blassey.bugs) → review-
> 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?
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
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #579721 - Attachment is obsolete: true
Attachment #580750 - Flags: review?(mark.finkle)
Attached patch patch v.2 (obsolete) — Splinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/c65be44ac489
Status: NEW → RESOLVED
Closed: 8 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)
Duplicate of this bug: 710744
this broke updater, backing out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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-
Attached patch Patch v3Splinter Review
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-
(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.
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/f70a43cdccf5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 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+
You need to log in before you can comment on or make changes to this bug.