1375280 - Allow the browser toolbox by default in local builds (and skip the prompt for remote debugger connections in Nightly)

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

For Nightly only, lets flip the preference that requires a remote connection prompt such that our Firefox developers using Nightly can avoid this dialog.  This dialog is a common annoyance for everyone.

All other channels of Firefox should have the pref turned on such that they continue to receive the prompt until bug 1013379 can be closed with a proper fix.

The preference in question is devtools.debugger.prompt-connection

http://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#1026

Something like this:

#ifdef NIGHTLY_BUILD
pref("devtools.debugger.prompt-connection", false);
#endif

Comment 1

[Brian Grinstead [:bgrins]]

I think what we really want to target here is a local build and not a nightly packaged build that non-devs might install.

Also, what do you think about also setting devtools.chrome.enabled and devtools.debugger.remote-enabled in this case (so that you can immediately open the browser toolbox without changing any prefs in a local build)?

Comment 2

[Brian Grinstead [:bgrins]]

Gregory, we'd like to target local builds to set some different default pref values.  AFAICT checking if MOZ_SOURCE_URL is defined is one way to do that (as in https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/AppConstants.jsm#336-342).  Is there a better way to check this?

Comment 3

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

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Gregory, we'd like to target local builds to set some different default pref
> values.  AFAICT checking if MOZ_SOURCE_URL is defined is one way to do that
> (as in
> https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/AppConstants.
> jsm#336-342).  Is there a better way to check this?

I think you could use !MOZILLA_OFFICIAL as an approximation of "is local build".

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: Bug 1375280 - Allow the Browser Toolbox by default in local builds;

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

Comment 5

[Brian Grinstead [:bgrins]]

Mark, I wanted to run this by you because of your involvement in Bug 1013379.  Would you be OK with enabling the Browser Toolbox by default without a prompt in local builds?  The server itself doesn't start until one of the following things happens:

1) the user requests the browser toolbox opens in the UI
2) The process is started with --jsdebugger so the browser toolbox opens at startups
3) The process is started with --start-debugger-server <PORT>

What this would enable that currently doesn't work is:

./mach clobber && ./mach build && ./mach run --jsdebugger

Comment 6

[Gregory Szorc [:gps]]

It's best to not test MOZ_SOURCE_URL. In general, behavior shouldn't differ based on the source repo URL.

MOZ_OFFICIAL is a rough approximation of official vs local builds.

In general, we want local builds and automation builds to be as similar as possible. If the local and automation builds vary, you can run into problems where tests pass one place and fail in the other. If you want to influence behavior locally, my recommendation is to do it at run-time via defaults in `mach run`.

Comment 7

[Brian Grinstead [:bgrins]]

Summary of a discussion :gps and I had about how to set prefs locally:

* There isn't another place that we set prefs from ./mach run.  To do so we'd either want to add a command line argument to firefox or accept an environment variable that changes (which feels hacky). Making this change is probably more effort than it is worth
* Generally it is more preferable to do this on a channel basis than by local vs automation build basis since divergence between local and automation builds can be dangerous, as they have the potential to significantly impact test results.  But if we must, we could have test runners force reset the defaults such as https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/geckoinstance.py#177 and https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/mochitest_options.py#732

Even though it's better to process by channel, I'm concerned about doing this for Nightly since we are actively encouraging non-devs to use Nightly as a daily driver.  And 'local build' is a better signal that you'll need to use the browser toolbox.

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> I think what we really want to target here is a local build and not a
> nightly packaged build that non-devs might install.

No, I'm asking for Nightly.  I don't know that more people are seeing this in local builds than just in Nightly but I don't think we have that data to confirm either way.

> Also, what do you think about also setting devtools.chrome.enabled and
> devtools.debugger.remote-enabled in this case (so that you can immediately
> open the browser toolbox without changing any prefs in a local build)?

