Closed Bug 1371335 Opened 7 years ago Closed 7 years ago

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

Categories

(Web Compatibility :: Tooling & Investigations, enhancement)

Product:
Web Compatibility
Component:
Tooling & Investigations
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: ehsan.akhgari, Assigned: miketaylr)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1371335. Part 1 - Lazily load TabListener module.
59 bytes, text/x-review-board-request
Gijs
: review+
Gijs
: feedback+
Details
Bug 1371335. Part 2 - Don't init WebCompatReporter add-on until after browser-delayed-startup-finished.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1371335. Part 3 - Add TabListener.jsm to browser_startup tests.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1371335. Part 4 - Remove pref observer during bootstrap shutdown().
59 bytes, text/x-review-board-request
florian
: review+
Details 
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
ooh, that seems bad.

Ehsan, are there any relevant bugs I can look at for pointers on how to defer loading?
Flags: needinfo?(ehsan)
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)
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)
Blocks: 1371442
Assignee: nobody → miket
Component: Go Faster → General
No longer blocks: 1371442
Attachment #8881933 - Flags: review?(gijskruitbosch+bugs)
Attachment #8881934 - Flags: review?(gijskruitbosch+bugs)
Attachment #8881935 - Flags: review?(gijskruitbosch+bugs)
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 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)
Attachment #8881934 - Flags: review?(mconley)
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 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 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-
Sweet, thanks for the pointers Mike and Gijs.
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 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?
(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. >_>
Attachment #8881934 - Flags: review?(gijskruitbosch+bugs)
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-
Thanks for the review feedback!
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 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+
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 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 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+
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
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 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 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 on attachment 8881933 [details]
Bug 1371335. Part 1 - Lazily load TabListener module.

Nice, thanks!
Attachment #8881933 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
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.

Attachment

General

Creator:
Created:
Updated:
Size: