1453712 - `flags.testing` is always false in content process

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement)

Description

[Alexandre Poirot [:ochameau]]

We set the testing flag from head file, like this:
https://searchfox.org/mozilla-central/source/devtools/client/shared/test/shared-head.js#112
  flags.testing = true;
  registerCleanupFunction(() => {
    flags.testing = false;
  });

But this is only modifying its value in the parent process.
We should switch to a preference in order to allow to correctly set this flag in the content processes as it can be used by actors.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1453712 - Use a preference to set the testing flag.

Review commit: https://reviewboard.mozilla.org/r/236122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236122/

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8967465 [details]
Bug 1453712 - Use a preference to set the testing flag.

https://reviewboard.mozilla.org/r/236122/#review241866

::: devtools/shared/flags.js:34
(Diff revision 1)
> -makeWritableFlag(exports, "wantVerbose");
> +makeWritableFlag(exports, "wantVerbose", "devtools.debugger.log.verbose");
>  
>  // When the testing flag is set, various behaviors may be altered from
>  // production mode, typically to enable easier testing or enhanced
>  // debugging.
> -makeWritableFlag(exports, "testing");
> +makeWritableFlag(exports, "testing", "devtools.testing");

The main modification is this file + server/main.js where I removed the pref logic as it is now managed here.

Comment 3

[Alexandre Poirot [:ochameau]]

An hopefully to be green try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=276931a57d409d103005921afdeb0aff07404137

Comment 4

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

Comment on attachment 8967465 [details]
Bug 1453712 - Use a preference to set the testing flag.

https://reviewboard.mozilla.org/r/236122/#review241974

Thanks for working on this! :) It seems like a very valuable improvement!

We may want to change what happens with workers, see more notes below.

::: devtools/client/memory/test/chrome/head.js:29
(Diff revision 1)
> -var flags = require("devtools/shared/flags");
> -flags.testing = true;
>  
> +Services.prefs.setBoolPref("devtools.testing", true);
> +SimpleTest.registerCleanupFunction(() => {
> +  Services.prefs.setBoolPref("devtools.testing", false);

Could we use `clearUserPref` on the cleanup paths?

::: devtools/client/shared/redux/middleware/test/head.js:12
(Diff revision 1)
>  "use strict";
>  
>  const { require } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});
> -const flags = require("devtools/shared/flags");
> +const Services = require("Services");
>  
> -flags.testing = true;
> +Services.prefs.setBoolPref("devtools.testing", true);

Hmm, can we remove this instance of it?  I would expect this file to just have helper functions.

