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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: miketaylr, Assigned: twisniewski)
References
Details
(Whiteboard: [webcompat] [geckoview:klar:p2])
Attachments
(2 files, 1 obsolete file)
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
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → twisniewski
Priority: P3 → P1
Reporter | ||
Comment 2•6 years ago
|
||
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)
Reporter | ||
Comment 3•6 years ago
|
||
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.
Comment 4•6 years ago
|
||
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.
Comment 5•6 years ago
|
||
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)
Comment 6•6 years ago
|
||
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)
Reporter | ||
Comment 7•6 years ago
|
||
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)
Comment 8•6 years ago
|
||
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
Comment 9•6 years ago
|
||
(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.
Comment 10•6 years ago
|
||
How would I ship it as an update? Is there a wiki link I could follow to figure that out?
Flags: needinfo?(rhelmer)
Comment 11•6 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Comment 13•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment 15•6 years ago
|
||
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.
Reporter | ||
Comment 16•6 years ago
|
||
(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!
Reporter | ||
Comment 18•6 years ago
|
||
mozreview-review |
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?
Comment 19•6 years ago
|
||
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).
Reporter | ||
Comment 20•6 years ago
|
||
> I do use that in the actual addon's moz.build
Oops, my bad, thanks!
Reporter | ||
Updated•6 years ago
|
Whiteboard: [webcompat]
Reporter | ||
Comment 21•6 years ago
|
||
(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
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Attachment #8971098 -
Attachment is obsolete: true
Comment 23•6 years ago
|
||
I just updated the first patch to add a missing file (install.rdf.in) and marked the second patch as obsolete.
Comment 24•6 years ago
|
||
mozreview-review |
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.
Flags: needinfo?(sdaswani)
> Susheel, should we find an alternative reviewer for this bug?
If so, please clear my r?.
Comment 27•6 years ago
|
||
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 28•6 years ago
|
||
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 29•6 years ago
|
||
mozreview-review |
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+
Comment 30•6 years ago
|
||
(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.
Comment 31•6 years ago
|
||
(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)
Reporter | ||
Comment 32•6 years ago
|
||
I think we're good to wait a few days, thanks Kris.
Comment 33•6 years ago
|
||
I landed the part of bug 1457321 that you should need for this.
Reporter | ||
Comment 34•6 years ago
|
||
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)
Comment 35•6 years ago
|
||
We can. I'll add it to the next version of the patch.
Flags: needinfo?(twisniewski)
Comment 36•6 years ago
|
||
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)
Updated•6 years ago
|
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]
Comment hidden (mozreview-request) |
Comment 38•6 years ago
|
||
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)
Reporter | ||
Comment 39•6 years ago
|
||
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)
Comment 40•6 years ago
|
||
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)
Reporter | ||
Comment 41•6 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 43•6 years ago
|
||
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)
Reporter | ||
Comment 44•6 years ago
|
||
(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.
Comment 45•6 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Comment 47•6 years ago
|
||
(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
Comment 48•6 years ago
|
||
(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
Comment 49•6 years ago
|
||
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
Comment 50•6 years ago
|
||
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
Comment 51•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dfc6d3905891
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 52•6 years ago
|
||
This is live as of today, I'd like it to run for 4 weeks then we should back it out.
Reporter | ||
Comment 53•6 years ago
|
||
https://miketaylr.com/posts/2018/07/google-search-in-firefox-for-android-nightly.html in case anybody shows up here for more info.
Comment 54•6 years ago
|
||
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.
Description
•