Yes, lets do this too.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Bryan Clark (DevTools PM) [:clarkbw] from comment #8)
> (In reply to Brian Grinstead [:bgrins] from comment #1)
> > I think what we really want to target here is a local build and not a
> > nightly packaged build that non-devs might install.
> 
> No, I'm asking for Nightly.  I don't know that more people are seeing this
> in local builds than just in Nightly but I don't think we have that data to
> confirm either way.
> 
> > Also, what do you think about also setting devtools.chrome.enabled and
> > devtools.debugger.remote-enabled in this case (so that you can immediately
> > open the browser toolbox without changing any prefs in a local build)?
> 
> Yes, lets do this too.

If I understand correctly, what you want is:

Release (same as current defaults):
devtools.debugger.prompt-connection = true
devtools.chrome.enabled = false
devtools.debugger.remote-enabled = false

Nighty (same as current defaults, but without prompt)
devtools.debugger.prompt-connection = false
devtools.chrome.enabled = false
devtools.debugger.remote-enabled = false

Local Build (allow BT by default, without a prompt):
devtools.debugger.prompt-connection = false
devtools.chrome.enabled = true
devtools.debugger.remote-enabled = true

Does that look right?

Comment 10

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> Does that look right?

Yes, that looks right!  Thx

Comment 11

[Mark Goodwin [:mgoodwin]]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Would you be OK with enabling the Browser Toolbox by default
> without a prompt in local builds?

I think that's reasonable.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8884083 [details]
Bug 1375280 - Allow the Browser Toolbox by default in local builds;

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

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> * Generally it is more preferable to do this on a channel basis than by
> local vs automation build basis since divergence between local and
> automation builds can be dangerous, as they have the potential to
> significantly impact test results.  But if we must, we could have test
> runners force reset the defaults such as
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/
> marionette_driver/geckoinstance.py#177 and
> https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/
> mochitest_options.py#732

I ended up not making this change in this version of the patch, since it's nice to have chrome debugging on by default when running tests like mochitests so you can run commands in the Browser Console when debugging a failure. Would be open to make the change (forcing chrome/remote debugging off for the test harness) - I believe the main risk of local/automation divergence in this case are in devtools tests that are covering chrome/remote debugging directly.

Comment 14

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

Comment on attachment 8884083 [details]
Bug 1375280 - Allow the Browser Toolbox by default in local builds;

https://reviewboard.mozilla.org/r/155022/#review162162

Looks good to me.  

This gives Nightly a slighty higher risk profile, since it's now only a one time checkbox away from remote control, as opposed to a one time checkbox and prompt on each use.  (Even then, we don't start a server just from the checkbox, so there's still several steps required to take over the browser.)  It sounds from the discussion that we believe the risk is worth the convenience here.

::: modules/libpref/init/all.js:1030
(Diff revision 2)
>  // Disable debugging chrome
>  pref("devtools.chrome.enabled", false);
> +// Disable remote debugging connections
> +pref("devtools.debugger.remote-enabled", false);
> +#else
> +// In local builds, enable the browser toolbox by default without a prompt

Maybe tweak this comment, since these two are about the connection, not the prompt itself?

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8884083 [details]
Bug 1375280 - Allow the Browser Toolbox by default in local builds;

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/847f584256c8
Allow the Browser Toolbox by default in local builds;r=jryans

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_webconsole_autocomplete_and_selfxss.js with 'Test for usage count getter - Got 5, expected 0' on Linux x64 asan:

https://hg.mozilla.org/integration/autoland/rev/42091be84f6bcd377f99b2cb3ba2711bf34f54e9

First push which ran tests: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=45c8fb17933b6db291beb275d074288c2a0fb66e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=114184418&repo=autoland
[task 2017-07-14T01:40:48.869925Z] 01:40:48     INFO - TEST-START | devtools/client/webconsole/test/browser_webconsole_autocomplete_and_selfxss.js
[task 2017-07-14T01:40:50.346897Z] 01:40:50     INFO - TEST-INFO | started process screentopng
[task 2017-07-14T01:40:50.667107Z] 01:40:50     INFO - TEST-INFO | screentopng: exit 0
[task 2017-07-14T01:40:50.668240Z] 01:40:50     INFO - Buffered messages logged at 01:40:48
[task 2017-07-14T01:40:50.669082Z] 01:40:50     INFO - Entering test bound 
[task 2017-07-14T01:40:50.670229Z] 01:40:50     INFO - Adding a new tab with URL: data:text/html;charset=utf-8,<p>test for bug 642615
[task 2017-07-14T01:40:50.672495Z] 01:40:50     INFO - Buffered messages logged at 01:40:49
[task 2017-07-14T01:40:50.673424Z] 01:40:50     INFO - Tab added and finished loading
[task 2017-07-14T01:40:50.674115Z] 01:40:50     INFO - Console message: [JavaScript Warning: "Unknown property ‘user-select’.  Declaration dropped." {file: "resource://devtools/client/shared/components/reps/reps.css" line: 212 column: 13 source: "  user-select: none;"}]
[task 2017-07-14T01:40:50.674862Z] 01:40:50     INFO - Buffered messages logged at 01:40:50
[task 2017-07-14T01:40:50.675568Z] 01:40:50     INFO - TEST-PASS | devtools/client/webconsole/test/browser_webconsole_autocomplete_and_selfxss.js | no completeNode.value - 
[task 2017-07-14T01:40:50.676276Z] 01:40:50     INFO - wait for completion value after typing 'docu'
[task 2017-07-14T01:40:50.677131Z] 01:40:50     INFO - Console message: [JavaScript Warning: "Key event not available on some keyboard layouts: key=“r†modifiers=“accel,alt†id=“key_toggleReaderModeâ€" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 815}]
[task 2017-07-14T01:40:50.677813Z] 01:40:50     INFO - Console message: [JavaScript Warning: "Key event not available on some keyboard layouts: key=“i†modifiers=“accel,alt,shift†id=“key_browserToolboxâ€" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 815}]
[task 2017-07-14T01:40:50.678179Z] 01:40:50     INFO - TEST-PASS | devtools/client/webconsole/test/browser_webconsole_autocomplete_and_selfxss.js | Clipboard has the given value - 
[task 2017-07-14T01:40:50.678586Z] 01:40:50     INFO - Self-xss paste tests
[task 2017-07-14T01:40:50.679803Z] 01:40:50     INFO - Buffered messages finished
[task 2017-07-14T01:40:50.680762Z] 01:40:50     INFO - TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_autocomplete_and_selfxss.js | Test for usage count getter - Got 5, expected 0
[task 2017-07-14T01:40:50.681072Z] 01:40:50     INFO - Stack trace:
[task 2017-07-14T01:40:50.681467Z] 01:40:50     INFO - chrome://mochikit/content/browser-test.js:test_is:967
[task 2017-07-14T01:40:50.682113Z] 01:40:50     INFO - chrome://mochitests/content/browser/devtools/client/webconsole/test/browser_webconsole_autocomplete_and_selfxss.js:testSelfXss:65
[task 2017-07-14T01:40:50.682391Z] 01:40:50     INFO - chrome://mochitests/content/browser/devtools/client/webconsole/test/browser_webconsole_autocomplete_and_selfxss.js:onClipboardCopy:52
[task 2017-07-14T01:40:50.682971Z] 01:40:50     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:wait:1019
[task 2017-07-14T01:40:50.683798Z] 01:40:50     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForClipboard/<:1033
[task 2017-07-14T01:40:50.684007Z] 01:40:50     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:wait:1019
[task 2017-07-14T01:40:50.684341Z] 01:40:50     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForClipboard:1029
[task 2017-07-14T01:40:50.684955Z] 01:40:50     INFO - chrome://mochikit/content/browser-test.js:test_waitForClipboard:1010
[task 2017-07-14T01:40:50.685175Z] 01:40:50     INFO - chrome://mochitests/content/browser/devtools/client/webconsole/test/browser_webconsole_autocomplete_and_selfxss.js:onCompletionValue:42
[task 2017-07-14T01:40:50.685373Z] 01:40:50     INFO - resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:handler:138
[task 2017-07-14T01:40:50.685494Z] 01:40:50     INFO - resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:emit:194
[task 2017-07-14T01:40:50.685761Z] 01:40:50     INFO - resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/jsterm.js:_receiveAutocompleteProperties:1614
[task 2017-07-14T01:40:50.686010Z] 01:40:50     INFO - resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:emitOnObject:110
[task 2017-07-14T01:40:50.686189Z] 01:40:50     INFO - resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:emit:86
[task 2017-07-14T01:40:50.686381Z] 01:40:50     INFO - resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:emit:1323
[task 2017-07-14T01:40:50.686551Z] 01:40:50     INFO - resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:emitReply:1022
[task 2017-07-14T01:40:50.686744Z] 01:40:50     INFO - Not taking screenshot here: see the one that was previously logged

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: Bug 1375280 - Force remote debugging prefs off if jsdebugger isn't requested for mochitests;

