Closed Bug 1375280 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Debugger, enhancement)

Product:
DevTools
Component:
Debugger
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 56

People

(Reporter: clarkbw, Assigned: bgrins)

References

Details

(Whiteboard: [browser-debugging-issues])

Attachments

(1 file, 2 obsolete files)

Bug 1375280 - Allow the Browser Toolbox by default in local builds;
59 bytes, text/x-review-board-request
jryans
: review+
Details 
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
See Also: → 1013379
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)?
Flags: needinfo?(clarkbw)
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?
Flags: needinfo?(gps)
(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".
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
Flags: needinfo?(mgoodwin)
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`.
Flags: needinfo?(gps)
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.
(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.
Flags: needinfo?(clarkbw)
(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?
Flags: needinfo?(clarkbw)
(In reply to Brian Grinstead [:bgrins] from comment #9)
> Does that look right?

Yes, that looks right!  Thx
Flags: needinfo?(clarkbw)
(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.
Flags: needinfo?(mgoodwin)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
(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.
Summary: Don't prompt for remote debugger connections to the Browser Toolbox in Nightly only → Allow the browser toolbox by default in local builds (and skip the prompt for remote debugger connections in Nightly)
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?
Attachment #8884083 - Flags: review?(jryans) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/847f584256c8
Allow the Browser Toolbox by default in local builds;r=jryans
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
Flags: needinfo?(bgrinstead)
(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
Flags: needinfo?(bgrinstead)
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
Attachment #8886595 - Flags: review?(jryans) → review-
(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.
(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
Attachment #8886595 - Attachment is obsolete: true
Attachment #8886596 - Attachment is obsolete: true
Attachment #8886596 - Flags: review?(cmanchester)
Clearing the review for the change to the test harnesses, since testing/profiles/prefs_general.js sets the value correctly (at least for mochitests)
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.
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f4feaeb32cfd
Allow the Browser Toolbox by default in local builds;r=jryans
https://hg.mozilla.org/mozilla-central/rev/f4feaeb32cfd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1383935
Whiteboard: [browser-debugging-issues]
See Also: → 1388471
See Also: → 1395711
See Also: → 1417137
Product: Firefox → DevTools
status-firefox56: fixed → ---
See Also: → 1431060
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: