Closed Bug 792992 Opened 7 years ago Closed 5 years ago

Can't override query URL for update service

Categories

(Firefox for Android :: General, defect)

x86
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox36 --- wontfix
firefox37 --- fixed
firefox38 --- fixed
fennec + ---

People

(Reporter: nthomas, Assigned: esawin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Updater])

Attachments

(4 files, 5 obsolete files)

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 ?
Whiteboard: [Updater]
[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.
tracking-fennec: --- → ?
Assignee: nobody → esawin
tracking-fennec: ? → +
Attached patch updater-prefs (obsolete) — Splinter Review
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
Attachment #8546828 - Flags: feedback?(snorp)
Status: NEW → ASSIGNED
Comment on attachment 8546828 [details] [diff] [review]
updater-prefs

Moving the refactoring work to bug 1122623.
Attachment #8546828 - Flags: feedback?(snorp)
Blocks: 1019724
Attached patch updater-refactor (obsolete) — Splinter Review
Attachment #8546828 - Attachment is obsolete: true
Attachment #8553851 - Flags: review?(snorp)
Attached patch updater-prefs (obsolete) — Splinter Review
This one is based on the refactoring patch and adds prefs to control the update URL and to enable/disable the Android updater.
Attachment #8553852 - Flags: review?(snorp)
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.
Attachment #8553852 - Flags: review?(snorp) → review-
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?
Attachment #8553851 - Flags: review?(snorp) → review+
Attached patch updater-refactorSplinter Review
Fixed a string comparison and removed TODO comments.
Attachment #8553851 - Attachment is obsolete: true
Attachment #8555826 - Flags: review+
Attached patch updater-url (obsolete) — Splinter Review
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.
Attachment #8553852 - Attachment is obsolete: true
Attachment #8555833 - Flags: review?(snorp)
Attachment #8555833 - Flags: review?(snorp) → review+
Attached patch updater-urlSplinter Review
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
Attachment #8555833 - Attachment is obsolete: true
Attachment #8558130 - Flags: review?(snorp)
Correction: the new pref's name is "app.update.url.android".
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.
(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 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.
Attachment #8558130 - Flags: review?(snorp) → review+
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.
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.
https://hg.mozilla.org/mozilla-central/rev/46ccf9bfde3f
https://hg.mozilla.org/mozilla-central/rev/da83b90c888e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Attached patch updater-delayed-start (obsolete) — Splinter Review
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.
Attachment #8560680 - Flags: review?(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 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?
Attachment #8560680 - Flags: review?(rnewman) → feedback+
(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.
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?
Flags: needinfo?(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.
Flags: needinfo?(rnewman)
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
Attachment #8560680 - Attachment is obsolete: true
Attachment #8561558 - Flags: review?(rnewman)
Attached patch updater-url-uriSplinter Review
URL -> URI.
Attachment #8561559 - Flags: review?(rnewman)
Attachment #8561558 - Flags: review?(rnewman) → review+
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
Attachment #8561559 - Flags: review?(rnewman) → review+
Are there any plans to uplift this to beta given it needs to land there so it can be tested? (bug 1117831)
(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.
Okay great, can you please request the uplift, thanks :-)
Flags: needinfo?(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
Flags: needinfo?(esawin)
Attachment #8561559 - Flags: approval-mozilla-beta?
Attachment #8561558 - Flags: approval-mozilla-beta?
Attachment #8558130 - Flags: approval-mozilla-beta?
Attachment #8555826 - Flags: approval-mozilla-beta?
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+
Attachment #8555826 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8558130 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8561558 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8561559 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1158131
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.
Flags: needinfo?(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.
Flags: needinfo?(esawin)
You need to log in before you can comment on or make changes to this bug.