Since the remote debugging pref defaults will be set to true in local builds,
force them off for tests unless --jsdebugger is passed, to limit differences
locally and in automation.

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

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: Bug 1375280 - Force remote debugging prefs off if jsdebugger isn't requested for marionette-test;

Since the remote debugging pref defaults will be set to true in local builds,
force them off for tests unless --jsdebugger is passed, to limit differences
locally and in automation.

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

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8884083 [details]
Bug 1375280 - Allow the Browser Toolbox by default in local builds;

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

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #17)
> Backed out for failing browser_webconsole_autocomplete_and_selfxss.js with
> 'Test for usage count getter - Got 5, expected 0' on Linux x64 asan:

I'll take this as a data point that the convenience having the prefs on by default for tests locally isn't worth the local/automation divergence. Sent up a couple of patches to address this for ./mach mochitest and ./mach marionette-test

Comment 22

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

Comment on attachment 8886595 [details]
Bug 1375280 - Force remote debugging prefs off if jsdebugger isn't requested for mochitests;

https://reviewboard.mozilla.org/r/157412/#review162530

::: testing/mochitest/mochitest_options.py:735
(Diff revision 1)
>              options.extraPrefs += [
>                  "devtools.debugger.remote-enabled=true",
>                  "devtools.chrome.enabled=true",
>                  "devtools.debugger.prompt-connection=false"
>              ]
> +        else:

