Bug 1464461 highlighted that exceptions happening in the console could lead to broken devtools without any exception/error logged anywhere! This is related to such code pattern: https://searchfox.org/mozilla-central/source/devtools/shared/client/debugger-client.js#435-448 return this.request(packet).then(response => { let consoleClient; if (!response.error) { if (this._clients.has(consoleActor)) { consoleClient = this._clients.get(consoleActor); } else { consoleClient = new WebConsoleClient(this, response); this.registerClient(consoleClient); } } onResponse(response, consoleClient); return [response, consoleClient]; }); The issue here is that request(packet) is a promise that is *rejected* when an error happens on the actor side (i.e. response.error is defined) https://searchfox.org/mozilla-central/source/devtools/shared/client/debugger-client.js#631-635 if (resp.error) { deferred.reject(safeOnResponse(resp)); } else { deferred.resolve(safeOnResponse(resp)); } And so, this attachConsole method never call onResponse callback when this happens. Leading to silent errors. This pattern is present in a couple of methods in this file.

Comment on attachment 8983228 [details] Bug 1466691 - Replace callback style in favor of promise for SourceClient methods. New and different patches are coming. Instead of fixing the callbacks, I'll try to switch to promises in callsites.

Comment on attachment 8983228 [details] Bug 1466691 - Replace callback style in favor of promise for SourceClient methods. https://reviewboard.mozilla.org/r/249094/#review256398 Thanks for working on this! :) I think I spotted a few more cleanups in my comments. ::: devtools/server/tests/unit/test_breakpoint-02.js:50 (Diff revision 4) > Assert.equal(packet.type, "paused"); > Assert.equal(packet.why.type, "interrupted"); > }); > > const source = gThreadClient.source(packet.frame.where.source); > - source.setBreakpoint(location, function(response) { > + source.setBreakpoint(location).then(function() { Not sure I follow what the expected behavior is here... is it expecting an error...? ::: devtools/shared/client/source-client.js:55 (Diff revision 4) > */ > blackBox: DebuggerClient.requester({ > type: "blackbox" > }, { > after: function(response) { > if (!response.error) { Should we update all of these `after` blocks as well? It looks like they now only run on success. ::: devtools/shared/client/source-client.js:152 (Diff revision 4) > > const { contentType, source } = response; > if (source.type === "arrayBuffer") { > const arrayBuffer = this._activeThread.threadArrayBuffer(source); > return arrayBuffer.slice(0, arrayBuffer.length).then(function(resp) { > if (resp.error) { This uses `requester`, so this error path will never be hit. ::: devtools/shared/client/source-client.js:171 (Diff revision 4) > }); > } > > const longString = this._activeThread.threadLongString(source); > return longString.substring(0, longString.length).then(function(resp) { > if (resp.error) { This uses `requester`, so this error path will never be hit. ::: devtools/shared/client/source-client.js:240 (Diff revision 4) > if (this._activeThread.paused) { > return doSetBreakpoint(); > } > // Otherwise, force a pause in order to set the breakpoint. > return this._activeThread.interrupt().then(response => { > if (response.error) { This uses `requester`, so this error path will never be hit.

Comment on attachment 8984206 [details] Bug 1466691 - Remove useless attachTrace method. https://reviewboard.mozilla.org/r/250008/#review256402 Hooray!

