mozilla / positron

a experimental, Electron-compatible runtime on top of Gecko
Other
564 stars 64 forks source link

enable remote debugging of Positron itself #40

Closed mykmelez closed 8 years ago

mykmelez commented 8 years ago

@jryans With this branch, I can successfully open a devtools window in a separate process by passing --jsdebugger on the command line and then debug Positron chrome by setting breakpoints and stepping through the code.

The hack in jsconsole-clhandler.manifest is pretty awful, though, so I'd love your thoughts on a real fix that we can upstream!

mykmelez commented 8 years ago

Incidentally, this branch outputs an error and a warning to the terminal:

JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 191: TypeError: can't redefine non-configurable property "Services" JavaScript warning: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/builtin-modules.js, line 153: DebuggeeWouldRun: debuggee `resource://gre/modules/commonjs/toolkit/loader.js:598' would run

(The warning also appears in both devtools consoles, the one opened by the "hello world" app and the one in the separate process opened by --jsdebugger.)

Neither appears to prevent the app from running, nor the devtools window from working, for that matter.

mykmelez commented 8 years ago

The hack in jsconsole-clhandler.manifest is pretty awful, though, so I'd love your thoughts on a real fix that we can upstream!

One possibility is to register devtools-startup with a unique contract ID and register it in the command-line-handler category with an "entry" that ensures it'll handle the command line before jsconsole-clhandler gets the opportunity, per:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/commandlines/nsICommandLineHandler.idl#17-24

mykmelez commented 8 years ago

@jryans: df6553e makes the Positron command-line handler wait for the devtools process to connect before starting the app, accomplishing the same goal as #41, but without modifying devtools-startup. This implementation also doesn't spin the event loop. But it does require the Positron CLH to load before devtools-startup, so it can detect the --jsdebugger flag before devtools-startup removes it from the list.

And since devtools-startup overrides @mozilla.org/toolkit/console-clh;1, whose category entry is b-jsconsole, I had to change Positron's entry from x-positron to a-positron, which means it runs before most or all other command-line handlers. I'm unsure if there are side-effects to that change.

I'm also unsure why implementation I like better. This one doesn't spin the event loop, and it doesn't require devtools changes. But it does rely on the brittle ordering of command-line handlers. And on the details of devtools-startup. Whereas #41 spins the event loop but also encapsulates the code to handle --jsdebugger in devtools-startup, where it's easier to maintain across changes to that code.

jryans commented 8 years ago

Incidentally, this branch outputs an error and a warning to the terminal:

JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 191: TypeError: can't redefine non-configurable property "Services"
JavaScript warning: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/builtin-modules.js, line 153: DebuggeeWouldRun: debuggee `resource://gre/modules/commonjs/toolkit/loader.js:598' would run

(The warning also appears in both devtools consoles, the one opened by the "hello world" app and the one in the separate process opened by --jsdebugger.)

Neither appears to prevent the app from running, nor the devtools window from working, for that matter.

These warnings appear in Firefox's browser toolbox as well, so should be safe to ignore.

jryans commented 8 years ago

Overall, I think this looks good, but I prefer the startup debugging approach from #41 since it seems to work for Firefox as well, so let's use that route for the startup portion.

mykmelez commented 8 years ago

Overall, I think this looks good, but I prefer the startup debugging approach from #41 since it seems to work for Firefox as well, so let's use that route for the startup portion.

Ok, sounds good, I've reverted the code on this branch to wait for the jsdebugger in 13e057e.

jryans commented 8 years ago

Great, this looks ready to merge!