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

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sebastian, Assigned: miketaylr)

Tracking

41 Branch
Firefox 42
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(p11+, firefox42 fixed, fennec42+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

3 years ago
This could be related to bug 1176018 and adding CATEGORY_BROWSABLE to Intent objects.
See Also: → bug 1176018
(Reporter)

Comment 2

3 years ago
(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();
(Reporter)

Comment 3

3 years ago
@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?
Flags: needinfo?(miket)
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...
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.

Updated

3 years ago
tracking-fennec: --- → ?
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).
See Also: → bug 1179300
Summary: Trying to play YouTube video shows error toast: "Cannot open link" → Dynamic ua updates not being applied from server (ua-override.json)

Updated

3 years ago
tracking-p11: --- → +
tracking-fennec: ? → 42+

Updated

3 years ago
See Also: → bug 1162099
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.
There's not much for me to do server-side, so I'm removing myself. Loop me in if needed.
Flags: needinfo?(rsoderberg)
> 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.
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
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.
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.
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.
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?
(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?
(Reporter)

Updated

3 years ago
See Also: → bug 1154473
I think it's once a day at 00:00 UTC. It can be made hourly or more often as needed.
Flags: needinfo?(rsoderberg)
Assignee: nobody → miket
Created attachment 8633290 [details] [diff] [review]
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 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 |
Attachment #8633290 - Flags: review?(nchen) → feedback+
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?
Duplicate of this bug: 1172015
Created attachment 8633762 [details] [diff] [review]
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.
Attachment #8633290 - Attachment is obsolete: true
Attachment #8633762 - Flags: review?(nchen)
Created attachment 8633763 [details] [diff] [review]
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.
Attachment #8633763 - Flags: review?(margaret.leibovic)
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.
Attachment #8633762 - Flags: review?(nchen) → review+
(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!
Created attachment 8633779 [details] [diff] [review]
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
Attachment #8633762 - Attachment is obsolete: true
Attachment #8633779 - Flags: review+
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.
Attachment #8633763 - Attachment is obsolete: true
Attachment #8633763 - Flags: review?(margaret.leibovic)
Try LGTM, some timeouts (only on B2G ICS Emulator debug) and some unrelated video oranges: -> checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/245bf6f33372
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.