1337782 - UA override web compat add-on caches all URIs ever

Status: RESOLVED FIXED

(Web Compatibility :: Interventions, defect)

Description

[:Gijs (he/him)]

This map:

https://dxr.mozilla.org/mozilla-central/source/browser/extensions/webcompat/content/lib/ua_overrider.jsm?q=path%3Aua_overrider&redirect_type=single#19

is process-global and never gets cleared. If you leave Firefox running a while, that map is going to have a lot of items.

We should probably FIFO limit it to something sane like 500 items.

Also, we're caching the entire URI. I think you just want the main domain, so probably just uri.prePath or something like that.

Comment 1

[:Gijs (he/him)]

Also, that code is not private browsing aware and should either not cache private browsing URIs or store them separately or... something.

Comment 2

[Dennis Schubert [:denschub]]

The main idea behind the cache was mainly to prevent having to look up items of the map for every request ever made and it totally made sense in the very first iteration we had. However, I now question the actual need for this cache.

The map is a map of base domains, and if we don't have an override for a given domain we exit pretty early (at [0] to be precise), which by no means is slower than the cache lookup. So we could, in theory, drop the cache at all.

The only possible downside is that if we have an override for a high-traffic domain like google.com or a subdomain in place, all requests would have to loop over the overrides for the given domain. In practice, the list of overrides probably will stay very small, and they won't stay in there for a long time.

Will do some performance measurements on how this would affect loading time in worse cases.

[0]: https://dxr.mozilla.org/mozilla-central/rev/f4f374622111022d41dd8d5eb9220624135c534a/browser/extensions/webcompat/content/lib/ua_overrider.jsm#110

Comment 3

[Dennis Schubert [:denschub]]

Attachment: Pull request, Version 1

Eric, I have removed the URI caching as mentioned above. I've done some local testing with 10.000 UA overrides and was not able to find any performance issues. When this gets a r+, I'll push the sources including the dummy overrides to try to double-check if we don't regress perf.

Comment 4

[Eric Tsai (no more review request accepted)]

Comment on attachment 8835038 [details] [review]
Pull request, Version 1

Looks good for me now. Add real cache with :Gijs' comment if we need it in the future.

Comment 5

[Dennis Schubert [:denschub]]

Merged. Landing the new changes will be tracked in bug 1337905, closing this one.