Closed Bug 1513574 Opened Last year Closed 4 months ago

Remove UserAgentOverrides.jsm

Categories

(Core :: Networking, enhancement, P2)

60 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(4 files)

UserAgentOverrides should be rewritten as a C++ service for performance and correctness reasons.
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?
Depends on: 1505722
Flags: needinfo?(dschubert)
Summary: Rewrite UserAgentOverrides.jsm as a C++ service → Remove UserAgentOverrides.jsm
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!
Flags: needinfo?(dschubert)
(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!
No longer depends on: 1505722
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.
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
> 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.
Depends on: 1515005

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?

Flags: needinfo?(valentin.gosu)

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?

Flags: needinfo?(valentin.gosu) → needinfo?(astevenson)
Attachment #9031882 - Attachment description: Bug 1513574 - Remove UAOverridesBootstrapper → Bug 1513574 - Remove UAOverridesBootstrapper r=michal
See Also: → 1548428
Pushed by valentin.gosu@gmail.com:
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

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

Flags: needinfo?(valentin.gosu)
Pushed by valentin.gosu@gmail.com:
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
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(astevenson)
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe583182f808
Backed out 4 changesets for causing several browser chrome failures. CLOSED TREE
Pushed by valentin.gosu@gmail.com:
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
Flags: needinfo?(valentin.gosu)

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?

(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.

(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.

(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)

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/

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?

Flags: needinfo?(valentin.gosu)

(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.

Flags: needinfo?(valentin.gosu)
Depends on: 1589607
You need to log in before you can comment on or make changes to this bug.