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

RESOLVED FIXED

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: miketaylr)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
ooh, that seems bad.

Ehsan, are there any relevant bugs I can look at for pointers on how to defer loading?
Flags: needinfo?(ehsan)
(Reporter)

Comment 2

2 years ago
https://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm#35 can be made a lazy getter using XPCOMUtils.defineLazyModuleGetter().  You should be able to find piles of examples for it.

https://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/browser/extensions/webcompat/bootstrap.js#13 is already a lazy getter, so some code is accessing the UAOverrider object too early.  Judging based on the names of the functions in question that's probably https://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/browser/extensions/webcompat/bootstrap.js#63.  You'll need to find a way to defer this work to later.

But before you do that you should first ask a more important question: does this code have a good raison d'etre?  As far as I can tell, it's just duplicating the existing Gecko's functionality: https://searchfox.org/mozilla-central/source/netwerk/protocol/http/UserAgentOverrides.jsm  If my understanding is correct, the desirable remedy for this code would be to delete it and use Necko's support for overriding UA strings which already works and is well tested.
Flags: needinfo?(ehsan)
(Assignee)

Comment 3

2 years ago
Thanks for the pointers -- let me pass off the question about overrides to Dennis, he owns the ua_overrider code (the "webcompat" and "webcompat-reporter" are different,  though similarly named addons :/).
Flags: needinfo?(dschubert)
> If my understanding is correct, the desirable remedy for this code would be to delete it and use Necko's support for overriding UA strings which already works and is well tested.

In bug 896114, loading the UserAgentOverrides on desktop was disabled for performance reasons. We reimplemented the logic with some added filtering so we don't iterate over functions and call them if we know the domain is not of interest for us, which keeps the performance up for sites we don't want to touch. So, as far as my knowledge goes, UserAgentOverrides.jsm is only used on Fennec at the moment. Our initial plan was to eventually remove the code for fennec as well as soon as we have our GoFaster addon running there.

Although, to be fair, I have not checked recently if UserAgentOverrides.jsm is back in desktop, which indeed would render our code redundant. Will check! Also thanks for the hint, will work on the startup as well.
Flags: needinfo?(dschubert)
(Assignee)

Updated

2 years ago
Blocks: 1371442
(Assignee)

Updated

2 years ago
Assignee: nobody → miket
Component: Go Faster → General
(Assignee)

Updated

2 years ago
No longer blocks: 1371442
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8881933 - Flags: review?(gijskruitbosch+bugs)
Attachment #8881934 - Flags: review?(gijskruitbosch+bugs)
Attachment #8881935 - Flags: review?(gijskruitbosch+bugs)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8881933 [details]
Bug 1371335. Part 1 - Lazily load TabListener module.

https://reviewboard.mozilla.org/r/153002/#review158206

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm
(Diff revision 1)
>          .getInterface(Ci.nsIDOMWindowUtils)
>          .removeSheet(wcStyleURI, this._sheetType);
>      }
>  
>      CustomizableUI.removeListener(this);
> -    Cu.unload(TABLISTENER_JSM);

You probably still want to do this. You can use Cu.isModuleLoaded() to determine if anyone loaded the TabListener module.
Attachment #8881933 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 9

2 years ago
mozreview-review
Comment on attachment 8881934 [details]
Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished.

https://reviewboard.mozilla.org/r/153004/#review158220

::: browser/extensions/webcompat-reporter/bootstrap.js:44
(Diff revision 1)
> -  if (enabled) {
> +    if (enabled) {
> -    WebCompatReporter.init();
> +      WebCompatReporter.init();
> -  }
> +    }
> +  };
> +
> +  mm.addMessageListener("Browser:FirstPaint", initOnFirstPaint);

I'm not sure this is the right thing... I suspect we want to use browser-delayed-startup-finished or something, though I'm not sure how that would work if you updated the add-on while Firefox was running.

Mike C., do you have suggestions here?
Attachment #8881934 - Flags: review?(gijskruitbosch+bugs)

Updated

2 years ago
Attachment #8881934 - Flags: review?(mconley)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8881935 [details]
Bug 1371335. Part 3 - Add TabListener.jsm to browser_startup tests.

https://reviewboard.mozilla.org/r/153006/#review158224

rs=me
Attachment #8881935 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8881934 [details]
Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished.

