792992 - Can't override query URL for update service

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Thomas [:nthomas] (UTC+12)]

The new update service handles the query url differently to toolkit updater code, mostly hardcoding it in 
 http://hg.mozilla.org/mozilla-central/file/default/mobile/android/base/UpdateServiceHelper.java.in#l49
with a bunch of build-time resolution for values like CHANNEL. This makes it very hard to test new updates, where QA would want to change the channel to something like betatest, or in general if we want to talk to a different host or modify other values. 

Could we add back in support for app.update.url.override or equivalent ?

Comment 1

[Nick Thomas [:nthomas] (UTC+12)]

[Tracking Requested - why for this release]:

Bug 1019724 has added single-locale builds for beta/release, ie for end users. Without a fix here we don't have a way of testing the updates before shipping them.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: updater-prefs

Sorry about the big patch, I will split it into smaller chunks, clean up and refactor the updater code a bit before review. I'm just looking for some early feedback before I proceed.

The refactoring goal is to move the public API for the updater into UpdateServiceHelper (might need to rename it) to avoid firing updater intents in other parts of the code, which makes it difficult to keep state (isEnabled) outside of the updater service.

Additionally, I'm adding a way to handle more prefs to customize updater behavior at runtime.

TODO:
* Implement unregisterForUpdates
* Share prefs state across processes (SharedPreference allows is but it's discouraged)
* Figure out why we don't get any pref changes
* Make sure the updater is disabled for tests

Comment 3

[Eugen Sawin [:esawin]]

Comment on attachment 8546828 [details] [diff] [review]
updater-prefs

Moving the refactoring work to bug 1122623.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: updater-refactor

Comment 5

[Eugen Sawin [:esawin]]

Attachment: updater-prefs

This one is based on the refactoring patch and adds prefs to control the update URL and to enable/disable the Android updater.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8553852 [details] [diff] [review]
updater-prefs

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

::: mobile/android/base/updater/UpdateServiceHelper.java
@@ +84,5 @@
>          }
>      }
>  
> +    @RobocopTarget
> +    public static void setEnabled(final boolean enabled) {

This isn't going to be good enough. You need to set an enabled state in the service itself, otherwise the scheduled update check will run.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8553851 [details] [diff] [review]
updater-refactor

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

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +1035,5 @@
>      }};
>  
>      @Override
>      public boolean onPreferenceChange(Preference preference, Object newValue) {
> +        // TODO: this is never triggered?

Hmmm. Why not?

Comment 9

[Eugen Sawin [:esawin]]

Attachment: updater-refactor

Fixed a string comparison and removed TODO comments.

Comment 10

[Eugen Sawin [:esawin]]

Attachment: updater-url

Removed "app.update.android.enabled" pref, should be handled in a follow-up bug.
This patch adds the "app.update.url" pref for the Android update service.

Comment 11

[Eugen Sawin [:esawin]]

Try results for attachment #8555826 [details] [diff] [review]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b477d69243ad

Comment 12

[Eugen Sawin [:esawin]]

Attachment: updater-url

There is an issue with calling PrefsHelper.getPrefs on prefs with valid URLs, see following try results for a minimal example: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03dbf7e25257

To fix that, we reset the update URL now for tests, which a good thing to do anyways to avoid downloading updates during tests. We could drop UpdateServiceHelper.setEnabled in a follow-up bug, but currently that's just an additional way to disable the updater.

I've also changed the pref name from "app.update.url" to "app.update.android.url" to avoid name collisions with other platforms.

Snorp, can you please have a quick look at it again, since a few things have changed.

Try (Android): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d640adce0748
Try (all): https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f6b6ca0bdcd

Comment 13

[Eugen Sawin [:esawin]]

Correction: the new pref's name is "app.update.url.android".

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8558130 [details] [diff] [review]
updater-url

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

I've only skimmed this patch, so you might have already addressed this, but --

Fetching a pref from Gecko requires Gecko to init successfully. This has two flaws:

1. If you're waiting, you might be waiting a long time.
2. If Gecko can't launch (e.g., wrong architecture), you'll never get the value.

So if you're having SharedPreferences be authoritative, and writing overrides into it when the Gecko pref changes (i.e., install a pref listener in browser.js), then great. If you're asking for the URL when you try to fetch an update: DANGER, WILL ROBINSON.

Testing: add a JS error in browser.js, Clear Data, and verify that the updater still works.

Comment 15

[Eugen Sawin [:esawin]]

(In reply to Richard Newman [:rnewman] from comment #14)
> Comment on attachment 8558130 [details] [diff] [review]
> updater-url
> 
> Review of attachment 8558130 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've only skimmed this patch, so you might have already addressed this, but
> --
> 
> Fetching a pref from Gecko requires Gecko to init successfully. This has two
> flaws:
> 
> 1. If you're waiting, you might be waiting a long time.
> 2. If Gecko can't launch (e.g., wrong architecture), you'll never get the
> value.
> 
> So if you're having SharedPreferences be authoritative, and writing
> overrides into it when the Gecko pref changes (i.e., install a pref listener
> in browser.js), then great. If you're asking for the URL when you try to
> fetch an update: DANGER, WILL ROBINSON.
> 
> Testing: add a JS error in browser.js, Clear Data, and verify that the
> updater still works.

Thanks for the feedback!

The patches don't change the way we handle the prefs for the update service. The refactoring just wraps the intent mechanics to provide a simpler interface and the new URL pref is handled the same way the "app.update.autodownload" pref is handled, so there is no change in mechanics here.

GeckoApp registers for updates in its initialize method, this is the time when we fetch the prefs. We also listen to pref changes in GeckoPreferences.onPreferenceChange.

Please let me know if you think there is an issue with this.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8558130 [details] [diff] [review]
updater-url

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

Looks good with the nit fixed

::: mobile/android/base/updater/UpdateServiceHelper.java
@@ +86,5 @@
>      public static void setEnabled(final boolean enabled) {
>          isEnabled = enabled;
>      }
>  
> +    public static URL getUpdateUrl(Context context, String updateUrl, boolean force) {

It's not just really a getter anymore. How about...expandUpdateUrl()? inflateUpdateUrl()? I dunno. Names are hard.

Comment 17

[Eugen Sawin [:esawin]]

https://hg.mozilla.org/integration/fx-team/rev/46ccf9bfde3f
https://hg.mozilla.org/integration/fx-team/rev/da83b90c888e

Comment 18

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

Eugen - I think this may have caused an autophone regression in the Nexus 4, maybe smaller regressions in the other devices.

http://phonedash.mozilla.org/#/org.mozilla.fennec/totalthrobber/local-blank/norejected/2015-01-26/2015-02-03/notcached/noerrorbars/standarderror/notry

I did not take a thorough look at the patch, but I do see uses of | editor.commit() | which is bad for the main thread. I'm not sure what, if any, code paths would be on the main thread, but you should be able to change all occurrences of | editor.commit() | to | editor.apply() | which is async.

Comment 19

[Eugen Sawin [:esawin]]

The update service runs in its own process and handles requests on its worker thread, so I don't think that the SharedPreference interactions could cause a regression. Since we never check for the success state of the commit call, we should be safe to switch to apply nonetheless.

We do call UpdateServiceHelper.registerForUpdates on the main thread, I wonder if the extended pref fetching is causing the regression for some reason.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46ccf9bfde3f
https://hg.mozilla.org/mozilla-central/rev/da83b90c888e

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

I believe this caused a talos regression (which will be battling a win from bug 1106935):
http://alertmanager.allizom.org:8080/alerts.html?rev=a9cc1659d77f&showAll=1&testIndex=0&platIndex=0

Comment 22

[Eugen Sawin [:esawin]]

Attachment: updater-delayed-start

It's still unclear why the prefs fetching is suddenly causing such a regression, especially on older devices, however, we can simply delay the update service startup to prevent it from interfering with Gecko startup.

Autophone results show an improvement:
Regression: (changeset da83b90c888e) http://phonedash.mozilla.org/#/org.mozilla.fennec/totalthrobber/local-blank/norejected/2015-01-28/2015-02-04/notcached/noerrorbars/standarderror/notry
Delayed start: (changeset 4a18d6ff525f) http://phonedash.mozilla.org/#/org.mozilla.fennec/totalthrobber/local-blank/norejected/2015-01-30/2015-02-06/notcached/noerrorbars/standarderror/try

Also the issue from comment 12 is worrying me, If having a valid URL in a pref would somehow automatically trigger a connection, then this could explain the regression.

Comment 23

[Richard Newman [:rnewman]]

(In reply to Eugen Sawin [:esawin] from comment #22)

> Also the issue from comment 12 is worrying me, If having a valid URL in a
> pref would somehow automatically trigger a connection, then this could
> explain the regression.

I just noticed that you're using `URL` in parts of this code.

Don't use java.net.URL unless you are immediately going to make a network request using URLConnection. Use java.net.URI instead.

Its standard behaviors like .equals and .hashCode involve DNS lookups. That means simple things like adding them to a HashTable will result in blocking network calls.

Furthermore, just *constructing* one means reading system properties and creating stream handlers, as well as parsing the URL. Take a look at this horror show:

http://osxr.org/android/source/libcore/luni/src/main/java/java/net/URL.java#0385

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 8560680 [details] [diff] [review]
updater-delayed-start

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

::: mobile/android/base/GeckoApp.java
@@ +1585,5 @@
> +            @Override
> +            public void run() {
> +                UpdateServiceHelper.registerForUpdates(GeckoApp.this);
> +            }
> +        }, 2000);

Lift out this magic constant. But why not just do this in delayedStartup and save a runnable?

Comment 25

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

(In reply to Richard Newman [:rnewman] from comment #24)

> Lift out this magic constant. But why not just do this in delayedStartup and
> save a runnable?

DelayedStartup depends on Gecko starting up without a problem. One nice aspect of our Updater is that we can "update" our way out of Gecko startup failures. Making the Updater depend on Gecko means people would need to manually re-install Fennec in situations where Gecko is not starting up.

Magic numbers delays are never pretty, but maybe we should take this one.

Comment 26

[Eugen Sawin [:esawin]]

The URL vs. URI argument sounds like a reasonable explanation for the sudden regression, so I've postponed the URL construction and replaced it with URI in all intermediate handling, but the results don't show an improvement (without the delayed start): [024a52d273e3] http://phonedash.mozilla.org/#/org.mozilla.fennec/totalthrobber/local-blank/norejected/2015-02-03/2015-02-09/notcached/noerrorbars/standarderror/try 

I can lift out the magic constant (into a named magic constant) and by mfinkle's reasoning we should probably keep it in GeckoApp's initialize, does that sound reasonable?

Comment 27

[Richard Newman [:rnewman]]

Yup. Good call, mfinkle.

Eugen:

If you really want to know what's happening here, take a Java profile -- add a startMethodTracing call in GeckoApplication.onCreate, and a stopMethodTracing call in, say, delayedStartup.

http://developer.android.com/tools/debugging/debugging-tracing.html#creatingtracefiles

If your hypothesis is correct (and similar to the Stumbler bug), then we're stalling a thread somewhere to start the service.

If your measurements suggest that queuing the runnable itself is expensive enough to make a difference, then consider adding this to the existing background service runnable, right before we call broadcastHealthReportUploadPref (~line 1600). That should be equivalent to your current code, but a little sooner, and saves a new runnable.

(I don't know why there's a useless return in that runnable...)


Also, I recommend landing the URL->URI change anyway. Passing URLs around is like juggling with a loaded gun, and at some point someone will hit the trigger.

Comment 28

[Eugen Sawin [:esawin]]

Attachment: updater-delayed-start

Reusing existing runnable to delay update service start.

[2dd8e6a23dad] http://phonedash.mozilla.org/#/org.mozilla.fennec/totalthrobber/local-blank/norejected/2015-02-03/2015-02-09/notcached/noerrorbars/standarderror/try

Comment 29

[Eugen Sawin [:esawin]]

Attachment: updater-url-uri

URL -> URI.

Comment 30

[Eugen Sawin [:esawin]]

Try results for the two patches above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=921da5c31ce9

Comment 31

[Richard Newman [:rnewman]]

Comment on attachment 8561559 [details] [diff] [review]
updater-url-uri

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

r+ with typos/naming.

::: mobile/android/base/updater/UpdateService.java
@@ +509,5 @@
> +        URL url = null;
> +        try {
> +            url = info.uri.toURL();
> +        } catch (java.net.MalformedURLException e) {
> +            Log.e(LOGTAG, "failed to :ead URL: ", e);

Typo

@@ +717,5 @@
>          editor.putInt(KEY_AUTODOWNLOAD_POLICY, policy.value);
>          editor.commit();
>      }
>  
> +    private URI getUpdateUri(boolean force) {

s/Uri/URI/g

(There's an Android class named "Uri", and this ain't it.)

@@ +718,5 @@
>          editor.commit();
>      }
>  
> +    private URI getUpdateUri(boolean force) {
> +        return UpdateServiceHelper.expandUpdateUri(this, mPrefs.getString(KEY_UPDATE_URL, null), force);

Same

Comment 32

[Eugen Sawin [:esawin]]

https://hg.mozilla.org/integration/fx-team/rev/9eecf3e8bf3a
https://hg.mozilla.org/integration/fx-team/rev/898a12e85098

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9eecf3e8bf3a
https://hg.mozilla.org/mozilla-central/rev/898a12e85098

Comment 34

[Kim Moir [:kmoir] ET]

Are there any plans to uplift this to beta given it needs to land there so it can be tested? (bug 1117831)

Comment 35

[Eugen Sawin [:esawin]]

(In reply to Kim Moir [:kmoir] from comment #34)
> Are there any plans to uplift this to beta given it needs to land there so
> it can be tested? (bug 1117831)

This bug wasn't tracking a particular version, so I didn't request for uplifting, but it should be safe to do so.

Comment 36

[Kim Moir [:kmoir] ET]

Okay great, can you please request the uplift, thanks :-)

Comment 37

[Eugen Sawin [:esawin]]

Comment on attachment 8561559 [details] [diff] [review]
updater-url-uri

Approval Request Comment
[Feature/regressing bug #]: 786380 
[User impact if declined]: required to verify single locale updates (bug 1117831)
[Describe test coverage new/current, TreeHerder]: has been on Nightly for weeks
[Risks and why]: low, only affects updates on Android, mostly refactoring work
[String/UUID change made/needed]: none, only a new Android-only pref added

Comment 38

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8555826 [details] [diff] [review]
updater-refactor

These changes have no impact on app store updates. As such, the changes shouldn't impact the release. Beta+

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f9f0120c1adf
https://hg.mozilla.org/releases/mozilla-beta/rev/ae511f0dda0f
https://hg.mozilla.org/releases/mozilla-beta/rev/7113cd46019c
https://hg.mozilla.org/releases/mozilla-beta/rev/bd0696c04755

Comment 40

[Nick Thomas [:nthomas] (UTC+12)]

Eugen, could you please provide a list of steps for changing the channel, for QA use ? eg which pref(s) should be modified, which processes/services need to be stopped, etc.

Comment 41

[Eugen Sawin [:esawin]]

In short:
1. Set |app.update.url.android|.
2. Restart Fennec.
 
The |app.update.url.android| pref sets the URL for the update service, which is retrieved on initialization.

Since we don't export the update service (org.mozilla.gecko.updater.UpdateService), you need to restart Fennec after modifying the pref, which then (re)starts the update service (delayed) with the new pref value.