1371442 - webcompat gofaster shouldn't load anything before opening and painting the first browser window

Status: RESOLVED FIXED

(Web Compatibility :: Interventions, defect)

Description

[Mike Taylor [:miketaylr]]

I'm gonna spin out this bug for Dennis to work on the ua override stuff. I'll tackle webcompat-reporter in the original.

+++ This bug was initially created as a clone of Bug #1371335 +++

browser/base/content/test/performance/browser_startup.js indicates that the following scripts are loaded before opening the first window:

chrome://webcompat-reporter/content/TabListener.jsm
chrome://webcompat/content/lib/ua_overrider.jsm

Comment 1

[Dennis Schubert [:denschub]]

My assumption that `UserAgentOverrides.jsm` only works on Fenenc apparently now is invalid. I'm not going to trace the history of that back, but apparently, there was some work recently on even optimizing it, including 1363421. Replacing our custom implementation with calling out to `UserAgentOverrides.jsm` is not a big task and something I'll do right away before doing anything else.

That, however, does not solve the main issue. Initially, I was going for a solution to load it after we get the notification that a window has opened, however, that would mean adding new handlers for every new window. Clearly, that's not the right solution since the parent process processes these regardless of the window the request was made, and we will end up calling the same callback multiple times.

So, probably, we should listen to window openings, but check if we already initialized the callbacks and if so, don't do it again. Will spend some more brain on that.

Comment 2

[Eric Tsai (no more review request accepted)]

First idea is to use
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWindowMediator 
listen to window open event. Once window opened and we initial UAOverrider, then we remove listener.
If so, we don't need module lazy load now. Need to check if nsIWindowMediator works in e10s.

Comment 3

[Dennis Schubert [:denschub]]

Attachment: Listen to Browser:FirstPaint to initialize the UAOverrider

Eric, I created a patch that listens to `Browser:FirstPaint` to initialize the overrider. I remove the listener after the first call to ensure it we do not end up with multiple overrider instances. Other than that, it should be pretty straightforward code.

Comment 4

[Eric Tsai (no more review request accepted)]

Comment on attachment 8881545 [details] [review]
Listen to Browser:FirstPaint to initialize the UAOverrider

Wait patch using browser-delayed-startup-finished instead.

Comment 5

[Dennis Schubert [:denschub]]

mconley, in bug 1371335, you suggested listening for the `browser-delayed-startup-finished` notification instead of `Browser:FirstPaint`, which is what I am doing here how, see [0].

However, this notification seems to get out too soon. We do have a performance test at browser/base/content/test/performance/browser_startup.js, and when waiting on `browser-delayed-startup-finished`, it still complains about us initializing too soon:

> ./mach mochitest browser/base/content/test/performance/browser_startup.js | grep -i "webcompat"
> [...]
> 353 INFO modules loaded before handling user events: chrome://webcompat/content/data/ua_overrides.jsm
> 356 INFO modules loaded before handling user events: chrome://webcompat/content/lib/ua_overrider.jsm

It does not do that when we wait until `Browser:FirstPaint`. Do you have any idea/suggestion here?

[0]: https://github.com/denschub/webcompat-addon/blob/22ee9f033ab273540f917d4d2fae648bc8f2647c/src/bootstrap.js#L70

Comment 6

[Florian Quèze [:florian]]

It seems the code you are initializing will observe the "http-on-useragent-request" notification. Could you initialize it lazily by observing "http-on-useragent-request" from bootstrap.js, and then call overrider.observe from bootstrap for the first http request?

Comment 7

[Dennis Schubert [:denschub]]

We are no longer doing that, see [0]. Also, we will have more pieces of just overriding user agents there in the future, so that's probably not a workaround we could use and I feel like we should have a "proper" solution here. :/

[0]: https://github.com/mozilla/webcompat-addon/commit/fea6d8890bce407219a37ae9b803bb392cf18ea4

Comment 8

[Florian Quèze [:florian]]

So what is the code actually doing?

It still sounds to me that what you want is more related to when we do the first http request than when we are done loading the first browser window.

Maybe you want to send an observer notification from http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/netwerk/protocol/http/UserAgentOverrides.jsm#70 ?

Comment 9

[Dennis Schubert [:denschub]]

Don't get me wrong, at the moment, you are perfectly right and we could just simply initialize our stuff whenever the first http request is made. However, this feels wrong for some reasons.

Initializing an extension when we do an http request feels like something people would not expect to happen. Especially with the tight performance tests on requests, I would be really worried about regressing them just because we have to initialize our listeners here (the request would get blocked until we are done).

Adding a new notification to `UserAgentOverrides.jsm` to fire when it's done initializing could do the trick as well, yes. However, this still feels like a workaround as this will not suit us in the future since we will be adding more parts to the GoFaster addon soon (like, for example, overwriting certain JavaScript objects) and we'd end up having the same issue again, so it feels less than optimal to me.

As we are just setting up listeners to other events in our init code, I think it's not needed to have them getting initialized when the actual event happens, why can't we do that earlier?

I also do not think that I am the only one who will ever run into this issue (in fact, we already do have bug 1371335) and I don't want "add notifications everywhere you need them" to be a solution. :) Clearly, waiting for a notification like `browser-delayed-startup-finished` feels more right, since that could be used by anyone who runs into this issue in the future. However, this notification is clearly firing too early.

Comment 10

[Florian Quèze [:florian]]

(In reply to Dennis Schubert [:denschub] from comment #9)

> Adding a new notification to `UserAgentOverrides.jsm` to fire when it's done
> initializing could do the trick as well, yes. However, this still feels like
> a workaround 
[...]
> Clearly,
> waiting for a notification like `browser-delayed-startup-finished` feels
> more right, since that could be used by anyone who runs into this issue in
> the future. However, this notification is clearly firing too early.

Clearly, this shows we have different perspectives on this.

We are trying very hard to avoid initializing during startup anything that's not needed for startup, and to push initialization of things to the first time they are needed.

Adding a notification letting you do what you need the first time it's needed feels right to me. Adding stuff at a random point of startup doesn't feel right at all. browser-delayed-startup-finished is most likely not what you need. An http request could have started before that (if we load a user's home page). And in most of our startup tests and for new profiles, the first http request will happen seconds after that.

Your other example (overwriting JS objects in web pages) seem like you want to intercept the first load in a content process. Again, this has nothing to do with browser-delayed-startup-finished (or more generally anything in the chrome process relating to the browser window).

Comment 11

[Dennis Schubert [:denschub]]

Thanks for your explanations, I see where you're headed. :) I will look out for a better place to initialize our overrider, possibly notifying UserAgentOverrides.jsm. 

> Your other example (overwriting JS objects in web pages) seem like you want to intercept the first load in a content process. Again, this has nothing to do with browser-delayed-startup-finished (or more generally anything in the chrome process relating to the browser window).

Thanks, we'll consider that!

Comment 12

[Dennis Schubert [:denschub]]

Comment on attachment 8881545 [details] [review]
Listen to Browser:FirstPaint to initialize the UAOverrider

Eric, since bug 1383909 is in Nightly now, do you mind having a look at the PR again?

Comment 13

[Eric Tsai (no more review request accepted)]

Comment on attachment 8881545 [details] [review]
Listen to Browser:FirstPaint to initialize the UAOverrider

Looks good for me using the startupWatcher.

Comment 14

[Dennis Schubert [:denschub]]

PR merged, closing this issue. Landing will be tracked in bug 1376602.