1178760 - Dynamic ua updates not being applied from server (ua-override.json)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

STR:
* Go to youtube.com
* Click on random video
* Click play

Expected Result: Video plays in native YouTube app (That's the behavior using Firefox Beta on the same device).

Actual Result: Toast with message "Cannot open link" appears.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

This could be related to bug 1176018 and adding CATEGORY_BROWSABLE to Intent objects.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Sebastian Kaspari from comment #1)
> This could be related to bug 1176018 and adding CATEGORY_BROWSABLE to Intent
> objects.

This assumption is wrong. However I can see this in the log:

> GeckoIntentHelper  W  Unable to handle URI: Intent { act=android.intent.action.VIEW dat=rtsp://r2--
>                       -sn-4g57kuek.googlevideo.com/ClULENy73wIaTAnAsbs-LEdACxMYDSANFC0mqZJVMOCoAUIJ
>                       bXYtZ29vZ2xlSARSBXdhdGNoYKamzt_1zaHJVYIBBWh0bWw1igELd1BTXy1jTUdBd1EM/3D1EAB6A
>                       D2F02A8725244BB5E99E9635115A7896.35558C143C38E3FDDD188B007149B4ED91E75E8B/yt5
>                       /1/video.3gp }

Logged from IntentHelper.openNoHandler();

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

@Mike: This bug sounds a lot like what you described in bug 1174784. But here it's not about playing the video inside the browser but jumping to the YouTube app. Your observations should be fixed with the UA override for YouTube, right?

Comment 4

[Mike Taylor [:miketaylr]]

Sebastian, yeah something is not right here. On Nightly, my UA override is in place and the video plays as expected. On Aurora however, the override is not applied.

Looking...

Comment 5

[Mike Taylor [:miketaylr]]

Hi Richard, can you check on the dynamic UA server logs? Are files for Fennec being requested? I just cleared all my data in Nightly and it appears the overrides aren't working now. Which is confusing.

Comment 6

[Mike Taylor [:miketaylr]]

If it's OK, I'm going to steal this bug to investigate why the dynamic UA overrides aren't working. Bug 1179300 covers adding a local override to fix Youtube (and disabling the override updates).

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

I think the problem is due to the bundled ua-update.json being inside omnijar. Before an update happens, we have to fallback on reading the bundled ua-update.json. However, UserAgentUpdates.jsm only supports reading from a file and not from omnijar, so it fails to load ua-update.json, causing UA overrides to not work. I was able to confirm that, once an update happens and we save the updated ua-update.json to disk, UA overrides starts to work again.

So I think we can either make UserAgentUpdates.jsm work with omnijar, or extract ua-update.json to disk on profile creation.

Comment 8

[:Atoll]

There's not much for me to do server-side, so I'm removing myself. Loop me in if needed.

Comment 9

[Mike Taylor [:miketaylr]]

> I think the problem is due to the bundled ua-update.json being inside omnijar.

I don't think it's actually inside the omnijar, but at the APK root.

cf.
https://dxr.mozilla.org/mozilla-central/source/mobile/android/app/moz.build?from=android/app/moz.build#21
https://dxr.mozilla.org/mozilla-central/source/mobile/android/installer/package-manifest.in?from=android/installer/package-manifest.in#512


> $ pwd
> /Users/mtaylor/dev/git-mozcentral/gecko-dev/obj-arm-linux-androidabi/dist
> $ unzip -l fennec-42.0a1.en-US.android-arm.apk | grep ua-update.json
>       55  07-07-15 17:19   ua-update.json

(I also unzipped omni.ja just to be sure).

That said, my assumption was that KEY_APPDIR (XCurProcD) would match the APK root and it looks like I'm wrong.

I wonder if we can do something like this to read the file from the APK root?

https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testResourceSubstitutions.js#40



> I was able to confirm that, once an update happens and we save the updated ua-update.json to disk, UA overrides starts to work again.

Yeah, same. It can happen anywhere from a few seconds to a few minutes after startup.

Comment 10

[Mike Taylor [:miketaylr]]

I wonder if copying ua-update.json to the profile dir on (right before?) creation would be the least hacky approach, and perhaps one that works best with guest profiles.

Richard, would enqueueInitialization be the right place to do something like this?

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoProfile.java#885

Comment 11

[Mike Taylor [:miketaylr]]

On second thought, I don't really want to touch profiles. Let me take a stab at teaching UserAgentUpdates.jsm to find the file in the jar.

Comment 12

[Karl Dubost💡 :karlcow]

can't we replicate what Firefox OS is doing. 
It seemed to me that it was doing the correct thing. And instead of having two separate ways/process maybe we can just adjust the Firefox Android one.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

ua-update.json is a standalone file on FxOS, but it's inside the apk file on Android. We can probably package the file inside omnijar and do a network request for resource://gre/ua-update.json to read the file. That should work on all platforms.

Comment 14

[Richard Newman [:rnewman]]

What Jim said :)