https://reviewboard.mozilla.org/r/153004/#review158220

> I'm not sure this is the right thing... I suspect we want to use browser-delayed-startup-finished or something, though I'm not sure how that would work if you updated the add-on while Firefox was running.
> 
> Mike C., do you have suggestions here?

I agree that waiting until the first window has painted is the right choice, and the browser-delayed-startup-finished notification is probably the right thing to listen for.

If the situation that Gijs mentions arises, you can always check that the most recent window has painted first. We do something similar here:

http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/browser/base/content/browser.js#1168-1182

Comment 12

2 years ago
mozreview-review
Comment on attachment 8881934 [details]
Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished.

https://reviewboard.mozilla.org/r/153004/#review158256

Thanks for the patch! r-'ing though, because I agree with Gijs - we should wait for the browser-delayed-startup-finished observer notification.
Attachment #8881934 - Flags: review?(mconley) → review-
(Assignee)

Comment 13

2 years ago
Sweet, thanks for the pointers Mike and Gijs.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8881934 [details]
Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished.

https://reviewboard.mozilla.org/r/153004/#review160386

::: browser/extensions/webcompat-reporter/bootstrap.js:42
(Diff revision 2)
> +
>  function startup(aData, aReason) {
>    // Observe pref changes and enable/disable as necessary.
>    Services.prefs.addObserver(PREF_WC_REPORTER_ENABLED, prefObserver);
>  
> -  // Only initialize if pref is enabled.
> +  // Only initialize if pref is enabled, after first paint.

Oops. Will update comment.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8881934 [details]
Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished.

https://reviewboard.mozilla.org/r/153004/#review160388

::: browser/extensions/webcompat-reporter/bootstrap.js:32
(Diff revision 3)
>  };
>  
> +let notificationObserver = {
> +  observe(aSubject, aTopic, aData) {
> +    if (aTopic === DELAYED_STARTUP_FINISHED) {
> +      WebCompatReporter.init();

How quickly after startup do we need this to happen? Would it be ok to schedule this using Services.tm.idleDispatchToMainThread?

::: browser/extensions/webcompat-reporter/bootstrap.js:45
(Diff revision 3)
>    Services.prefs.addObserver(PREF_WC_REPORTER_ENABLED, prefObserver);
>  
> -  // Only initialize if pref is enabled.
> +  // Only initialize if pref is enabled, after the delayed startup notification.
>    let enabled = Services.prefs.getBoolPref(PREF_WC_REPORTER_ENABLED);
>    if (enabled) {
> +    if (window.gBrowserInit.delayedStartupFinished) {

I think 'window' is undefined here. Have you tested this code?
(Assignee)

Comment 21

2 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> Comment on attachment 8881934 [details]
> Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after
> browser-delayed-startup-finished.
> 
> https://reviewboard.mozilla.org/r/153004/#review160388
> 
> ::: browser/extensions/webcompat-reporter/bootstrap.js:32
> (Diff revision 3)
> >  };
> >  
> > +let notificationObserver = {
> > +  observe(aSubject, aTopic, aData) {
> > +    if (aTopic === DELAYED_STARTUP_FINISHED) {
> > +      WebCompatReporter.init();
> 
> How quickly after startup do we need this to happen? Would it be ok to
> schedule this using Services.tm.idleDispatchToMainThread?

Not that quickly, I think. Most people are interacting with this addon only when they've encountered a busted page... 

Will look into idleDispatchToMainThread.
 
> ::: browser/extensions/webcompat-reporter/bootstrap.js:45
> (Diff revision 3)
> >    Services.prefs.addObserver(PREF_WC_REPORTER_ENABLED, prefObserver);
> >  
> > -  // Only initialize if pref is enabled.
> > +  // Only initialize if pref is enabled, after the delayed startup notification.
> >    let enabled = Services.prefs.getBoolPref(PREF_WC_REPORTER_ENABLED);
> >    if (enabled) {
> > +    if (window.gBrowserInit.delayedStartupFinished) {
> 
> I think 'window' is undefined here. Have you tested this code?

Uhh, you're right. >_>
(Assignee)

Updated

2 years ago
Attachment #8881934 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8881934 [details]
Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished.

https://reviewboard.mozilla.org/r/153004/#review160784

Almost there :-)

::: browser/extensions/webcompat-reporter/bootstrap.js:38
(Diff revision 4)
>      WebCompatReporter.uninit();
>    }
>  };
>  
> +let notificationObserver = {
> +  observe(aSubject, aTopic, aData) {

Please make this a simple function, and remove the switch/case stuff, we will only need it if we start observing multiple different notification topics.

::: browser/extensions/webcompat-reporter/bootstrap.js:55
(Diff revision 4)
>    Services.prefs.addObserver(PREF_WC_REPORTER_ENABLED, prefObserver);
>  
> -  // Only initialize if pref is enabled.
> +  // Only initialize if pref is enabled, after the delayed startup notification.
>    let enabled = Services.prefs.getBoolPref(PREF_WC_REPORTER_ENABLED);
>    if (enabled) {
> -    WebCompatReporter.init();
> +    let win = RecentWindow.getMostRecentBrowserWindow();

I would just use Services.wm.getMostRecentWindow("navigator:browser") here. RecentWindow.jsm isn't loaded yet at this point of startup, and it's probably not needed at all before first paint.

::: browser/extensions/webcompat-reporter/bootstrap.js:70
(Diff revision 4)
>  function shutdown(aData, aReason) {
>    if (aReason === APP_SHUTDOWN) {
>      return;
>    }
>  
>    Cu.unload(WEBCOMPATREPORTER_JSM);

not related to what this patch is trying to fix, but I think there's a missing Services.prefs.removeObserver(PREF_WC_REPORTER_ENABLED, prefObserver); here, which may cause the old version of this add-on to be leaked when updating.
Attachment #8881934 - Flags: review?(florian) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 28

2 years ago
Thanks for the review feedback!

Comment 29

2 years ago
mozreview-review
Comment on attachment 8881934 [details]
Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished.

https://reviewboard.mozilla.org/r/153004/#review160884

::: browser/extensions/webcompat-reporter/bootstrap.js:20
(Diff revision 5)
>  XPCOMUtils.defineLazyModuleGetter(this, "WebCompatReporter",
>    WEBCOMPATREPORTER_JSM);
>  
>  const PREF_WC_REPORTER_ENABLED = "extensions.webcompat-reporter.enabled";
>  
> +let requestReporterInit = () => {

This should be a simple function:
function requestReporterInit() {

(don't forget to remove the no longer needed ';')

::: browser/extensions/webcompat-reporter/bootstrap.js:26
(Diff revision 5)
> +  Services.tm.idleDispatchToMainThread(() => {
> +    WebCompatReporter.init();
> +  });
> +};
> +
>  let prefObserver = function(aSubject, aTopic, aData) {

nit: This could also become a simple function, but I'm not requiring it as it's not new code.

::: browser/extensions/webcompat-reporter/bootstrap.js:35
(Diff revision 5)
>    } else {
>      WebCompatReporter.uninit();
>    }
>  };
>  
> +let onDelayedStartupFinished = (subject, topic, data) => {

You don't need to access 'this' from within this function, so it should be a simple function:

function onDelayedStartupFinished(subject, topic, data) {

::: browser/extensions/webcompat-reporter/bootstrap.js:37
(Diff revision 5)
>    }
>  };
>  
> +let onDelayedStartupFinished = (subject, topic, data) => {
> +  requestReporterInit();
> +  Services.obs.removeObserver(this, DELAYED_STARTUP_FINISHED);

this doesn't point to the observer here, but likely to the global scope.

You want to removeObserver(onDelayedStartupFinished, ...
ie. call removeObserver with the same first parameter as you gave to addObserver.

::: browser/extensions/webcompat-reporter/bootstrap.js:38
(Diff revision 5)
>  };
>  
> +let onDelayedStartupFinished = (subject, topic, data) => {
> +  requestReporterInit();
> +  Services.obs.removeObserver(this, DELAYED_STARTUP_FINISHED);
> +};

when converting to a normal function, don't forget to remove the ';' here.
Attachment #8881934 - Flags: review?(florian) → review-

Comment 30

2 years ago
mozreview-review
Comment on attachment 8884958 [details]
Bug 1371335. Part 4 - Remove pref observer during bootstrap shutdown().

https://reviewboard.mozilla.org/r/155784/#review160888
Attachment #8884958 - Flags: review?(florian) → review+
(Assignee)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8881934 [details]
Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished.

https://reviewboard.mozilla.org/r/153004/#review160926

::: browser/extensions/webcompat-reporter/bootstrap.js:20
(Diff revision 5)
>  XPCOMUtils.defineLazyModuleGetter(this, "WebCompatReporter",
>    WEBCOMPATREPORTER_JSM);
>  
>  const PREF_WC_REPORTER_ENABLED = "extensions.webcompat-reporter.enabled";
>  
> +let requestReporterInit = () => {

Roger that.

::: browser/extensions/webcompat-reporter/bootstrap.js:37
(Diff revision 5)
>    }
>  };
>  
> +let onDelayedStartupFinished = (subject, topic, data) => {
> +  requestReporterInit();
> +  Services.obs.removeObserver(this, DELAYED_STARTUP_FINISHED);

Ah yeah. I'll just change this to a named function so this points to the right thing.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

2 years ago
mozreview-review
Comment on attachment 8881934 [details]
Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished.

https://reviewboard.mozilla.org/r/153004/#review160934

::: browser/extensions/webcompat-reporter/bootstrap.js:37
(Diff revision 6)
>    }
> -};
> +}
> +
> +function onDelayedStartupFinished(subject, topic, data) {
> +  requestReporterInit();
> +  Services.obs.removeObserver(this, DELAYED_STARTUP_FINISHED);

'this' still points to the global scope here. As I said before, you should removeObserver(onDelayedStartupFinished, ...
Attachment #8881934 - Flags: review?(florian) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 39

2 years ago
mozreview-review
Comment on attachment 8881934 [details]
Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished.

https://reviewboard.mozilla.org/r/153004/#review160948
Attachment #8881934 - Flags: review?(florian) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

2 years ago
Had some timeouts in browser_report_site_issue.js that was due to not re-importing the TabListener module in init, after it had been explicitly unloaded in uninit (which will happen if the pref is flipped to false, which browser_disabled_cleanup.js does just before this test).

So, just added the following in init, and pushed a new try run:

if (!Cu.isModuleLoaded(TABLISTENER_JSM)) {
  Cu.import(TABLISTENER_JSM);
}

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14eb7f09d68a16994b89912572f5ffb7bcd1fb2e
(Assignee)

Comment 45

2 years ago
Comment on attachment 8881933 [details]
Bug 1371335. Part 1 - Lazily load TabListener module.

Try is green for relevant tests. 

Gijs can you sanity check the changes described in Comment #44 before I land this, please?
Attachment #8881933 - Flags: feedback?(gijskruitbosch+bugs)

Comment 46

2 years ago
Comment on attachment 8881933 [details]
Bug 1371335. Part 1 - Lazily load TabListener module.

Discussed on IRC. This will result in importing the jsm on startup again, though patch 2 gets rid of that. We could either not take this part, or try to define the lazy getter in init() instead of in the global scope, so if init reruns it gets defined a second time.
Attachment #8881933 - Flags: feedback?(gijskruitbosch+bugs) → feedback-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 55

2 years ago
Comment on attachment 8881933 [details]
Bug 1371335. Part 1 - Lazily load TabListener module.

(sorry, i don't know how to clear a review on reviewboard, or re-request review on a previously r+'d patch...)

OK. I've defined the lazyModuleGetter inside init so the TabListener sits on the WebCompatReporter object, rather than the module global.

Browser Chrome Test Summary
	Passed: 11
	Failed: 0
	Todo: 0
	Mode: e10s
Attachment #8881933 - Flags: feedback?(gijskruitbosch+bugs)

Comment 56

2 years ago
Comment on attachment 8881933 [details]
Bug 1371335. Part 1 - Lazily load TabListener module.

Nice, thanks!
Attachment #8881933 - Flags: feedback?(gijskruitbosch+bugs) → feedback+

Comment 57

2 years ago
Pushed by mitaylor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0bb8e7c81cb6
Part 1 - Lazily load TabListener module. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d38dd453257f
Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished. r=florian
https://hg.mozilla.org/integration/autoland/rev/7059c615165a
Part 3 - Add TabListener.jsm to browser_startup tests. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/4f321d1ee0fd
Part 4 - Remove pref observer during bootstrap shutdown(). r=florian
You need to log in before you can comment on or make changes to this bug.