986841 - Lazy load modules in gDevTools.jsm

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Lazy-loading Promise.jsm is straightforward since we don't use it in the top-level script, but for ProfilerController we should just require() it at the point of use as the sdk loader always loads eagerly.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Lazy load modules as much as possible in gDevTools.jsm

This passes all tests locally.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=6483bd320ba3

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

There is a failing test (browser_toolbox_options.js) in the try run. Working on a fix for that before landing.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Lazy load modules as much as possible in gDevTools.jsm v2

Fixing that test proved harder than I expected, but totally worthwhile. The problem was that moving the ProfilerController loading out of gDevTools.jsm global scope exposed a race in the devtools loader, which would now load main.js twice in the browser_toolbox_options.js test. If main.js didn't have any side effects this might have been OK, albeit a little slower, but among other things it registers the default tools in the toolbox. The second time registerTools was called, it would unfortunately treat the default tools like add-ons, breaking the test.

To get a better understanding of the race, here is the stack trace for the first attempt to load main.js:

load@resource://gre/modules/commonjs/toolkit/loader.js:270:3
main@resource://gre/modules/commonjs/toolkit/loader.js:649:3
DevToolsLoader.prototype.main@resource://gre/modules/devtools/Loader.jsm:285:5
DevToolsLoader.prototype.setProvider@resource://gre/modules/devtools/Loader.jsm:314:7
DevToolsLoader.prototype._chooseProvider@resource://gre/modules/devtools/Loader.jsm:325:7
DevToolsLoader.prototype.provider@resource://gre/modules/devtools/Loader.jsm:242:7
DevToolsLoader.prototype.main@resource://gre/modules/devtools/Loader.jsm:285:5
@resource:///modules/devtools/gDevTools.jsm:932:1
XPCU_moduleLambda@resource://gre/modules/XPCOMUtils.jsm:226:7
XPCU_defineLazyGetter/<.get@resource://gre/modules/XPCOMUtils.jsm:177:9
gBrowserInit._delayedStartup@chrome://browser/content/browser.js:10841:5

And here is the stack from the second attempt:

load@resource://gre/modules/commonjs/toolkit/loader.js:270:3
main@resource://gre/modules/commonjs/toolkit/loader.js:649:3
DevToolsLoader.prototype.main@resource://gre/modules/devtools/Loader.jsm:285:5
@resource:///modules/devtools/gDevTools.jsm:932:1
XPCU_moduleLambda@resource://gre/modules/XPCOMUtils.jsm:226:7
XPCU_defineLazyGetter/<.get@resource://gre/modules/XPCOMUtils.jsm:177:9
gBrowserInit._delayedStartup@chrome://browser/content/browser.js:10841:5

The change I made to Loader.jsm in this patch is to bail if main() has been already called.

Try: https://tbpl.mozilla.org/?tree=Try&rev=89e7dd956b97

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Miscellaneous lazification and cleanups in devtools framework code

Here are a bunch of cleanups and minor performance improvements that I made while digging into the aforementioned test failure. I think reasonable minds can disagree on what constitutes good taste in some of these cases and it's not necessary for fixing this bug, so feel free to dump it if you want.

Try: https://tbpl.mozilla.org/?tree=Try&rev=388424d622e1

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/08873449ec1e
https://hg.mozilla.org/integration/fx-team/rev/28ac5b562d9f

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08873449ec1e
https://hg.mozilla.org/mozilla-central/rev/28ac5b562d9f