Closed Bug 1453691 Opened 6 years ago Closed 6 years ago

Create a system addon for overriding Fennec UA string for Google and Facebook properties

Categories

(Web Compatibility :: Interventions, enhancement, P1)

Product:
Web Compatibility
Component:
Interventions
Platform:
Unspecified
Android
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: miketaylr, Assigned: twisniewski)

References

Details

(Whiteboard: [webcompat] [geckoview:klar:p2])

Attachments

(2 files, 1 obsolete file)

Bug 1453691 - create a Fennec system addon to spoof the UA string as Chrome for Google Search and Facebook.
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Screenshot_20180530-161014.png
189.92 KB, image/png
Details 
We would like to create a system addon for running an experiment with pre-release users, testing if using the Chrome Mobile UA string results in better experiences for Facebook and Google (search?) properties. This bug will track that work.
[triage] Seems non-critical but please change to P1 if someone is actively working on this - thanks! :)
Priority: -- → P3
Assignee: nobody → twisniewski
Priority: P3 → P1
Andreas, any recs for what UA string we should use?

"Mozilla/5.0 (Linux; Android 5.0; SM-G900P Build/LRX21T) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3401.0 Mobile Safari/537.36" is what the chrome devtools uses for a Galaxy S5. Do we know what the most popular device people use Fennec on? Maybe we should use that (I'm guessing a Samsung Galaxy s6 or something).

Maybe we should add some token or version number hint in the UA string so we can recognize bug reports?

Looking at release tags, I don't see anything that ends in .999 (and there are some version that have 3 digit PATCH numbers, so it shouldn't break UA parsing).

https://chromium.googlesource.com/chromium/src.git/+refs

Maybe Mozilla/5.0 (Linux; Android 5.0; SM-G900P Build/LRX21T) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3401.999 Mobile Safari/537.36
Flags: needinfo?(abovens)
Also, we want this to be NIGHTLY only for a week or so to ensure we don't break the world for FB and Google users. After that we'll request beta uplift. But neither version should be able to ride to release.
As far as the UA string goes, in my Google Search Fixer addon I'm just using a stock Chrome UA string from a Nexus 5 phone or Nexus 7 tablet, depending on whether the user is on a phone or tablet.

However, I'm also changing the Chrome/x.x.x.x version number to y.0.0.0, where y is the current Firefox version plus 4 (in an effort to make it stay "current" without having to update the addon regularly just for that purpose).

I'm also adding an extra string "FxQuantum/x.x.x.x" with Firefox's current version number.

It has been working fine so far.
Samsung Galaxy S7 is the most popular device, but I don't think it matters that much. +1 for using a token.
Flags: needinfo?(abovens)
Oh, I should also mention that spoofing the UA device type this vaguely will also cause breakage that might annoy users (though I've only had one report of this so far on my other addon):

>Please note that using this addon will make your browser appear to Google
>to be an "LG Nexus" device, regardless of the actual make and model number
>of your device. As such it will appear that way on your Google
>"recently used devices" listings (https://myaccount.google.com/device-activity).

The only way to avoid this would be for me to more accurately spoof that part of the UA-string (ie, "Nexus 5 Build/MRA58N"), but webextensions don't seem to have access to that info, so to avoid this issue I'd have to dig around to find out how to get that info in Gecko, and also make this a hybrid/bootstrapped addon.

What do you think, Andreas, Mike?
Flags: needinfo?(miket)
Flags: needinfo?(abovens)
I think for this experiment, we can just ignore that Tom. But thanks for raising -- I hadn't considered that side effect.
Flags: needinfo?(miket)
It turns out that the code to handle Fennec system addons was removed when FlyWeb was removed in bug 1374574, and things later changed even more so that Fennec system addons won't be picked up during packaging until bug 1440789 lands.

On top of that, it seems like I have to use a mozilla.com address in my addon id, though other system addons use mozilla.org. That shouldn't be an issue for this experiment, but I felt it was definitely worth noting in case it's an inconsistency which will cause issues for other system addons.
Depends on: 1440789
(In reply to Thomas Wisniewski from comment #8)
> It turns out that the code to handle Fennec system addons was removed when
> FlyWeb was removed in bug 1374574, and things later changed even more so
> that Fennec system addons won't be picked up during packaging until bug
> 1440789 lands.
> 
> On top of that, it seems like I have to use a mozilla.com address in my
> addon id, though other system addons use mozilla.org. That shouldn't be an
> issue for this experiment, but I felt it was definitely worth noting in case
> it's an inconsistency which will cause issues for other system addons.

Bug 1440789 is only for built-in add-ons (those that ship with the application)

If you're shipping the system add-on as an update then it should work today.
How would I ship it as an update? Is there a wiki link I could follow to figure that out?
Flags: needinfo?(rhelmer)
(In reply to Thomas Wisniewski from comment #10)
> How would I ship it as an update? Is there a wiki link I could follow to
> figure that out?

Yes, please see this on shipping a one-off update:

https://wiki.mozilla.org/Firefox/Go_Faster/System_Add-ons/Process#Is_this_a_one-off_system_add-on.3F

Then for actually getting it shipped see: https://wiki.mozilla.org/Firefox/Go_Faster/System_Add-ons/Process#Tracking_Bug_and_Intent_to_Implement
Flags: needinfo?(rhelmer)
Alright, I have the candidate addon ready, so we can decide how to integrate it as we wish. In the review request it takes the form of a built-in system addon, though the patches there to make it build don't necessarily work correctly as-is. The extension itself is ready for consideration/review.
It turns out that it was also easier than I thought to include the actual device's model and build id in the Chrome UA String we're spoofing, rather than a hard-coded version that could have issues like the one I mentioned in comment #6.

However, it does require changes to C++ code, in order to grab the same values that the Chromium code uses to build their UA string. They're minor, but it's something to consider.
(In reply to Thomas Wisniewski from comment #15)
> It turns out that it was also easier than I thought to include the actual
> device's model and build id in the Chrome UA String we're spoofing, rather
> than a hard-coded version that could have issues like the one I mentioned in
> comment #6.
> 
> However, it does require changes to C++ code, in order to grab the same
> values that the Chromium code uses to build their UA string. They're minor,
> but it's something to consider.

I think we shouldn't go this route right now -- historically we haven't included this in our UA string for privacy and fingerprinting reasons. Exposing that to Google and FB for an experiment seems like bad optics, and would probably need some sign off from privacy/leadership folks. But it's cool you have a patch in case we want to do that in the future!
I agree with Mike :)
Flags: needinfo?(abovens)
Comment on attachment 8971092 [details]
Bug 1453691 - create a Fennec system addon to spoof the UA string as Chrome for Google Search and Facebook.

https://reviewboard.mozilla.org/r/239880/#review246180

::: mobile/android/extensions/moz.build:8
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +with Files('**'):
> +    BUG_COMPONENT = ('Firefox for Android', 'General')

Can we change this to Web Compatibility Tools, General?
I do use that in the actual addon's moz.build, the one you mentioned is the buildfile for all Fennec system addons. It was removed when FlyWeb was removed in bug 1374574, and I basically just resurrected it from that patchset and changed the addon list to only show this one.

Also bear in mind that this buildfile might not end up in the final patchset. If we take rhelmer's suggestion in comment 9 and ship this as a one-off addon, then we won't need the parts of this patchset that make this a built-in system addon (including that moz.build file).
> I do use that in the actual addon's moz.build

Oops, my bad, thanks!
Whiteboard: [webcompat]
(gonna move this, but this won't affect reviews, etc!)
Component: General → Go Faster
Product: Firefox for Android → Web Compatibility Tools
Version: Firefox 56 → unspecified
Attachment #8971098 - Attachment is obsolete: true
I just updated the first patch to add a missing file (install.rdf.in) and marked the second patch as obsolete.
Comment on attachment 8971092 [details]
Bug 1453691 - create a Fennec system addon to spoof the UA string as Chrome for Google Search and Facebook.

https://reviewboard.mozilla.org/r/239880/#review247708


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: mobile/android/extensions/gws-and-facebook-chrome-spoof/bootstrap.js:45
(Diff revision 2)
> +      let domain = new URL(channel.URI.spec).host;
> +      if (domain.match(TLDsToSpoof)) {
> +        console.info(OverrideNotice);
> +        return TargetUA;
> +      }
> +    } catch(_) {

Error: Expected space(s) after "catch". [eslint: keyword-spacing]
Susheel, should we find an alternative reviewer for this bug?

FYI that I'm only on Fennec for critical reviews and triage.
> Susheel, should we find an alternative reviewer for this bug?

If so, please clear my r?.
Andreas do we even want to consider this change / feature? Not sure per your comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1453691#c17
Flags: needinfo?(sdaswani) → needinfo?(abovens)
Attachment #8971092 - Flags: review?(michael.l.comella)
Comment on attachment 8971092 [details]
Bug 1453691 - create a Fennec system addon to spoof the UA string as Chrome for Google Search and Facebook.

kmag, I seem to recall you doing some cleanup recently on this python json-file-generating-hack used for built-in add-ons, is that something that can be re-used here too?
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8971092 [details]
Bug 1453691 - create a Fennec system addon to spoof the UA string as Chrome for Google Search and Facebook.

https://reviewboard.mozilla.org/r/239880/#review247722

This lgtm as-is... is this done as a built-in extension so it can easily be overridden remotely with an update?

Seems like the core of this (`TLDsToSpoof` and UAs to substitute) could be delivered as data, while preserving the option of shipping bespoke code if there's no alternative. As-is any updates will need to ship new code, and there's always some non-zero risk around this.

I don't think that should stop shipping this or anything, just something to consider for the future.
Attachment #8971092 - Flags: review?(rhelmer) → review+
(In reply to :sdaswani from comment #27)
> Andreas do we even want to consider this change / feature? Not sure per your
> comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1453691#c17

I'd read that comment as only agreeing with comment #16 above, i.e. not including the actual device name/build ID in the spoofed UA.
(In reply to Robert Helmer [:rhelmer] from comment #28)
> kmag, I seem to recall you doing some cleanup recently on this python
> json-file-generating-hack used for built-in add-ons, is that something that
> can be re-used here too?

Yup. Bug 1457321 gets us 90% of the way here. We'd just need to pass the featured directory for fennec to the build action.

Please either wait for that to land (in the next couple of days) or take that part of the patch.
Flags: needinfo?(kmaglione+bmo)
I think we're good to wait a few days, thanks Kris.
I landed the part of bug 1457321 that you should need for this.
Tom, can we add a pref so a user could manually disable this or opt-out, in case there are unintended side effects (before we could back it out)? Something like extensions.gws-and-facebook-chrome-spoof.enabled or whatever.
Flags: needinfo?(twisniewski)
We can. I'll add it to the next version of the patch.
Flags: needinfo?(twisniewski)
Yes, we should go ahead with the feature. I just don't think we should expose device details in the UA string (as Mike also indicated).
Flags: needinfo?(abovens)
OS: Unspecified → Android
Summary: Create a system addon for overriding UA string for Google and Facebook properties → Create a system addon for overriding Fennec UA string for Google and Facebook properties
Whiteboard: [webcompat] → [webcompat] [geckoview:klar:p2]
Alright, I've updated the patch to include the pref Mike requested in comment 34 and also updated how the system addon is found to match the work kmag has completed in bug 1457321.

There may still be a quirk that kmag wants to investigate (wherein you have to run "mach build faster" instead of just "mach build" for the addon to be bundled properly), but it's unclear to me if that issue affects us or not just yet.

I otherwise think this is ready.

sdaswani, were you still going to do a review on this?
Flags: needinfo?(sdaswani)
Thanks Tom!

> sdaswani, were you still going to do a review on this?

We don't need a review from Susheel. ^__^

One last thing, Tom -- having just spoke with Andreas, we'd like to ship this as NIGHTLY-only for now, rather than push a system add-on to the beta population.

One way to do that, is to only include the sources in mobile/android/extensions/moz.build for nightly, if CONFIG['NIGHTLY_BUILD']:, etc.
Flags: needinfo?(wisniewskit)
Funny enough, it turns out that I already have that in mobile/android/extensions/moz.build as:

>+# Only include the following system add-ons if building Aurora or Nightly
>+if not CONFIG['RELEASE_OR_BETA']:
>+    DIRS += [
>+        'gws-and-facebook-chrome-spoof',
>+    ]

Will that do, or should I change it to NIGHTLY_BUILD instead to be 100% sure?
Flags: needinfo?(wisniewskit)
Flags: needinfo?(sdaswani)
Flags: needinfo?(miket)
(In reply to Thomas Wisniewski from comment #40)
> Funny enough, it turns out that I already have that in
> mobile/android/extensions/moz.build as:
> 
> >+# Only include the following system add-ons if building Aurora or Nightly
> >+if not CONFIG['RELEASE_OR_BETA']:
> >+    DIRS += [
> >+        'gws-and-facebook-chrome-spoof',
> >+    ]
> 
> Will that do, or should I change it to NIGHTLY_BUILD instead to be 100% sure?

Yeah, just NIGHTLY_BUILD, we don't want this to ride to Beta right now.
Flags: needinfo?(miket)
Hey Robert, the current plan is to land the patches as-is, Nightly-only for a few weeks and observe Google Play feedback and incoming compat reports. Just a heads up since they changed slightly (added a pref, tweaked to be nightly only) since you gave r+. Thanks!
Flags: needinfo?(rhelmer)
(In reply to Thomas Wisniewski from comment #38)
> There may still be a quirk that kmag wants to investigate (wherein you have
> to run "mach build faster" instead of just "mach build" for the addon to be
> bundled properly), but it's unclear to me if that issue affects us or not
> just yet.

I didn't run into this quirk -- a regular ./mach build (and package and install, etc) is working for me as expected.
(In reply to Mike Taylor [:miketaylr] (62 Regression Engineering Owner) from comment #43)
> Hey Robert, the current plan is to land the patches as-is, Nightly-only for
> a few weeks and observe Google Play feedback and incoming compat reports.
> Just a heads up since they changed slightly (added a pref, tweaked to be
> nightly only) since you gave r+. Thanks!

WFM, thanks for the heads-up!
Flags: needinfo?(rhelmer)
(In reply to Mike Taylor [:miketaylr] (62 Regression Engineering Owner) from comment #44)
> I didn't run into this quirk -- a regular ./mach build (and package and
> install, etc) is working for me as expected.

Oddly, I still run into it. Maybe it's Linux-specific?

Either way, I've adjusted the comment to reflect that this is nightly-only, and am requesting check-in.
Keywords: checkin-needed
(In reply to Thomas Wisniewski from comment #47)
> (In reply to Mike Taylor [:miketaylr] (62 Regression Engineering Owner) from
> comment #44)
> > I didn't run into this quirk -- a regular ./mach build (and package and
> > install, etc) is working for me as expected.
> 
> Oddly, I still run into it. Maybe it's Linux-specific?
> 
> Either way, I've adjusted the comment to reflect that this is nightly-only,
> and am requesting check-in.

This cannot be landed as in mozzreview there 1 open issue.
Keywords: checkin-needed
Le sigh. Someday I'll learn to consistently mark those as fixed before requesting checkin (and that day will probably be just as we moved to another review tool).

Let's try again.
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfc6d3905891
create a Fennec system addon to spoof the UA string as Chrome for Google Search and Facebook. r=rhelmer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dfc6d3905891
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1466539
Depends on: 1472110
This is live as of today, I'd like it to run for 4 weeks then we should back it out.
Blocks: 1472220
Blocks: 1473181
Depends on: 1473354
No longer depends on: 1473354
Adding the seeAlso for Bug 1232091  for the main reasons why we are running this spoofing on Facebook too.
See Also: → 1232091
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: