1162099 - Use the dynamic UA override mechanism in Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Taylor [:miketaylr]]

The infrastructure for this was handled in Bug 897221.

This bug would mean Fennec would be able to actually use it which would require:

1) Shipping a ua-updates.json.
2) Settings prefs to enable it and point to the updateable list.
2) Making sure the in-tree file is synced to the same place B2G's is: "https://dynamicua.cdn.mozilla.net/0/%APP_ID%"

(once this is done we could move the youtube override from mobile.js to ua-updates.json as well).

I'm happy to write the patches for this, with some feedback from the Fennec team.

Comment 1

[Mike Taylor [:miketaylr]]

I should note that it's possible to restrict this to distributions as well, rather than shipping to everyone. But that may lead to situations where people on different Fennec builds/distributions get different experiences, which seems bad.

Comment 2

[Mike Taylor [:miketaylr]]

Attachment: Part-1-Bug-1162099-Create-ua-override.json.in-and-move-Yout.patch

Comment 3

[Mike Taylor [:miketaylr]]

Attachment: Part-2-Bug-1162099-Add-Makefile.in-to-mobile-android-app-fo.patch

Comment 4

[Mike Taylor [:miketaylr]]

Attachment: Part-3-Bug-1162099-Add-ua-update.json-to-package-manifest.patch

Comment 5

[Mike Taylor [:miketaylr]]

Attachment: Part-4-Bug-1162099-Set-the-prefs-to-use-the-dynamic-UA-over.patch

Comment 6

[Mike Taylor [:miketaylr]]

If you want to test that this works manually, you can flip use these pref values in mobile.js:

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

You should then be able to navigate to echo.opera.com and see something like "IT WORKS!!" for a User-Agent header.

Comment 7

[Mike Taylor [:miketaylr]]

I should note that I cribbed the Makefile jazz from how B2G does this: <https://dxr.mozilla.org/mozilla-central/source/b2g/app/Makefile.in#8>

I about as inexperienced as they get with the build system, so there may be nicer ways to go about this.

Comment 8

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

Comment on attachment 8604292 [details] [diff] [review]
Part-2-Bug-1162099-Add-Makefile.in-to-mobile-android-app-fo.patch

Nick might have ideas for moving this into moz.build

Comment 9

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

Comment on attachment 8604290 [details] [diff] [review]
Part-1-Bug-1162099-Create-ua-override.json.in-and-move-Yout.patch

>diff --git a/mobile/android/app/ua-update.json.in b/mobile/android/app/ua-update.json.in

>+  "youtube.com": "Android; Tablet;#Android; Mobile;"
>+}
>\ No newline at end of file

Add a newline

Comment 10

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

Comment on attachment 8604294 [details] [diff] [review]
Part-4-Bug-1162099-Set-the-prefs-to-use-the-dynamic-UA-over.patch


>+// See ua-update.json.in for the packaged UA override list
>+// XXX See Bug 1163759 before setting this to true

Drop the XXX

Comment 11

[Mike Taylor [:miketaylr]]

Thanks for the reviews Mark.

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 8604292 [details] [diff] [review]
> Part-2-Bug-1162099-Add-Makefile.in-to-mobile-android-app-fo.patch
> 
> Nick might have ideas for moving this into moz.build

Cool. Originally I was using DIST_FILES += ['ua-update.json.in'] in /m/a/app's moz.build, but I wasn't sure if it was possible to strip comments from the file with sed after that point (without a Makefile). Like I said, sketchy grasp of the build system.

Comment 12

[Mike Taylor [:miketaylr]]

Attachment: Part-1-Bug-1162099-Create-ua-override.json.in.patch

Carrying forward r+.

Comment 13

[Mike Taylor [:miketaylr]]

Attachment: Part-4-Bug-1162099-Set-the-prefs-to-use-the-dynamic-UA-over.patch

Carrying forward r+.

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8604292 [details] [diff] [review]
Part-2-Bug-1162099-Add-Makefile.in-to-mobile-android-app-fo.patch

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

We really don't want to add more invocations of sed to the build system.

There are two things we can do here: use DIST_FILES (in moz.build) and the preprocessor's built-in "slashslash" directive, which appears to be intended for this usage; or use the new-ish moz.build GENERATED_FILES option.  I'd prefer the former.  So we'd have:

.json.in:
#filter slashslash
// Comments.
{
  "foo": "bar", // Comment.
  // Full line comment.
}

in moz.build:
DIST_FILES += ['.json.in']

(I see an example in https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/extensions of a preprocessed DIST_FILES entry.)

We should do this both here and in b2g/app.  I am happy to do this and get build peer review, or to make this a separate ticket.

Comment 15

[Mike Taylor [:miketaylr]]

Attachment: 1162099-1-Create-ua-override.json.patch

