Various attach methods from TabClient do not handle actors error correctly

RESOLVED FIXED in Firefox 62

Status

defect
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Blocks 1 bug)

unspecified
Firefox 62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

11 months ago
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.
(Assignee)

Updated

11 months ago
Assignee: nobody → poirot.alex
Comment hidden (mozreview-request)
(Assignee)

Comment 3

11 months ago
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.
Attachment #8983228 - Flags: review?(jryans)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

11 months ago
mozreview-review
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.
Attachment #8983228 - Flags: review?(jryans) → review+

Comment 11

11 months ago
mozreview-review
Comment on attachment 8984206 [details]
Bug 1466691 - Remove useless attachTrace method.

https://reviewboard.mozilla.org/r/250008/#review256402

Hooray!
Attachment #8984206 - Flags: review?(jryans) → review+

Comment 12

11 months ago
mozreview-review
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?
Attachment #8984207 - Flags: review?(jryans) → review+
(Assignee)

Comment 13

11 months ago
(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?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

11 months ago
(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!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

10 months ago
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

Updated

10 months ago
Product: Firefox → DevTools

Comment 23

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/973e4e602458
https://hg.mozilla.org/mozilla-central/rev/8543c314a9ac
https://hg.mozilla.org/mozilla-central/rev/d7ee621c8778
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
(Assignee)

Updated

10 months ago
You need to log in before you can comment on or make changes to this bug.