There is some client modules being loaded from server. We end up loading client modules twice because of this. It is bad for memory but also for the state of each modules which can end up being confused. We should avoid loading client resources from server in first place, but at least we should use the client module loader to pull them.

See comment 0.

Fixed eslint.

Comment on attachment 8725167 [details] [diff] [review] patch v2 Review of attachment 8725167 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, I don't think I have enough context. The GCLI comamnds are loaded by both the GCLI actor and the developer toolbar. Are you saying they are each using separate loaders? Tell me more about the paths that caused different loaders to be used.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3) > Comment on attachment 8725167 [details] [diff] [review] > patch v2 > > Review of attachment 8725167 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hmm, I don't think I have enough context. > > The GCLI comamnds are loaded by both the GCLI actor and the developer > toolbar. Are you saying they are each using separate loaders? Oh right. These modules ended up being loaded from the browser toolbox actors. That's why I ended up disabling gcli for the browser toolbox in bug 1252245. I'm not sure the gcli function were actually called. I think the exception I hit was just a require being done in a header. Like debugger-commands.js or csscoverage.js. It ended up loading devtools.js multiple times and register the about:devtools-toolbox protocol more than once (since then, I tweaked the procotol module to only register if not already registered, but that's a workaround.). We do have two loaders. The once instanciated by devtools-startup and any other code using Loader.jsm simply by calling its require symbol or loader.main(). We do reuse this same loader for common debugger server, for tabs in parent process. We do instanciate another one in child process, and any of these codepath will be broken, but that's another topic! The codepath that fails is related to the server we create for the browser toolbox. We do instanciate it in its own loader, required to set invisibleToDebugger=true. This end up loading actors and gcli commands in another loader and we instanciate another instance of devtools.js. So at first place, these calls from the content process of the browser toolbox are meant to fail. the gDevTools isn't going to work in child and isn't the one we expect in browser toolbox, I think. So we should at least ensure lazy loading these gDevTools. And may be at a check within devtools.js to somehow check we are loaded in the right loader?

(In reply to Alexandre Poirot [:ochameau] PTO, back on 14th from comment #4) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3) > > Comment on attachment 8725167 [details] [diff] [review] > > patch v2 > > > > Review of attachment 8725167 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Hmm, I don't think I have enough context. > > > > The GCLI comamnds are loaded by both the GCLI actor and the developer > > toolbar. Are you saying they are each using separate loaders? > > Oh right. These modules ended up being loaded from the browser toolbox > actors. That's why I ended up disabling gcli for the browser toolbox in bug > 1252245. I'm not sure the gcli function were actually called. I think the > exception I hit was just a require being done in a header. > Like debugger-commands.js or csscoverage.js. > It ended up loading devtools.js multiple times and register the > about:devtools-toolbox protocol more than once (since then, I tweaked the > procotol module to only register if not already registered, but that's a > workaround.). What happens is that the toolbox talks to the GCLI actor to enumerate the toolbox command buttons it should show. But GCLI's startup path loads all the GCLI commands[1]. It does this because the GCLI actor wants to give[2] the client a list of every command it could possibly run, and that infomation is metadata inside the command modules, so they all need to be loaded. I am sure this GCLI design could be made more efficient / less eager loading. :) But doing that probably isn't a good use of time at the moment. [1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/index.js [2]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/gcli.js#95 > We do have two loaders. The once instanciated by devtools-startup and any > other code using Loader.jsm simply by calling its require symbol or > loader.main(). We do reuse this same loader for common debugger server, for > tabs in parent process. We do instanciate another one in child process, and > any of these codepath will be broken, but that's another topic! > The codepath that fails is related to the server we create for the browser > toolbox. We do instanciate it in its own loader, required to set > invisibleToDebugger=true. > This end up loading actors and gcli commands in another loader and we > instanciate another instance of devtools.js. Okay, so far all of this makes sense to me, and seems correct. We need the separate loaders to implement browser loader in its current form. > So at first place, these calls from the content process of the browser > toolbox are meant to fail. the gDevTools isn't going to work in child and > isn't the one we expect in browser toolbox, I think. > So we should at least ensure lazy loading these gDevTools. And may be at a > check within devtools.js to somehow check we are loaded in the right loader? The browser toolbox is not really a "content process" exactly, it's a totally separate Firefox, with UI code running that main / only process of this other separate Firefox. (Maybe that's what you meant, unsure when reusing terminology...) Anyway, I think we are agreeing that calling gDevTools from Browser Toolbox does not make sense. So yes, I think the simplest thing is for these GCLI command files to lazy load gDevTools. The actual commands that use it should not actually get run over there, since they are `runAt: "client"` commands and there is no Developer Toolbar in which to run them. So, I don't think we need import a loader in these change, but only make gDevTools lazy with the existing loader.

Just do lazy loading. (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5) > I am sure this GCLI design could be made more efficient / less eager > loading. :) But doing that probably isn't a good use of time at the moment. +1 > > So at first place, these calls from the content process of the browser > > toolbox are meant to fail. the gDevTools isn't going to work in child and > > isn't the one we expect in browser toolbox, I think. > > So we should at least ensure lazy loading these gDevTools. And may be at a > > check within devtools.js to somehow check we are loaded in the right loader? > > The browser toolbox is not really a "content process" exactly, it's a > totally separate Firefox, with UI code running that main / only process of > this other separate Firefox. (Maybe that's what you meant, unsure when > reusing terminology...) The issue doesn't happen in the browser toolbox process. It happens in the original firefox process. Issues happens in the DebuggerServer instance used for the browser toolbox. It lives in original firefox process, with the gcli actors. It is being instanciated over here: http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm#123 With its own Loader instance and it allows to have another set of module instances. Which is exactly what we want, but not necessarely for client modules like devtools.js. > Anyway, I think we are agreeing that calling gDevTools from Browser Toolbox > does not make sense. Yes. > So yes, I think the simplest thing is for these GCLI > command files to lazy load gDevTools. So I did that for all callsites but one: devtools/shared/gcli/commands/index.js Which is using client modules directly for its toplevel scope. http://mxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/index.js#85 http://mxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/index.js#99 It is somewhat wrong to keep that as-is, but I ended up ensure we do not call any gcli things in the browser toolbox to work around that, in bug 1252245.

https://hg.mozilla.org/integration/fx-team/rev/0a13cd48daad34bc365679d26f4998dbc0c994d4 Bug 1252247 - Load devtools/clients resources lazily when used from devtools/server. r=jryans