Closed Bug 1466691 Opened 6 years ago Closed 6 years ago

Various attach methods from TabClient do not handle actors error correctly

Categories

(DevTools :: Framework, defect)

Product:
DevTools
Component:
Framework
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1466691 - Replace callback style in favor of promise for SourceClient methods.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1466691 - Remove useless attachTrace method.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1466691 - Replace callback style in favor of promise for TabClient methods.
59 bytes, text/x-review-board-request
jryans
: review+
Details 
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: nobody → poirot.alex
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 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 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 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+
(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
Product: Firefox → DevTools
Blocks: dt-polish-debt
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: