1513574 - Remove UserAgentOverrides.jsm

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

UserAgentOverrides should be rewritten as a C++ service for performance and correctness reasons.

Comment 1

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

So, from what I can tell, following bug 1505722, UserAgentOverrides.jsm isn't even needed anymore.
Bug 1511281 can be fixed using nsIDocshell.customUserAgent, meaning the need for UserAgentOverrides.jsm and UserAgentUpdates.jsm is pretty much gone.

Dennis, from what I can tell the webcompat addon is capable enough to replace what we already have here. Can you think of any reason not to remove this code?

Comment 2

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6af91996de0b22031582953edd084e73c3b848c

Comment 3

[Dennis Schubert [:denschub]]

Hey Valentin, thanks for the ping! You are not wrong, the webcompat addon is able to do what we did with UserAgentOverrides.jsm, and although we landed the existing overrides in bug 1512907 yesterday, we are unfortunately not yet ready to remove the old code.

One important piece missing on our side is to verify that we can push updates of that addon to release users on both desktop *and* mobile. Pushing UA overrides to the release population is very important, and we have used that multiple times to fix completely-broken sites.

We are working on this, and will be testing the rollout very soon. We're pretty sure this work will be done in Q1. Unless we have a very urgent reason to remove UserAgentOverrides.jsm right now, it's probably better (for us) to just leave the old code in place for now. I will ping back as soon as we have verified that our plans work out!

Comment 4

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

(In reply to Dennis Schubert [:denschub] from comment #3)
> We are working on this, and will be testing the rollout very soon. We're
> pretty sure this work will be done in Q1. Unless we have a very urgent
> reason to remove UserAgentOverrides.jsm right now, it's probably better (for
> us) to just leave the old code in place for now. I will ping back as soon as
> we have verified that our plans work out!

That's great. I'll have the patches to remove reviewed and ready.
The only rush to remove the code would be bug 1513582 comment 0 ; bug 1443429 comment 23
This bug was initially to rewrite the code in C++, but I'd rather to not do all that work just to remove it a few weeks later. :)

Could you make the "push updates of that addon to release users" bug block this one? Thanks!

Comment 5

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec4b36de13feada061ea38860200df6697cba95a

Comment 6

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

Attachment: Bug 1513574 - Remove UAOverride use from mobile/.../browser.js

This patch removes the use fo UserAgentOverrides from browser.js
The UA change when in desktop mode now uses nsIDocShell.customUserAgent, while the feature landed in bug 838332 that is only performed for t.co URLs is removed, as it landed 4 years ago and was limited to Nightly.

Comment 7

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

Attachment: Bug 1513574 - Remove UAOverridesBootstrapper r=michal

Depends on D14749

Comment 8

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

Attachment: Bug 1513574 - Remove UserAgentOverrides.jsm and UserAgentUpdates.jsm

Also removes the UA cache attached to nsILoadGroup and nsIRequestContext and the "http-on-useragent-request" observer notification.
If overriding the user agent is needed "http-on-modify-request" is equally usable (but should be used rarely, for performance reasons). A better way is using nsIDocShell.customUserAgent.

Depends on D14750

Comment 9

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

Attachment: Bug 1513574 - Remove ua-update.json.in from android build

Depends on D14751

Comment 10

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f90d39b730ed916dbc67a07d93bd9f21ff406d74

Comment 11

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=afcfc368608d6e31e2e9e2a1a894c387f8d08889

Comment 12

[Dennis Schubert [:denschub]]

> Could you make the "push updates of that addon to release users" bug block this one? Thanks!

Absolutely! I filed a meta bug. Right now, we track a lot of this work outside of Bugzilla, but will make sure that the bug gets updated, and closed, when there is progress.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:valentin, could you have a look please?

Comment 14

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

I was waiting on bug 1515005 before I can land the patches.
Adam, I see that bug 1527352 is verified. Can we remove UserAgentOverrides.jsm now?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e4cababccc7
Remove UAOverride use from mobile/.../browser.js r=snorp
https://hg.mozilla.org/integration/autoland/rev/5f8b5f72a4ea
Remove UAOverridesBootstrapper r=michal
https://hg.mozilla.org/integration/autoland/rev/1f2bbbe0f781
Remove UserAgentOverrides.jsm and UserAgentUpdates.jsm r=michal
https://hg.mozilla.org/integration/autoland/rev/f155c449e516
Remove ua-update.json.in from android build r=snorp

Comment 16

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=f155c449e5167ab7ec60027f6f8a67924c966b21&selectedJob=266582414

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=266581728&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=266582414&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/8c3cf0c548977009df6f6c690c7f7335b032626f

[task 2019-09-13T18:25:18.357Z] 18:25:18 INFO - Killing every process called emulator64-arm
[task 2019-09-13T18:25:18.364Z] 18:25:18 INFO - Killing pid 177.
[task 2019-09-13T18:25:18.367Z] 18:25:18 INFO - [mozharness: 2019-09-13 18:25:18.364740Z] Finished run-tests step (failed)
[task 2019-09-13T18:25:18.367Z] 18:25:18 FATAL - Uncaught exception: Traceback (most recent call last):
[task 2019-09-13T18:25:18.368Z] 18:25:18 FATAL - File "/builds/worker/workspace/build/src/testing/mozharness/mozharness/base/script.py", line 2101, in run
[task 2019-09-13T18:25:18.369Z] 18:25:18 FATAL - self.run_action(action)
[task 2019-09-13T18:25:18.369Z] 18:25:18 FATAL - File "/builds/worker/workspace/build/src/testing/mozharness/mozharness/base/script.py", line 2040, in run_action
[task 2019-09-13T18:25:18.369Z] 18:25:18 FATAL - self._possibly_run_method(method_name, error_if_missing=True)
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - File "/builds/worker/workspace/build/src/testing/mozharness/mozharness/base/script.py", line 1995, in _possibly_run_method
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - return getattr(self, method_name)()
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - File "/builds/worker/workspace/build/src/testing/mozharness/scripts/android_emulator_pgo.py", line 229, in run_tests
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - env=env,
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 462, in init
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - self.start_binary(self.startup_timeout)
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 491, in start_binary
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - reraise(IOError, msg.format(timeout), tb)
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 482, in start_binary
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - self.raise_for_port(timeout=timeout)
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/marionette_driver/marionette.py", line 569, in raise_for_port
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - self.host, self.port))
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - IOError: Process killed after 1000s because no connection to Marionette server could be established. Check gecko.log for errors
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - Running post_fatal callback...
[task 2019-09-13T18:25:18.370Z] 18:25:18 FATAL - Exiting -1

[task 2019-09-13T18:04:38.933Z] 18:04:38 INFO - Console message: [JavaScript Error: "RemoteWebProgress failed to call onProgressChange: [Exception... "JavaScript component does not have a method named: "onProgressChange"'JavaScript component does not have a method named: "onProgressChange"' when calling method: [nsIWebProgressListener::onProgressChange]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "JS frame :: resource://gre/modules/RemoteWebProgress.jsm :: _callProgressListeners :: line 75" data: no]
[task 2019-09-13T18:04:38.933Z] 18:04:38 INFO - " {file: "resource://gre/modules/RemoteWebProgress.jsm" line: 77}]
[task 2019-09-13T18:04:38.933Z] 18:04:38 INFO - _callProgressListeners@resource://gre/modules/RemoteWebProgress.jsm:77:14
[task 2019-09-13T18:04:38.933Z] 18:04:38 INFO - onProgressChange@resource://gre/modules/RemoteWebProgress.jsm:104:10
[task 2019-09-13T18:04:38.934Z] 18:04:38 INFO -
[task 2019-09-13T18:04:38.937Z] 18:04:38 INFO - Console message: [JavaScript Warning: "Loading failed for the <script> with source “http://example.com/browser/dom/serviceworkers/test/utils.js”." {file: "http://example.com/browser/dom/serviceworkers/test/empty_with_utils.html" line: 9}]
[task 2019-09-13T18:04:38.937Z] 18:04:38 INFO - Buffered messages finished
[task 2019-09-13T18:04:38.937Z] 18:04:38 INFO - TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/browser_navigation_process_swap.js | Test timed out -
[task 2019-09-13T18:04:38.937Z] 18:04:38 INFO - GECKO(4209) | JavaScript error: resource://testing-common/PromiseTestUtils.jsm, line 112: uncaught exception: Object
[task 2019-09-13T18:04:38.938Z] 18:04:38 INFO - Console message: [JavaScript Error: "uncaught exception: Object" {file: "resource://testing-common/PromiseTestUtils.jsm" line: 112}]
[task 2019-09-13T18:04:38.938Z] 18:04:38 INFO - GECKO(4209) | MEMORY STAT | vsize 20975013MB | residentFast 1064MB
[task 2019-09-13T18:04:38.938Z] 18:04:38 INFO - TEST-OK | dom/serviceworkers/test/browser_navigation_process_swap.js | took 90028ms
[task 2019-09-13T18:04:38.938Z] 18:04:38 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-09-13T18:04:38.939Z] 18:04:38 INFO - TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/browser_navigation_process_swap.js | Found a tab after previous test timed out: about:blank -
[task 2019-09-13T18:04:38.939Z] 18:04:38 INFO - checking window state
[task 2019-09-13T18:04:38.939Z] 18:04:38 INFO - TEST-START | dom/serviceworkers/test/browser_storage_permission.js
[task 2019-09-13T18:04:42.005Z] 18:04:42 INFO - GECKO(4209) | JavaScript error: resource://testing-common/PromiseTestUtils.jsm, line 112: uncaught exception: Object

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce51efd054b8
Remove UAOverride use from mobile/.../browser.js r=snorp
https://hg.mozilla.org/integration/autoland/rev/be6959a563f6
Remove UAOverridesBootstrapper r=michal
https://hg.mozilla.org/integration/autoland/rev/11f015a3e739
Remove UserAgentOverrides.jsm and UserAgentUpdates.jsm r=michal
https://hg.mozilla.org/integration/autoland/rev/7d950fc452fb
Remove ua-update.json.in from android build r=snorp

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe583182f808
Backed out 4 changesets for causing several browser chrome failures. CLOSED TREE

Comment 19

[Cosmin Sabou [:CosminS]]

Backed out 4 changesets (Bug 1513574) for causing several browser chrome failures. Please take a look over those browser chrome failures that turned into permafailures after this bug landed. That happened also on your previous land here: https://tinyurl.com/y28el3c9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=7d950fc452fbce20546a5f415245cd4e788edd13&searchStr=browser%2Cchrome&selectedJob=266826086

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266826086&repo=autoland&lineNumber=17656
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266824748&repo=autoland&lineNumber=12603
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266826095&repo=autoland&lineNumber=13892
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266824590&repo=autoland&lineNumber=3244

Backout link: https://hg.mozilla.org/integration/autoland/rev/fe583182f808dc2c7587c1468b3a54932ed0908a

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ab38d48e289
Remove UAOverride use from mobile/.../browser.js r=snorp
https://hg.mozilla.org/integration/autoland/rev/2e0fe7f115ee
Remove UAOverridesBootstrapper r=michal
https://hg.mozilla.org/integration/autoland/rev/59d191ed3f5b
Remove UserAgentOverrides.jsm and UserAgentUpdates.jsm r=michal
https://hg.mozilla.org/integration/autoland/rev/4536fef9e911
Remove ua-update.json.in from android build r=snorp

Comment 21

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3ab38d48e289
https://hg.mozilla.org/mozilla-central/rev/2e0fe7f115ee
https://hg.mozilla.org/mozilla-central/rev/59d191ed3f5b
https://hg.mozilla.org/mozilla-central/rev/4536fef9e911

Comment 22

[alessino]

Does this change mean the domain specific override using "general.useragent.override.[DOMAIN]" does not work any more? Because this is what apparently happened. Since the build of September 17th that setting is ignored.

Any workaround?

Comment 23

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

(In reply to alessino from comment #22)

Does this change mean the domain specific override using "general.useragent.override.[DOMAIN]" does not work any more? Because this is what apparently happened. Since the build of September 17th that setting is ignored.

Any workaround?

That is correct.
There are lots of webextensions that can change the username in a much more reliable way.
Take your pick.

Comment 24

[alessino]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #23)

That is correct.
There are lots of webextensions that can change the username in a much more reliable way.
Take your pick.

Thanks for the response.

It would seem as if "general.useragent.override" is still working, right?

Was there a particular reason why the domain specific part was removed? It did work reliably and was a lot more elegant than to have to install yet another extension.

Comment 25

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

(In reply to alessino from comment #24)

It would seem as if "general.useragent.override" is still working, right?

Yes, the general browser-wide UA pref still works.

Was there a particular reason why the domain specific part was removed? It did work reliably and was a lot more elegant than to have to install yet another extension.

Yes. This code was quite inefficient, running for all loads, checking if you might have a site-specific UA pref set.
Plus, it had a years old bug where the UA change only happened for the top level load, not for subresource loads.
So it was bad, inefficient and buggy 🙂 We now have better ways for users to perform this change (via webextensions), and for Mozilla to run UA interventions (via webcompat-addon)

Comment 26

[alessino]

Thats a pity.

It was a very elegant and convenient way to keep the old user interface on Twitter -> https://www.reddit.com/r/Twitter/comments/ce1bea/reverting_back_to_the_old_twitter_interface_for/

Comment 27

[Filip Štamcar]

I also think that it was better to just create about:config entry than installing another extension just to overwrite user-agent, especially if you want to change it for the long term. Because of that, I think that it would be better to implement it back.

Yes. This code was quite inefficient, running for all loads, checking if you might have a site-specific UA pref set.

So, can it be rewritten without those bugs? If addons can already change user-agent, wouldn't be possible to just add another option to "fetch" it from about:config?

Comment 28

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

(In reply to Filip Š from comment #27)

I also think that it was better to just create about:config entry than installing another extension just to overwrite user-agent, especially if you want to change it for the long term. Because of that, I think that it would be better to implement it back.

While this might work for you, about:config is a tool non-technical users should be using, as changing some things can completely break your browser.

Yes. This code was quite inefficient, running for all loads, checking if you might have a site-specific UA pref set.

So, can it be rewritten without those bugs? If addons can already change user-agent, wouldn't be possible to just add another option to "fetch" it from about:config?

Things that are controlled by about:config prefs will sooner or later break. We can only test a small number of configurations, and just as before, having multiple ways of changing the userAgent means not all of them are well tested. And the technical burden of maintaining the code that does this (even assuming it's "bug free") is not worth it.
I agree that it was more convenient just to add a pref for what you wanted, but webExtensions allows you to do that just as easily, and is well tested.