gps, we've tested that the "slashslash" preprocessor directive does what it's supposed to here (which is to remove the comments so the JSON isn't invalid). 

Is there any reason to not use it? I see there isn't any current usage in the tree.

Comment 16

[Mike Taylor [:miketaylr]]

Attachment: 1162099-2-Add-to-package-manifest.patch

(updated to include mfinkle's r+ in the commit message)

Comment 17

[Mike Taylor [:miketaylr]]

Attachment: 1162099-3-Add-to-DIST_FILES.patch

Comment 18

[Mike Taylor [:miketaylr]]

Attachment: 1162099-4-set-prefs.patch

(updated commit message w/ r=mfinkle)

Comment 19

[Mike Taylor [:miketaylr]]

Once the updater mechanism does its thing and hits the server, this works as expected. For the life of me I can't figure out why I get the following message in the console on startup though (before anything is fetched from the network). I double checked with the original patchsets and it showed up there too...I think I missed it by not clearing data once an updated was saved to my profile.

E/GeckoConsole(29088): [JavaScript Error: "A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
E/GeckoConsole(29088): See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
E/GeckoConsole(29088): 
E/GeckoConsole(29088): Date: Wed May 13 2015 21:30:23 GMT-0500 (CDT)
E/GeckoConsole(29088): Full Message: Unix error 2 during operation open on file /data/data/org.mozilla.fennec_mtaylor/ua-update.json (No such file or directory)
E/GeckoConsole(29088): Full Stack: JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: PendingErrors.register :: line 162
E/GeckoConsole(29088): JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.completePromise :: line 675
E/GeckoConsole(29088): JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 903
E/GeckoConsole(29088): JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 746
E/GeckoConsole(29088): JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/

Nick, does dist/bin/ua-update.json not correspond to /data/data/org.mozilla.fennec_mtaylor/ua-update.json?

Comment 20

[Mike Taylor [:miketaylr]]

Acutally, maybe it's better to  ni? jchen since he wrote the original code.

Comment 21

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8605617 [details] [diff] [review]
1162099-1-Create-ua-override.json.patch

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

::: mobile/android/app/ua-update.json.in
@@ +1,2 @@
> +#filter slashslash
> +// Comments must be on their own lines and must start with "//"

This is no longer true.  Everything after // is a comment.  If we were to ever want // in a User-Agent string, that would be a problem.

Comment 22

[Nick Alexander :nalexander [he/him]]

> Nick, does dist/bin/ua-update.json not correspond to
> /data/data/org.mozilla.fennec_mtaylor/ua-update.json?

Add ua-update.json to the list at https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/upload-files.mk#305.

Then run |mach package| and verify that it is in the APK:

nalexander@chocho ~/M/gecko> unzip -l objdir-droid/dist/fennec-*apk | grep ua-update.json
       62  05-13-15 20:29   ua-update.json

I would much prefer such files make it into assets/, but one thing at a time.  See Bug 1160563.

Comment 23

[Mike Taylor [:miketaylr]]

(In reply to Nick Alexander :nalexander from comment #21)
> ::: mobile/android/app/ua-update.json.in
> @@ +1,2 @@
> > +#filter slashslash
> > +// Comments must be on their own lines and must start with "//"
> 
> This is no longer true.  Everything after // is a comment.  If we were to
> ever want // in a User-Agent string, that would be a problem.

Roger that.

(In reply to Nick Alexander :nalexander from comment #22)
> > Nick, does dist/bin/ua-update.json not correspond to
> > /data/data/org.mozilla.fennec_mtaylor/ua-update.json?
> 
> Add ua-update.json to the list at
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/
> upload-files.mk#305.

Cool, thanks again. ^_^

Comment 24

[Mike Taylor [:miketaylr]]

Attachment: 1162099-1-Create-ua-override.json.patch

Updated the comment and commit message to reflect r+.

Comment 25

[Mike Taylor [:miketaylr]]

Attachment: 1162099-2,3-Add-ua-override-to-package-manifest+DIST_FILES.patch

Seems like it makes sense to squash these commits that add ua-update.json(.in) to package.manifest, upload-files.mk and moz.build.

And one last r? nalexander to rubber stamp adding the file to upload-files.mk.

Comment 26

[Mike Taylor [:miketaylr]]

Comment on attachment 8605646 [details] [diff] [review]
1162099-1-Create-ua-override.json.patch

Oops, forgot to re-request feedback from gps on using the slashslash filter.

Comment 27

[Mike Taylor [:miketaylr]]

Try run looks good, but I'll wait for gps' feedback before requesting check-in.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c07b2ed6d234&filter-tier=1

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8605646 [details] [diff] [review]
1162099-1-Create-ua-override.json.patch

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

I'm not a huge fan of using the preprocessor period. But JSON doesn't give you much choice. It's better to have comments than no comments. So this is acceptable.

Alternatively, you could write a Python script that defines a data structure and prints JSON. But that's probably a bit more scaffolding than you are willing to create. If only JSON supported in-line comments.

Comment 29

[Mike Taylor [:miketaylr]]

Thanks! Given the time-sensitive nature of this request, I think we'll skip the python -> JSON routine. But maybe that could be an option for the future.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/346670ddd12d
https://hg.mozilla.org/integration/fx-team/rev/884f77cac189
https://hg.mozilla.org/integration/fx-team/rev/141de298d81e

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/346670ddd12d
https://hg.mozilla.org/mozilla-central/rev/884f77cac189
https://hg.mozilla.org/mozilla-central/rev/141de298d81e