1494249 - Log the client-side stack in _queueResponse when devtools.testing is true

Status: NEW

(DevTools :: Framework, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

While investigating the intermittent on Bug 1493968 the only information I had about the failure was:

TEST-UNEXPECTED-FAIL | devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_navigate.js | A promise chain failed to handle a rejection: Connection closed, pending error from root, type listTabs failed - stack: _queueResponse/responsePromise<@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1590:15

followed by a stack trace of the server-side. In this kind of scenarios, the server side part is usually uninteresting because what happens is that something started a request on the client, but we didn't wait for it to complete before stopping the test. 

It would be much more useful to get the stack information from the client side here. I propose to add this information when the devtools.testing flag is set to true.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1494249 - Add sendClientStack flag to print client stacktrace on transport errors;r=ochameau

Uploading for feedback. Not sure if this behavior should be in the codebase, or maybe
should be behind the wantLogging/wantVerbose flags.

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 9012127 [details]
Bug 1494249 - Add sendClientStack flag to print client stacktrace on transport errors;r=ochameau

Alex, what do you think about such code:
- always do this when running tests
- do this only on demand (wantLogging, wantVerbose flags)
- keep as a gist/snippet out of the codebase, and simply advertise it

I find the information useful so in theory it would be great to directly be able to have it anytime we have a connection closed failure. But since this is not something we can do by default, I am not sure if we should enable it by default on tests (test-only behavior is never great). Having it on demand only could make sense, but has a good chance of being forgotten about and just pollute the codebase. Maybe a compromise could be "on demand" but mention it in the default error message eg "Connection closed, pending [...] failed. You can get the original client side stacktrace by setting devtools.somepref.something to true" or something similar.

Comment 3

[Julian Descottes [:jdescottes]]

I am not 100% sure where the best place is to capture the stack. My initial patches were doing this in debugger-client request() method. The transport class felt like a more centralized spot to do this, however the stack we are getting is harder to analyze because of the various queueing systems we have in devtools. In the test I am investigating we basically fire 3 requests in a row A, B, C. The one that fails on the server is B, but because of queueing, the client side stack is the one for A. In my case the code for the requests is next to each other, so it's not hard to make the connection, but in other cases it might be harder.

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 9012127 [details]
Bug 1494249 - Add sendClientStack flag to print client stacktrace on transport errors;r=ochameau

(In reply to Julian Descottes [:jdescottes][:julian] from comment #2)
> Comment on attachment 9012127 [details]
> Bug 1494249 - Log client stack trace when transport was destroyed in test
> env;r=ochameau
> 
> Alex, what do you think about such code:
> - always do this when running tests
> - do this only on demand (wantLogging, wantVerbose flags)
> - keep as a gist/snippet out of the codebase, and simply advertise it
> 
> I find the information useful so in theory it would be great to directly be
> able to have it anytime we have a connection closed failure.

This information looks useful anytime you care about figuring out these kind of errors.

Using `flags.testing` looks like the good choice for this.
It aligns pretty well with the test-only error being thrown from here:
  https://searchfox.org/mozilla-central/source/devtools/shared/transport/child-transport.js#98
Without flags.testing you only get a useless platform error, missing any actionable context.

I think it is fine having a *test only* code, especially when it is out of typical production code and mostly triggers when an exception/error occurs.

Now, both your patch and the code I linked actually enabled code for every packet.
If I'm following the patch correctly, it will pipe full stacks for every packet, right?
I'm slightly concerned about the overhead it may add to every single packet.
It may play with races...
I'm wondering if you pushed that to try to get a first sense?

Also, it may disallow using wantsLoggin/wantsVerbose as the packet would suddently be super huge.

I think your goal is worth pursuing. Do not keep that in a gist!
But it may require a dedicated flag.

May be we should make something around the pref flags:
  https://searchfox.org/mozilla-central/source/devtools/shared/flags.js#47-65
Random actions:
* Make them easier to use?
* Document them more?
* Mentioning them in exception may make a big difference?
* Wouldn't there be a recent improvement in ./mach environment that would make them easier to use? (Like have this pref turned on on development profiles, like dump, chrome and other devtools prefs)
* ... ?