1264625 - Fix "would run" warning fired when opening the browser toolbox

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

That, to prevent risk of stepping into debugger/actors internals.
This warning means that we run some code that is not invisible to debugger while the debugger is active (onNewScript, onNewGlobal or breakpoint callbacks).

There is various dependencies, mostly JSM that we should not use in the context of the browser toolbox.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Comment 2

[Eddy Bruel [:ejpbruel]]

How is this bug different from bug 1263935?

Comment 3

[Alexandre Poirot [:ochameau]]

Bug 1263935 is focusing on the AddonManager warning which is very precise.
This bug is about the various other internals we call, especially XPCOMUtils and the lazy getter helpers.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Forgot to 'git add'...

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8743020 [details] [diff] [review]
patch v2

Review of attachment 8743020 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/shared/Loader.jsm
@@ +47,5 @@
> +      "source-map": "resource://devtools/shared/sourcemap/source-map.js",
> +      // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠
> +      // Allow access to xpcshell test items from the loader.
> +      "xpcshell-test": "resource://test",
> +    };

Moving `paths` over here prevent having to copy this dictionnary.

@@ -162,5 @@
>    this.require = this.require.bind(this);
> -  this.lazyGetter = XPCOMUtils.defineLazyGetter.bind(XPCOMUtils);
> -  this.lazyImporter = XPCOMUtils.defineLazyModuleGetter.bind(XPCOMUtils);
> -  this.lazyServiceGetter = XPCOMUtils.defineLazyServiceGetter.bind(XPCOMUtils);
> -  this.lazyRequireGetter = this.lazyRequireGetter.bind(this);

I have to move the definition of the lazy getter in loader modules.
We basicaly can't use jsm because jsm are not flagged as invisible to debugger.

@@ +152,5 @@
> +    for (let id in modules) {
> +      let exports = modules[id];
> +      let uri = resolveURI(id, loader.mapping);
> +      loader.modules[uri] = { exports };
> +    }

Using a module to define pseudo modules and globals is something done in the SDK.
Like here, for pseudo modules:
  http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/addon/runner.js#82
And globals:
  http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/addon/runner.js#67

Comment 6

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=915ddedafcaf

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Comment 8

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d75f3065edb2

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8746011 [details] [diff] [review]
patch v3

Review of attachment 8746011 [details] [diff] [review]:
-----------------------------------------------------------------

See comment 5 for more info about this patch.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8746011 [details] [diff] [review]
patch v3

Review of attachment 8746011 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I think this makes sense, just a few questions.

Will browser-loader work correctly with these changes?  It particular it:

* Pulls some options from the "default" loader[1]
* Defines some of the same lazy methods[2]

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/browser-loader.js#79
[2]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/browser-loader.js#127-132

::: devtools/shared/builtin-modules.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +/**
> + * This module defines custom globals injected in all our modules

Nit: seems wrapped strangely, rewrap to 80 chars

@@ +5,5 @@
> +"use strict";
> +
> +/**
> + * This module defines custom globals injected in all our modules
> + * and also pseudo modules that aren't js file but just dynamically set values.

Nit: maybe "aren't separate files"?

@@ +17,5 @@
> +const { Loader } = Cu.import("resource://gre/modules/commonjs/toolkit/loader.js", {});
> +const promise = Cu.import("resource://gre/modules/Promise.jsm", {}).Promise;
> +const jsmScope = Cu.import("resource://gre/modules/Services.jsm", {});
> +const { Services } = jsmScope;
> +// Steal various global only available in JSM scope (and not Sandbox one)

Nit: globals

@@ +149,5 @@
> +      // a proxy whose defineProperty handler might unwittingly trigger this
> +      // getter again.
> +      delete obj[property];
> +      let value = destructure
> +        ? require(module)[property]

Will this end up being the "correct" require here, even in the case of browser-loader.js?

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> Comment on attachment 8746011 [details] [diff] [review]
> patch v3
> 
> Review of attachment 8746011 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall I think this makes sense, just a few questions.
> 
> Will browser-loader work correctly with these changes?  It particular it:
> 
> * Pulls some options from the "default" loader[1]

It pulls `paths` and `invilibleToDebugger` which are still exposed. (`modules` and `globals` are no longer passed, but browser-loader doesn't use them)

> * Defines some of the same lazy methods[2]

Good catch.
I imagine builtin-modules.js would have overloaded all that. But it shouldn't be loaded in browser-loader Loader instance (it is only loaded for main Loader, in, Loader.jsm).
https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/browser-loader.js#106-133
But I'm wondering if browser-loader should also use builtin-modules the same way we use it in Loader.jsm? It would work for everything but the `console` object, which should be different.
We could use a condition in builtin-modules to define console accordingly, but at the end, would it be misleading or clearer??

> @@ +149,5 @@
> > +      // a proxy whose defineProperty handler might unwittingly trigger this
> > +      // getter again.
> > +      delete obj[property];
> > +      let value = destructure
> > +        ? require(module)[property]
> 
> Will this end up being the "correct" require here, even in the case of
> browser-loader.js?

In the current patch, builtin-modules isn't used in browser-loader. Instead we end up using the lazy getter defined browser-loader and its version of lazyRequireGetter which should work.
If as said previously, we make it so that browser-loader also uses builtin-modules, the require() here would be the expected one.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> > * Defines some of the same lazy methods[2]
> 
> Good catch.
> I imagine builtin-modules.js would have overloaded all that. But it
> shouldn't be loaded in browser-loader Loader instance (it is only loaded for
> main Loader, in, Loader.jsm).
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/
> browser-loader.js#106-133
> But I'm wondering if browser-loader should also use builtin-modules the same
> way we use it in Loader.jsm? It would work for everything but the `console`
> object, which should be different.
> We could use a condition in builtin-modules to define console accordingly,
> but at the end, would it be misleading or clearer??

Okay, it sounds like browser-loader is safe as is then?  Probably easier to leave it alone for now, and revise later if we decide we want the same technique there.

It's less important for this issue since it's not the loader used by the server.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8746011 [details] [diff] [review]
patch v3

Review of attachment 8746011 [details] [diff] [review]:
-----------------------------------------------------------------

Since it sounds like browser-loader is safe, r+ assuming other nits are fixed.

Comment 14

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7732d78ce9d

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

Fixed the nits.

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Fix xpcshell test.

Comment 17

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3f2fe660991

Comment 18

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/9d54c99aa3adcb26d174a18bdedff704047066a9
Bug 1264625 - Ensure using only invisible modules when opening the browser toolbox. r=jryans

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d54c99aa3ad