Merge startup debugging support from Positron branch

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools
--
enhancement
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
Firefox 55
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Over in the Positron branch, we added support platform debugging (the equivalent of Browser Toolbox) to pause the platform when --jsdebugger is passed to aid in debugging startup issues.

This should be generally useful to Firefox developers as well.

This bug should include some kind of squashed version of the changes from:

https://github.com/mozilla/positron/pull/41
https://github.com/mozilla/positron/pull/43
https://github.com/mozilla/positron/pull/54
Severity: normal → enhancement
@jryans: This is another one of those Positron DevTools enhancements that it would be helpful to upstream.  Is it something you can tackle, or shall I take it on?
Flags: needinfo?(jryans)
I'll take it on, thanks for the reminder!
Flags: needinfo?(jryans)
Comment hidden (mozreview-request)
Comment on attachment 8843039 [details]
Bug 1275942 - Add option to wait for debugger on startup.

I think this is an accurate rebase of the Positron code, but it doesn't seem as useful in Firefox so far...  I tried to test it out in various startup files, but command line handler runs too late in startup, so the startup modules have already loaded by the time we pause.

Is it useful for you in this state?
Attachment #8843039 - Flags: feedback?(myk)
Comment on attachment 8843039 [details]
Bug 1275942 - Add option to wait for debugger on startup.

I tried it in Firefox, adding a debugger statement to the app-startup/profile-after-change observer in TelemetryStartup.js, and that observer got called before Firefox could pause to await the debugger connection.

I also tried it in the nsDefaultCLH.handle() method in nsDefaultCLH.js, and it broke into the debugger as expected (although the initial browser window also appeared and functioned, so I guess nsICommandLineHandler.handle's caller doesn't wait for it to return).  It was also effective at the top of browser.js, breaking into the debugger and stalling the browser window.

I suppose its utility depends on how early the startup code runs.  For some Firefox use cases, it'll work.  And it also works for my use case.  So I think it's a worthy enhancement.

NB: Chrome appears to have the equivalent, according to http://peter.sh/experiments/chromium-command-line-switches/#wait-for-debugger, except that it only waits for up to 60 seconds.  I don't think a timeout is particularly necessary, although I'm willing to entertain it.
Attachment #8843039 - Flags: feedback?(myk) → feedback+
Okay, cool.  It does work for me as well when I try in browser.js.

I think I'll rename the option to "wait-for-jsdebugger", that seems a bit more explicit about the purpose.  (It seems like Chrome's "wait-for-debugger" is for a native code debugger, like gdb / LLDB.)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2493b3fc1d777ebd750eed1be6dad740f8378fe
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03b951e954ca822e8e3c182822ff087921b8809a

Comment 12

9 months ago
mozreview-review
Comment on attachment 8843039 [details]
Bug 1275942 - Add option to wait for debugger on startup.

https://reviewboard.mozilla.org/r/116794/#review119216

Looks good! I don't really know much about hiddenDOMWindow to comment on this part, but the rest looks good to me.
Just one question: is there any reason to not make this the default behavior when using --jsdebugger? Should it even be an option?

::: devtools/client/devtools-startup.js:226
(Diff revision 4)
>  
>    /* eslint-disable max-len */
>    helpInfo: "  --jsconsole        Open the Browser Console.\n" +
>              "  --jsdebugger       Open the Browser Toolbox.\n" +
> +            "  --wait-for-jsdebugger Spin event loop until JS debugger connects.\n" +
> +            "                     Enables debugging (some) application startup code paths.\n" +

handleDebuggerFlag is only called if the --jsdebugger flag is on. This means that --wait-for-jsdebugger requires --jsdebugger to be on.
Either:
- mention it in the helpInfo for --wait-for-jsdebugger
- call handleDebuggerFlag as well if --wait-for-jsdebugger is on
Attachment #8843039 - Flags: review?(jdescottes) → review+
(Assignee)

Comment 13

9 months ago
mozreview-review-reply
Comment on attachment 8843039 [details]
Bug 1275942 - Add option to wait for debugger on startup.

https://reviewboard.mozilla.org/r/116794/#review119216

> handleDebuggerFlag is only called if the --jsdebugger flag is on. This means that --wait-for-jsdebugger requires --jsdebugger to be on.
> Either:
> - mention it in the helpInfo for --wait-for-jsdebugger
> - call handleDebuggerFlag as well if --wait-for-jsdebugger is on

I was worried that `--wait-for-jsdebugger` might slow down some workflows, but testing again here, I don't really notice a slow down.  So for now, I'll remove the option and have this always enabled with `--jsdebugger`.
(Assignee)

Comment 14

9 months ago
mozreview-review-reply
Comment on attachment 8843039 [details]
Bug 1275942 - Add option to wait for debugger on startup.

https://reviewboard.mozilla.org/r/116794/#review119216

> I was worried that `--wait-for-jsdebugger` might slow down some workflows, but testing again here, I don't really notice a slow down.  So for now, I'll remove the option and have this always enabled with `--jsdebugger`.

Hmm, actually I've changed my mind.  I think it could be confusing for some people if they use `--jsdebugger` to access the inspector because this change will make it default to the hidden window instead of the first browser window.  (You can still switch to the first browser window in frames, but it's probably not obvious.)

So, to avoid disturbing workflows, I'll keep the extra option for this, and note in the help that you need `--jsdebugger` too.
Comment hidden (mozreview-request)

Comment 16

9 months ago
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/783dcc2af468
Add option to wait for debugger on startup. r=jdescottes

Comment 17

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/783dcc2af468
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Adding dev-doc-needed, we should add an entry to https://developer.mozilla.org/en-US/docs/Mozilla/Command_Line_Options describing the new "--wait-for-jsdebugger" command line option.
Keywords: dev-doc-needed
Posted to various mailing lists:

https://groups.google.com/d/topic/mozilla.dev.developer-tools/xIM8TxAcCjk/discussion
Blocks: 814298
You need to log in before you can comment on or make changes to this bug.