::: devtools/shared/flags.js:12
(Diff revision 1)
>   * We cannot make a normal property writeable on `exports` because
>   * the module system freezes it.
>   */
> -function makeWritableFlag(exports, name) {
> -  let flag = false;
> +function makeWritableFlag(exports, name, pref) {
> +  let flag;
> +  // We don't have access to pref in worker and enable all logs by default

Hmm, why do you want to default logs on in workers...?  Logs (just the regular `log` one) are still disabled for many test directories because they too verbose.  `log.verbose` is even more over the top.

Doesn't this also mean workers would log all this even in production usage?  That's a lot of logging that I don't think we want.

Hmm, I guess you are copying what was in server/main.js.  But it seems like a terrible default for production use!

::: devtools/shared/flags.js:17
(Diff revision 1)
> +  // We don't have access to pref in worker and enable all logs by default
> +  if (isWorker) {
> +    flag = name != "testing";
> +  } else {
> +    flag = Services.prefs.getBoolPref(pref, false);
> +    Services.prefs.addObserver(pref, () => {

Do we need to remove this observer when the loader shuts down, or does it all get GCed correctly anyway?

::: devtools/shared/flags.js:34
(Diff revision 1)
> -makeWritableFlag(exports, "wantVerbose");
> +makeWritableFlag(exports, "wantVerbose", "devtools.debugger.log.verbose");
>  
>  // When the testing flag is set, various behaviors may be altered from
>  // production mode, typically to enable easier testing or enhanced
>  // debugging.
> -makeWritableFlag(exports, "testing");
> +makeWritableFlag(exports, "testing", "devtools.testing");

Since it's now a pref, should we set the pref in https://searchfox.org/mozilla-central/source/testing/profiles/prefs_general.js rather than setting it from various head files?

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8967465 [details]
Bug 1453712 - Use a preference to set the testing flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236122/diff/1-2/

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8967465 [details]
Bug 1453712 - Use a preference to set the testing flag.

https://reviewboard.mozilla.org/r/236122/#review242032


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/shared/flags.js:23
(Diff revision 2)
> +      flag = Services.prefs.getBoolPref(pref, false);
> +    };
> +    Services.prefs.addObserver(pref, prefObserver);
> +
> +    // Also listen for Loader unload to unregister the pref observer and prevent leaking
> +    let unloadObserver = function () {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> ::: devtools/client/memory/test/chrome/head.js:29
> (Diff revision 1)
> > -var flags = require("devtools/shared/flags");
> > -flags.testing = true;
> >  
> > +Services.prefs.setBoolPref("devtools.testing", true);
> > +SimpleTest.registerCleanupFunction(() => {
> > +  Services.prefs.setBoolPref("devtools.testing", false);
> 
> Could we use `clearUserPref` on the cleanup paths?

Done

> 
> ::: devtools/client/shared/redux/middleware/test/head.js:12
> (Diff revision 1)
> >  "use strict";
> >  
> >  const { require } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});
> > -const flags = require("devtools/shared/flags");
> > +const Services = require("Services");
> >  
> > -flags.testing = true;
> > +Services.prefs.setBoolPref("devtools.testing", true);
> 
> Hmm, can we remove this instance of it?  I would expect this file to just
> have helper functions.

Removed.

> ::: devtools/shared/flags.js:12
> (Diff revision 1)
> >   * We cannot make a normal property writeable on `exports` because
> >   * the module system freezes it.
> >   */
> > -function makeWritableFlag(exports, name) {
> > -  let flag = false;
> > +function makeWritableFlag(exports, name, pref) {
> > +  let flag;
> > +  // We don't have access to pref in worker and enable all logs by default
> 
> Hmm, why do you want to default logs on in workers...?  Logs (just the
> regular `log` one) are still disabled for many test directories because they
> too verbose.  `log.verbose` is even more over the top.
> 
> Doesn't this also mean workers would log all this even in production usage? 
> That's a lot of logging that I don't think we want.
> 
> Hmm, I guess you are copying what was in server/main.js.  But it seems like
> a terrible default for production use!

Exactly, It looks like all logs were enable on workers.
I switched it back to false for workers.
So that you have to manually edit this file to see them.

> ::: devtools/shared/flags.js:17
> (Diff revision 1)
> > +  // We don't have access to pref in worker and enable all logs by default
> > +  if (isWorker) {
> > +    flag = name != "testing";
> > +  } else {
> > +    flag = Services.prefs.getBoolPref(pref, false);
> > +    Services.prefs.addObserver(pref, () => {
> 
> Do we need to remove this observer when the loader shuts down, or does it
> all get GCed correctly anyway?

The state of addon/loader unload is uncertain, but I added to code to clean things up.
I'm not sure it would be GCed correctly, unless we nuke the sandboxes.

> ::: devtools/shared/flags.js:34
> (Diff revision 1)
> > -makeWritableFlag(exports, "wantVerbose");
> > +makeWritableFlag(exports, "wantVerbose", "devtools.debugger.log.verbose");
> >  
> >  // When the testing flag is set, various behaviors may be altered from
> >  // production mode, typically to enable easier testing or enhanced
> >  // debugging.
> > -makeWritableFlag(exports, "testing");
> > +makeWritableFlag(exports, "testing", "devtools.testing");
> 
> Since it's now a pref, should we set the pref in
> https://searchfox.org/mozilla-central/source/testing/profiles/prefs_general.
> js rather than setting it from various head files?

I added the pref to this file and removed all manual set from mochitest's heads,
but kept them for head.js files related to xpcshell test suite (which doesn't use prefs_general.js).


Do not hesitate to re-scan the new patch!

Comment 8

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

Comment on attachment 8967465 [details]
Bug 1453712 - Use a preference to set the testing flag.

https://reviewboard.mozilla.org/r/236122/#review242040

Looks great to me! :)

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8967465 [details]
Bug 1453712 - Use a preference to set the testing flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236122/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b131d7086d3
Use a preference to set the testing flag. r=jryans

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1b131d7086d3