Closed Bug 1275942 Opened 4 years ago Closed 3 years ago

Merge startup debugging support from Positron branch

Categories

(DevTools :: General, enhancement)

enhancement
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file)

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 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 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+
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`.
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.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/783dcc2af468
Add option to wait for debugger on startup. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/783dcc2af468
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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
Blocks: 814298
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.