Can you place these testing defaults in the testing prefs file[1] instead?  If not, I am curious why.  They might cover both test suites that way as well (not sure).

[1]: http://searchfox.org/mozilla-central/source/testing/profiles/prefs_general.js

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #21)
> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on
> intermittent or backout) from comment #17)
> > Backed out for failing browser_webconsole_autocomplete_and_selfxss.js with
> > 'Test for usage count getter - Got 5, expected 0' on Linux x64 asan:
> 
> I'll take this as a data point that the convenience having the prefs on by
> default for tests locally isn't worth the local/automation divergence. Sent
> up a couple of patches to address this for ./mach mochitest and ./mach
> marionette-test

To be clear: it turns out this failure wasn't a case of divergence between local and automation, at least for asan builds, since the test fails locally as well (it's expecting a different default pref value).  It appears asan may have MOZ_OFFICIAL not set, since it was running with the same prefs as locally. The divergence is that the test seems to pass in all other builds.

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #22)
> Can you place these testing defaults in the testing prefs file[1] instead? 
> If not, I am curious why.  They might cover both test suites that way as
> well (not sure).

Good idea, I'll try

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8884083 [details]
Bug 1375280 - Allow the Browser Toolbox by default in local builds;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155022/diff/4-5/

Comment 26

[Brian Grinstead [:bgrins]]

Clearing the review for the change to the test harnesses, since testing/profiles/prefs_general.js sets the value correctly (at least for mochitests)

Comment 27

[Brian Grinstead [:bgrins]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d3f2323a1e1ab5bfa7db5af1ff25e5a99ccb079 - the dt test is fixed on asan. There's a lot of orange there but I've seen this on other try pushes as well.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4feaeb32cfd
Allow the Browser Toolbox by default in local builds;r=jryans

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f4feaeb32cfd