1473181 - GWS/Facebook UA experiment is not sending the custom UA on XMLHttpRequests

Status: RESOLVED FIXED

(Web Compatibility :: Interventions, enhancement, P1)

Description

[Thomas Wisniewski [:twisniewski]]

It turns out that UserAgentOverrides.jsm observes http-on-useragent-request, which do not occur on XHRs. This causes breakage in the experiment (as seen in the see also link) where the tier-1 Google "more results" buttons are broken, because they send XHRs which get the stock Fennec UA string instead of the custom UA string the experiment should be sending.

If we instead hook into http-on-modify-request, the overrides will apply correctly  to all the relevant requests (it is what webRequest.onBeforeSendHeaders uses).

While we're here I also plan to tweak the way the prefs work so that resetting the preference does not cause the pref to vanish without disabling the add-on.

Comment 1

[Thomas Wisniewski]

Attachment: Bug 1473181 - Observe http modify request rather than useragent request in the GWS/FB addon to support overriding UA string on XHRs as well.

Review commit: https://reviewboard.mozilla.org/r/254626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254626/

Comment 2

[Thomas Wisniewski [:twisniewski]]

Mike, do you perhaps have the cycles to give this one a quicker review?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8989603 [details]
Bug 1473181 - Observe http modify request rather than useragent request in the GWS/FB addon to support overriding UA string on XHRs as well.

https://reviewboard.mozilla.org/r/254626/#review261856

Thanks!

::: mobile/android/extensions/gws-and-facebook-chrome-spoof/bootstrap.js:36
(Diff revision 1)
> +  if (channel) {
> +    if (channel.URI.asciiHost.match(TLDsToSpoof)) {

Let's combine these conditions, like:

```js
if (channel &&
    channel.URI.asciiHost.match(TLDsToSpoof)) {
  // Do work
}
```

::: mobile/android/extensions/gws-and-facebook-chrome-spoof/bootstrap.js:52
(Diff revision 1)
> +  } catch (_) {
> +  }
> +}
> +
> +function disable() {
> +  Services.obs.removeObserver(HTTP_on_modify_request, "http-on-modify-request");

Maybe we should put this in a try/catch too?

::: mobile/android/extensions/gws-and-facebook-chrome-spoof/bootstrap.js:63
(Diff revision 1)
> +      enable();
> +    } else {
> +      disable();
> +    }
> +  } catch (_) {
> +    Services.prefs.getDefaultBranch(EnabledPrefBranch).setBoolPref("enabled", true);

Can you add a comment here about why we're using getDefaultBranch?

::: mobile/android/extensions/gws-and-facebook-chrome-spoof/bootstrap.js:73
(Diff revision 1)
>  this.install = () => {
>    Services.prefs.setBoolPref(EnabledPref, true);
>  };
>  
>  this.uninstall = () => {
>    Services.prefs.clearUserPref(EnabledPref);

We can probably skip this, since we're just deleting the pref now.

Comment 4

[Thomas Wisniewski]

Comment on attachment 8989603 [details]
Bug 1473181 - Observe http modify request rather than useragent request in the GWS/FB addon to support overriding UA string on XHRs as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254626/diff/1-2/

Comment 5

[Thomas Wisniewski]

Comment on attachment 8989603 [details]
Bug 1473181 - Observe http modify request rather than useragent request in the GWS/FB addon to support overriding UA string on XHRs as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254626/diff/2-3/

Comment 6

[Thomas Wisniewski]

Comment on attachment 8989603 [details]
Bug 1473181 - Observe http modify request rather than useragent request in the GWS/FB addon to support overriding UA string on XHRs as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254626/diff/3-4/

Comment 7

[Thomas Wisniewski [:twisniewski]]

Thanks Mike! I had to make one final tweak to pass the right kind of object into add/removeObserver (otherwise it was not actually removing the observer properly when I just passed in the observing function).

I'm carrying over r+, since this was a trivial change as can be seen in the interdiff: https://reviewboard.mozilla.org/r/254626/diff/2-4/

Requesting check-in.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06295b3bc400
Observe http modify request rather than useragent request in the GWS/FB addon to support overriding UA string on XHRs as well. r=mconley

Comment 9

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/06295b3bc400