Closed Bug 1434543 Opened 6 years ago Closed 6 years ago

GCLI listen no longer works "ChromeUtils is not defined"

Categories

(DevTools :: General, defect, P2)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(2 files)

Bug 1434543 - use require to load DevToolsLoader in gcli/listen.js;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1434543 - removed unused getElementFromPoint from devtools layout/utils;
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
Regression from Bug 1431533
What about exposing ChromeUtils from our sandbox? This would help with Bug 1434374, but I might be overlooking other impacts here.
See Also: → 1434374
Comment on attachment 8947013 [details]
Bug 1434543 - use require to load DevToolsLoader in gcli/listen.js;

https://reviewboard.mozilla.org/r/216836/#review222646

I don't

::: devtools/shared/base-loader.js:153
(Diff revision 1)
>      sandboxName: options.name,
>      sandboxPrototype: "prototype" in options ? options.prototype : {},
>      invisibleToDebugger: "invisibleToDebugger" in options ?
>                           options.invisibleToDebugger : false,
> -    waiveInterposition: false
> +    waiveInterposition: false,
> +    wantGlobalProperties: ["ChromeUtils"],

We are using builtin-modules.js to expose special globals like this:
https://searchfox.org/mozilla-central/source/devtools/shared/builtin-modules.js#214


Also in most of the modules we do require it manually:
https://searchfox.org/mozilla-central/search?q=ChromeUtils&case=false&regexp=false&path=devtools%2F
Everywhere but layout/utils.js and heapsnapshot.

This ChromeUtils module is set over here:
https://searchfox.org/mozilla-central/source/devtools/shared/builtin-modules.js#18-31


So we should either require ChromeUtils manually for gcli (and probably layout/utils and heapsnapshot.
Or, get rid of this special ChromeUtils modules and depends on it being in global scope.

I'm slightly in favor of explicit require of it as we try to limit the global to whatever is available in a web page context. There is some exception to that, like `loader`. I think Kris idea from bug 1434374 to use require() to pull the jsm makes sense. Then only the base-loader will use ChromeUtils to load modules.
Attachment #8947013 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Comment on attachment 8947013 [details]
> Bug 1434543 - expose ChromeUtils in base-loader sandbox;
> 
> https://reviewboard.mozilla.org/r/216836/#review222646
> 
> I don't

Well, I

> 
> ::: devtools/shared/base-loader.js:153
> (Diff revision 1)
> >      sandboxName: options.name,
> >      sandboxPrototype: "prototype" in options ? options.prototype : {},
> >      invisibleToDebugger: "invisibleToDebugger" in options ?
> >                           options.invisibleToDebugger : false,
> > -    waiveInterposition: false
> > +    waiveInterposition: false,
> > +    wantGlobalProperties: ["ChromeUtils"],
> 
> We are using builtin-modules.js to expose special globals like this:
> https://searchfox.org/mozilla-central/source/devtools/shared/builtin-modules.
> js#214
> 
> 
> Also in most of the modules we do require it manually:
> https://searchfox.org/mozilla-central/
> search?q=ChromeUtils&case=false&regexp=false&path=devtools%2F
> Everywhere but layout/utils.js and heapsnapshot.
> 
> This ChromeUtils module is set over here:
> https://searchfox.org/mozilla-central/source/devtools/shared/builtin-modules.
> js#18-31
> 
> 
> So we should either require ChromeUtils manually for gcli (and probably
> layout/utils and heapsnapshot.

Here we can directly require DevToolsLoader without having to get ChromeUtils first, right?

layout/utils: I assume you mean the content of getElementFromPoint() which is only retrieved in test-actor.js but not actually used. We should either remove this code or actually test it + fix it. ChromeUtils was introduced in this file by a previous mass refactor, but since it's not tested the problem was not detected.

heapsnapshot: are you thinking about https://searchfox.org/mozilla-central/source/devtools/shared/heapsnapshot/HeapAnalysesWorker.js ? It's loaded as ChromeWorker so I assume it has access to ChromeUtils globally? 

> Or, get rid of this special ChromeUtils modules and depends on it being in
> global scope.
> 
> I'm slightly in favor of explicit require of it as we try to limit the
> global to whatever is available in a web page context. There is some
> exception to that, like `loader`. I think Kris idea from bug 1434374 to use
> require() to pull the jsm makes sense. Then only the base-loader will use
> ChromeUtils to load modules.

As well as places where we use Cu.import to get { require } ?
https://searchfox.org/mozilla-central/search?q=require+%7D+%3D+Cu.import(&case=false&regexp=false&path=devtools%2F
Comment on attachment 8947013 [details]
Bug 1434543 - use require to load DevToolsLoader in gcli/listen.js;

https://reviewboard.mozilla.org/r/216836/#review222676

Looks good,

::: devtools/shared/gcli/commands/listen.js:24
(Diff revision 2)
>    // a separate instance of the DebuggingServer from the rest of the
>    // devtools.  This allows us to safely use the tools against even the
>    // actors and DebuggingServer itself, especially since we can mark
>    // serverLoader as invisible to the debugger (unlike the usual loader
>    // settings).
> +  const { DevToolsLoader } = require("resource://devtools/shared/Loader.jsm");

Note that this jsm is going to be already loaded anyway, so you can keep that in the module's header.
Attachment #8947013 - Flags: review?(poirot.alex) → review+
Comment on attachment 8947047 [details]
Bug 1434543 - removed unused getElementFromPoint from devtools layout/utils;

https://reviewboard.mozilla.org/r/216846/#review222678

thanks!
Attachment #8947047 - Flags: review?(poirot.alex) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9195839434f
use require to load DevToolsLoader in gcli/listen.js;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/38b9e1c864b4
removed unused getElementFromPoint from devtools layout/utils;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/b9195839434f
https://hg.mozilla.org/mozilla-central/rev/38b9e1c864b4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: