1296011 - Create prototype go faster addon to do UA overrides

Status: RESOLVED FIXED

(Web Compatibility :: Interventions, defect)

Description

[Mike Taylor [:miketaylr]]

(Bonus points for a commit that describes in the repo README how to build against the GitHub repo and link to moz-central)

Creating this bug for Dennis to use as a feedback mechanism for his current prototype.

Comment 1

[Dennis Schubert [:denschub]]

Attachment: PR containing the prototype for UA overrides in a system add-on

In this PR, I created a small proof of concept to override user agents from a (system) extension with minimal impact on page loading times. This extension simply prepends "GoFaster Test" to all user agents to show it's working.

I use a `nsIObserver` to listen for `http-on-useragent-request` notifications. Although this notification is not documented ([0]), it is used by `UserAgentOverrides.jsm` [1] to allow user agent overrides in Fennec and Firefox.

Even if we'd decide to remove `UserAgentOverrides.jsm` and the updating stuff for Fennec, we could continue using this notification as long as the calls in `nsHttpChannel.cpp` [2] are kept in place.

`UserAgentOverrides.jsm` got disabled for desktop builds in bug 896114 because of its huge impact on page loading times. On one hand, this was caused by less-than-optimal domain name checking algorithms, but there has been a performance issue caused by invoking the override generator for all requests.

This has been solved in bug 1148544 by implementing a "cache" for the UA overrides. However, something seems to be a bit off here. In bug 1148544 comment 6, :snorp wrote:

> It would if for instance you were navigating to foobar.com and it loaded
> some resource (js, css) from yahoo.com. The resource load for yahoo.com
> would use whatever UA we determined for the containing document (in this
> case foobar.com). But if you went directly to yahoo.com or loaded yahoo.com
> in an iframe, things would work as they do now more or less.

However, that's not the case. The override module gets invoked for the document URI and the override gets applied there, but all subsequent requests (included CSS, JS, images, ...) will get the default UA, not the one we've determined from the document URI.

Off the top of my head, I cannot think of a case where this could lead to problems since usually, UA differences are invoked by the app server and not the CDN shipping JS files, but this is something to think about.

With that caveat, my method works fine on both Firefox Desktop and Fennec. I'd continue with implementing a way to set overrides for specific domains/URIs if everyone agrees that's the method we can continue using.

[0]: https://developer.mozilla.org/en-US/docs/Observer_Notifications
[1]: https://dxr.mozilla.org/mozilla-central/rev/fe895421dfbe1f1f8f1fc6a39bb20774423a6d74/netwerk/protocol/http/UserAgentOverrides.jsm#49
[2]: https://dxr.mozilla.org/mozilla-central/rev/fe895421dfbe1f1f8f1fc6a39bb20774423a6d74/netwerk/protocol/http/nsHttpChannel.cpp#7805

Comment 2

[Dennis Schubert [:denschub]]

ni?'ing Eric as well.

Comment 3

[Eric Tsai (no more review request accepted)]

Please refer to my comment on github :)

Comment 4

[Dennis Schubert [:denschub]]

Attachment: PR for actual override implementations

So, this PR contains more than a prototype, it could actually be used in a shipping version of our add-on if everyone agrees, but I guess this bug is still fine. Sorry it took way longer than expected, but some issues I did not expect showed up. For the sake of having them documented, I'm going to write them down here (should also make reviewing a bit easier).

1. It is surprisingly complicated to have code in a system extension and also build XPIs to update out of the release cycle. That's mainly caused by the fact I have to use a jar manifest (jar.mn) to get all the files embedded in a m-c build (otherwise, we'd have to specify all files manually in the moz.build, which is somewhat painful). In an XPI however, a chrome.manifest is used. Luckly, it's somewhat easy to generate that from an existing jar.mn, so I've written some code for that.
2. The handler for NS_HTTP_ON_USERAGENT_REQUEST_TOPIC needs to... block until we're done. I initially planned on using an IndexedDB provided by the addon-sdk for improved lookup performance, but we don't have async functions yet (that'd be bug 1185106) and all other methods of pseudo-locking either don't work here or look way to ugly to publish. If we don't block the observer, we might be too late with assigning the new User-Agent header, the request might already be started. Not a situation we want to be in...

Because of point 2, I've now implemented lookups based on a simple object, with an added per-URI cache based on a map. I don't think we're able to cache anything on a per-domain base, since we want to be able to overwrite the headers for specific urls. Overall, this is very similar to the current UserAgentOverrides.jsm with more flexibility. I also was not able to find any significant performance impacts on sites we want to override, and it's performing only a base domain lookup and one object lookup for sites we don't override, so that should be fine.

Mike, I decided not to implement the search/replace feature that's currently implemented for UA overrides. I found it rather hard to understand and the implementation is not that pretty. Instead, I decided to have a function per override rule that can return whatever it wants to. Also, there is a function per rule to decide if we care about the current URL or not. Looks and feels pretty good to me. I've written a bit of documentation on [0] and I'd greatly appreciate any feedback on this. Do you think this is fine? Did I miss any requirements?

Eric, again asking you for review. Please also ping me if you've found anything hard to read, but this should be fairly easy to review, I hope. I've also written some very basic tests, so there are some unrelated changes in this PR, but it's not too bad. Sorry for that.

[0]: https://github.com/denschub/webcompat-addon/blob/6d7e2c3a702b6b7a5c4f56ca5a39b69ca0dc0c35/src/content/data/ua_overrides.js#L5

Comment 5

[Mike Taylor [:miketaylr]]

> So, this PR contains more than a prototype, it could actually be used in a shipping version of our add-on if everyone agrees, but I guess this bug is still fine.

We can change the title to "Create UA override APIs for webcompat go faster" or something. :)

Comment 6

[Mike Taylor [:miketaylr]]

> Instead, I decided to have a function per override rule that can return whatever it wants to. Also, there is a function per rule to decide if we care about the current URL or not. Looks and feels pretty good to me. I've written a bit of documentation on [0] and I'd greatly appreciate any feedback on this. Do you think this is fine?

On the face of it, SGTM (and potentially way less confusing, IMO. I have to consult my notes on the format every time I do a "regular" ua override).

Comment 7

[Eric Tsai (no more review request accepted)]

Comment on attachment 8795640 [details] [review]
PR for actual override implementations

The patch works fine, another question from the addon requirement page, will we have another patch for these two requirements?

> Ability for developers to see that a site patch is enabled, with short explanation and link to bug (likely via developer console).
> Ability to temporarily disable site patch for developers to test fixes.

Comment 8

[Dennis Schubert [:denschub]]

We definitely will have another patch. Site patching requires much more work and thorough planning before we can start implementing anything, while UA overrides are a really basic thing we're already doing these days, which is why this is prioritized at the moment. I added an item to our topics bank for the upcoming Taipei.

Thanks for checking!

Comment 9

[Eric Tsai (no more review request accepted)]

Comment on attachment 8795640 [details] [review]
PR for actual override implementations

Please file a follow-up bug for developer console notification and temporarily disable site fix

Comment 10

[Eric Tsai (no more review request accepted)]

(In reply to Eric Tsai from comment #9)
> Comment on attachment 8795640 [details] [review]
> PR for actual override implementations
> 
> Please file a follow-up bug for developer console notification and
> temporarily disable site fix

Another follow-up for XPCOMUtils.defineLazyModuleGetter in Mike's comment.

Comment 11

[Dennis Schubert [:denschub]]

Thanks for the great review feedback, Eric. I've merged the PR, so this issues is FIXED. While we technically could ship and use this, it's probably best to wait until we have testing infrastructure in place to verify the need of overrides.