pause until debugger thread resumed so we can debug startup

[mykmelez]

@jryans This works as expected when applied on top of #40. Is it something that DevTools might be willing take upstream? Perhaps in coordination with another flag like --pause-jsdebugger-until-server-thread-resumes-even-if-that-means-hanging-the-app?

Note: I do see two instances of this error after the server connects:

Handler function threw an exception: TypeError: this._progressListener is undefined Stack: _onDocShellCreated/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webbrowser.js:1319:9 exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 Line: 1319, column: 9

[mykmelez]

Upon further reflection, I might be able to simply delay parsing the appPath argument, and starting the app, until devtools-thread-resumed. Then I wouldn't need to spin the event loop.

[mykmelez]

Upon further reflection, I might be able to simply delay parsing the appPath argument, and starting the app, until devtools-thread-resumed. Then I wouldn't need to spin the event loop.

I tried this over in #40, and it works, but it has its own drawbacks. I'm unsure which implementation I prefer.

[jryans]

This approach to startup debugging seems better to me, since it would be available to any application. I applied this to a Firefox build, and it enabled startup debugging over there as well, which is great!

I think it should be fine to upstream this mostly as-is, but @ochameau and I would need to work out how to handle the _progressListener error. This is happening because we currently expect a window to exist at startup if there are ever going to be windows opening at all. @ochameau and I can work that out in a bug to merge this upstream.

The _progressListener is there to notice if windows change location, etc. which is not too important for Positron's platform debugging. So for Positron at least, you could silence the error if you like by changing webbrowser.js to test for this._progressListener first where the error happens. That should be enough to merge this approach now for Positron without waiting on upstream.

[mykmelez]

The _progressListener is there to notice if windows change location, etc. which is not too important for Positron's platform debugging. So for Positron at least, you could silence the error if you like by changing webbrowser.js to test for this._progressListener first where the error happens. That should be enough to merge this approach now for Positron without waiting on upstream.

Ok, sounds good, I'll make that change!

[mykmelez]

Ok, sounds good, I'll make that change!

I made the change in 4735aeb.

[jryans]

Overall this looks good to me! You may want to add the extra checks I just commented for safety, but I wasn't sure whether they'd be hit in Positron or not.

[mykmelez]

Overall this looks good to me! You may want to add the extra checks I just commented for safety, but I wasn't sure whether they'd be hit in Positron or not.

I hit one of them earlier this morning!

JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webbrowser.js, line 1402: TypeError: this._progressListener is undefined

(Line number slightly different, presumably because of the changes I've already made to this file, but this is the same as the first additional case you noted.)

Additional cases fixed in d6e3096.

[jryans]

Great, this looks ready to go!

[jryans]

I'll file a bug to upstream in a bit.

[mykmelez]

I'll file a bug to upstream in a bit.

Thanks, @jryans! Please cc: me on it!

[jryans]

This will be upstreamed as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1275942.