Something to consider: what happens when the file in omni.ja is newer than the downloaded one? Do you always want to use the downloaded one if it exists?

Comment 15

[Mike Taylor [:miketaylr]]

(In reply to Richard Newman [:rnewman] from comment #14)
> Something to consider: what happens when the file in omni.ja is newer than
> the downloaded one? Do you always want to use the downloaded one if it
> exists?

Good question. AFAIU, ua-update.json is pushed to the CDN on an hourly (maybe?) cron job, iff there are changes to that file on m-c. 

If by "the downloaded one" you mean the ua-update.json that is fetched from the server, then I guess there's a possible limbo state for Nightly.

Richard, do you know offhand the cron frequency of the updater script?

Comment 16

[:Atoll]

I think it's once a day at 00:00 UTC. It can be made hourly or more often as needed.

Comment 17

[Mike Taylor [:miketaylr]]

Attachment: 1178760-Read-ua-update.json-from-APK-for-Fennec-.patch

Jim, would you mind reviewing since you authored UserAgentUpdates.jsm?

Rather than moving everything into the omnijar for both B2G and Fennec, this patch allows _applySavedUpdate to read from the APK root (via resource://android/ua-update.json) if it doesn't find an already saved update in user's profile--which happens when a successful _update calls _saveToFile.

Tested locally by adding the following override to mobile/android/app/ua-update.json.in 

"echo.opera.com": "Hello"

And tweaking the prefs in mobile.js to:

pref("general.useragent.updates.enabled", true);
pref("general.useragent.updates.url", "https://miketaylr.com/bzla/ua-update.json");

If you start up Fennec with the patch applied and visit echo.opera.com, you should see "Hello" as the reported User-Agent. After a minute or two (when ua-update.json is fetched), you can refresh and see a User-Agent that says "Wowowowow IT WORKS!11".

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8633290 [details] [diff] [review]
1178760-Read-ua-update.json-from-APK-for-Fennec-.patch

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

The idea looks good to me. I think the code needs a bit more improvement.

::: netwerk/protocol/http/UserAgentUpdates.jsm
@@ +72,5 @@
> +      channel.contentType = "application/json";
> +
> +      NetUtil.asyncFetch(channel, (inputStream, status) => {
> +        if (!Components.isSuccessCode(status)) {
> +          reject();

Shouldn't we return here?

@@ +81,5 @@
> +        );
> +        resolve(data);
> +      });
> +    } catch (ex) {
> +      Cu.reportError("UserAgentUpdates: Could not parse JSON: " + url + " " +

I think | "could not fetch " + url + ... | will be more accurate because JSON parsing happens later on inside the asyncFetch callback.

@@ +83,5 @@
> +      });
> +    } catch (ex) {
> +      Cu.reportError("UserAgentUpdates: Could not parse JSON: " + url + " " +
> +                     ex + "\n" + ex.stack);
> +      resolve(null);

I think we should reject() rather than resolve().

@@ +129,5 @@
>      // try loading from profile dir, then from app dir
>      let dirs = [KEY_PREFDIR, KEY_APPDIR];
> +
> +    // Fennec won't have a saved update in KEY_PREFDIR until an update
> +    // is saved to its profile. So we read ua-update.json from the APK root.

Using .exists() below is not ideal because it causes main thread IO.

Instead of having a separate code path, I think you should keep the original code even for Fennec. The dirs.reduce() call returns a Promise. If that Promise fails and we're on Android, we will make an additional attempt to load from the APK.

So it'll look something like,

> dirs.reduce((...) => {
>   ...
> }, null).then(null, (ex) => {
> 
>   // previous load attempts have failed
>   if (not fennec) {
>     throw ex; // no more attempts; just fail
>   }
>   return readChannel(...); // make another attempt for fennec
> 
> }).then(
>   (update) => this._applyUpdate(update)
> );

@@ +132,5 @@
> +    // Fennec won't have a saved update in KEY_PREFDIR until an update
> +    // is saved to its profile. So we read ua-update.json from the APK root.
> +    if (AppConstants.platform == "android" &&
> +      !FileUtils.getFile(KEY_PREFDIR, [FILE_UPDATES], true).exists()) {
> +      readChannel("resource://android/ua-update.json").then(

Use | "resource://android/" + FILE_UPDATES |

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Also, you should test the failure case as well, i.e. what if we're Fennec but the apk doesn't contain a ua-update.json?

Comment 21

[Mike Taylor [:miketaylr]]

Attachment: 1178760-Add-ability-to-read-ua-update.json-from-.patch

Thanks for the feedback Jim--I didn't pay attention to tryNext returning a promise. I believe I've addressed all comments (and it feels less hacky now).

As for when ua-update.json might be missing from the APK, readChannel will throw and the final catch() will log the exception.

Comment 22

[Mike Taylor [:miketaylr]]

Attachment: 1178760-Re-enable-dynamic-UA-overrides-for-Fenne.patch

Assuming we're g2g on the fix and try looks good, we should flip the pref back on.

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8633762 [details] [diff] [review]
1178760-Add-ability-to-read-ua-update.json-from-.patch

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

r+ with these fixes:

::: netwerk/protocol/http/UserAgentUpdates.jsm
@@ +72,5 @@
> +      channel.contentType = "application/json";
> +
> +      NetUtil.asyncFetch(channel, (inputStream, status) => {
> +        if (!Components.isSuccessCode(status)) {
> +          return;

Sorry, I meant return after calling reject(). Otherwise we'd end up calling resolve() below.

@@ +142,5 @@
>        );
>        // try to load next one if the previous load failed
>        return prevLoad ? prevLoad.then(null, tryNext) : tryNext();
> +    }, null).then(null, (ex) => {
> +      if (!AppConstants.platform == "android") {

AppConstants.platform !== "android"

@@ +152,5 @@
> +      return readChannel("resource://android/" + FILE_UPDATES);
> +    }).then((update) => {
> +      // Apply update if loading was successful
> +      this._applyUpdate(update);
> +      this._scheduleUpdate();

Keep this._scheduleUpdate() outside, because we want to schedule updates whether or not we succeeded reading from a file.

Comment 24

[Mike Taylor [:miketaylr]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #23)
> Comment on attachment 8633762 [details] [diff] [review]
> 1178760-Add-ability-to-read-ua-update.json-from-.patch
> 
> Review of attachment 8633762 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with these fixes:
> 
> ::: netwerk/protocol/http/UserAgentUpdates.jsm
> @@ +72,5 @@
> > +      channel.contentType = "application/json";
> > +
> > +      NetUtil.asyncFetch(channel, (inputStream, status) => {
> > +        if (!Components.isSuccessCode(status)) {
> > +          return;
> 
> Sorry, I meant return after calling reject(). Otherwise we'd end up calling
> resolve() below.

Oops. I'm still cutting my Promises baby teeth.

Will update the patch and push to try, thanks!

Comment 25

[Mike Taylor [:miketaylr]]

Attachment: 1178760-Add-ability-to-read-ua-update.json-from-.patch

Nits addressed, carrying forward r+.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ede2d9ae95

Comment 26

[Mike Taylor [:miketaylr]]

Comment on attachment 8633763 [details] [diff] [review]
1178760-Re-enable-dynamic-UA-overrides-for-Fenne.patch

On second thought, let's spin out re-enabling into its own bug: Bug 1184268.

Comment 27

[Mike Taylor [:miketaylr]]

Try LGTM, some timeouts (only on B2G ICS Emulator debug) and some unrelated video oranges: -> checkin-needed.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/245bf6f33372

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/245bf6f33372