Merge startup debugging support from Positron branch

RESOLVED FIXED in Firefox 55

Status

--
enhancement
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

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

unspecified
Firefox 55
dev-doc-needed

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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
(Assignee)

Updated

3 years ago
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)
(Assignee)

Comment 2

2 years ago
I'll take it on, thanks for the reminder!
Flags: needinfo?(jryans)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
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+
(Assignee)

Comment 6

2 years ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/783dcc2af468
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years 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
(Assignee)

Updated

2 years ago
Blocks: 814298

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.