Comment on attachment 8984207 [details] Bug 1466691 - Replace callback style in favor of promise for TabClient methods. https://reviewboard.mozilla.org/r/250010/#review256392 Thanks, looks good overall! :) ::: devtools/client/framework/target.js:440 (Diff revision 2) > } > > this._setupRemoteListeners(); > > - const attachTab = () => { > - this._client.attachTab(this._form.actor, (response, tabClient) => { > + const attachTab = async () => { > + const [ response, tabClient ] = await this._client.attachTab(this._form.actor); If there's an error in `attachTab` (which rejects the promise), then `await` will throw, so `!tabClient` is now unreachable. Maybe use try / catch or then with error handling? ::: devtools/client/framework/target.js:452 (Diff revision 2) > - attachConsole(); > + attachConsole(); > - }); > }; > > - const onConsoleAttached = (response, consoleClient) => { > + const onConsoleAttached = ([response, consoleClient]) => { > if (!consoleClient) { This case is now unreachable; it's handled by the rejection path. ::: devtools/shared/client/debugger-client.js:400 (Diff revision 2) > - * @param function onResponse > - * Called with the response packet and a WebConsoleClient > - * instance (which will be undefined on error). > */ > attachConsole: > - function(consoleActor, listeners, onResponse = noop) { > + function(consoleActor, listeners) { Nit: maybe this can move up a line now?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10) > Comment on attachment 8983228 [details] > Bug 1466691 - Replace callback style in favor of promise for SourceClient > methods. > > https://reviewboard.mozilla.org/r/249094/#review256398 > > Thanks for working on this! :) I think I spotted a few more cleanups in my > comments. > > ::: devtools/server/tests/unit/test_breakpoint-02.js:50 > (Diff revision 4) > > Assert.equal(packet.type, "paused"); > > Assert.equal(packet.why.type, "interrupted"); > > }); > > > > const source = gThreadClient.source(packet.frame.where.source); > > - source.setBreakpoint(location, function(response) { > > + source.setBreakpoint(location).then(function() { > > Not sure I follow what the expected behavior is here... is it expecting an > error...? This is really unclear. The usage of Assert.notEqual is *really* confusing. It comes from bug 820012 without clear reason... Note that the new code may make this even more unclear, but it shouldn't change from what we currently do, where the callback was called in both success/failure. > > ::: devtools/shared/client/source-client.js:55 > (Diff revision 4) > > */ > > blackBox: DebuggerClient.requester({ > > type: "blackbox" > > }, { > > after: function(response) { > > if (!response.error) { > > Should we update all of these `after` blocks as well? It looks like they now > only run on success. I don't think that's called only on success. Look closely at requester implementation: https://searchfox.org/mozilla-central/source/devtools/shared/client/debugger-client.js#114-115 return this.request(outgoingPacket, DevToolsUtils.makeInfallible((response) => { if (after) { const { from } = response; response = after.call(this, response); It uses DebuggerClient.request's `onResponse` callback and not the returned promise. The callback is called on both error/success. Please tell me if I'm getting confused... That being said, I'm wondering if we can refactor `requester` usages to help the future refactoring to protocol.js Fronts? Or is it better to just wait for the full refactoring?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12) > ::: devtools/client/framework/target.js:440 > (Diff revision 2) > > } > > > > this._setupRemoteListeners(); > > > > - const attachTab = () => { > > - this._client.attachTab(this._form.actor, (response, tabClient) => { > > + const attachTab = async () => { > > + const [ response, tabClient ] = await this._client.attachTab(this._form.actor); > > If there's an error in `attachTab` (which rejects the promise), then `await` > will throw, so `!tabClient` is now unreachable. Maybe use try / catch or > then with error handling? Good catch! Used a try/catch. > ::: devtools/client/framework/target.js:452 > (Diff revision 2) > > - attachConsole(); > > + attachConsole(); > > - }); > > }; > > > > - const onConsoleAttached = (response, consoleClient) => { > > + const onConsoleAttached = ([response, consoleClient]) => { > > if (!consoleClient) { > > This case is now unreachable; it's handled by the rejection path. Removed. > ::: devtools/shared/client/debugger-client.js:400 > (Diff revision 2) > > - * @param function onResponse > > - * Called with the response packet and a WebConsoleClient > > - * instance (which will be undefined on error). > > */ > > attachConsole: > > - function(consoleActor, listeners, onResponse = noop) { > > + function(consoleActor, listeners) { > > Nit: maybe this can move up a line now? Done.

(In reply to Alexandre Poirot [:ochameau] from comment #13) > > ::: devtools/shared/client/source-client.js:55 > > (Diff revision 4) > > > */ > > > blackBox: DebuggerClient.requester({ > > > type: "blackbox" > > > }, { > > > after: function(response) { > > > if (!response.error) { > > > > Should we update all of these `after` blocks as well? It looks like they now > > only run on success. > > I don't think that's called only on success. > Look closely at requester implementation: > https://searchfox.org/mozilla-central/source/devtools/shared/client/debugger- > client.js#114-115 > return this.request(outgoingPacket, > DevToolsUtils.makeInfallible((response) => { > if (after) { > const { from } = response; > response = after.call(this, response); > > It uses DebuggerClient.request's `onResponse` callback and not the returned > promise. > The callback is called on both error/success. Ah ah, okay, you are right. > That being said, I'm wondering if we can refactor `requester` usages to help > the future refactoring to protocol.js Fronts? > Or is it better to just wait for the full refactoring? If you have specific refactorings in mind, let's discuss them!

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/973e4e602458 Replace callback style in favor of promise for SourceClient methods. r=jryans https://hg.mozilla.org/integration/autoland/rev/8543c314a9ac Remove useless attachTrace method. r=jryans https://hg.mozilla.org/integration/autoland/rev/d7ee621c8778 Replace callback style in favor of promise for TabClient methods. r=jryans

https://hg.mozilla.org/mozilla-central/rev/973e4e602458 https://hg.mozilla.org/mozilla-central/rev/8543c314a9ac https://hg.mozilla.org/mozilla-central/rev/d7ee621c8778