debugger: fix debugger failure when passing commands with non-latin symbols

[Let0s]

Debug agent incorrectly used Content-Length and cut Content-Length symbols instead of Content-Length bytes from debugger protocol messages. This causes incorrect command body extraction and parsing that leads to debugger disconnection.

I use to fix the issue code similar to https://github.com/nodejs/node/blob/master/lib/_debugger.js#L136
This fixes debugger problem mistakenly reported here microsoft/vscode#17389

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

debugger

[bnoordhuis]

I don't think this PR by itself is sufficient. I had a look at Client.prototype._transform and it does this.buffer += data but data is a Buffer object as far as I can tell.

That's going to mangle UTF-8 character sequences when the sequence is split across buffer boundaries. I didn't test it but I suspect the Client constructor should have a Transform.call({ decodeStrings: true }) in it.

[quanterion]

In debugger typeof this.data is string. Even more in Client.prototype._transform there are this.buffer.match() this.buffer.startsWith() function calls that suggests that this.buffer should be a string.

Even more, concatenation of Buffer objects like this this.buffer += data is not possible (it will result in string)

[bnoordhuis]

@quanterion data is coerced to a string by this.buffer += data but that does the wrong thing when data ends in a partial UTF-8 character sequence. Here's an example:

> var x = Buffer.from('λ')
undefined
> x
<Buffer ce bb>
> '' + x
'λ'
> x.slice(0,1) + x.slice(1)
'��'

[Let0s]

@bnoordhuis does new commit resolve all possible cases with incorrect data transformation?

[bnoordhuis]

Mostly LGTM. Would it be possible to add a regression test?

The tests in test/debugger aren't well-maintained but we have tests in test/sequential and test/parallel that you can use as examples (e.g., test/fixtures/debugger-repeat-last.js.)

[bnoordhuis]

Long line. Can you run make test, it also lints source files.

[bnoordhuis]

Can you use Buffer.alloc(0) here?

[Let0s]

@bnoordhuis where should test file be located? In debugger folder or another?

[bnoordhuis]

@Let0s test/parallel, unless it cannot run in parallel with other tests. In that case it should go into test/sequential.

[jasnell]

There will likely be a bit of a performance hit on this given the multiple utf8 conversions involved on chunks of the buffer (the toString above, the byteLength call here). We should benchmark this change to see how much of a hit this causes.

[bnoordhuis]

this.buffer could be turned into a string_decoder.StringDecoder. It's more structured than manual buffer->string manipulation and should be more efficient too.

[bnoordhuis]

@Let0s The old debugger has been removed in the master branch. If you still want to pursue this, can you file a pull request against the v6.x-staging branch? We can discuss back-porting to v4.x once it lands in v6.x.

I'll take the liberty of closing this one.

[Let0s]

@bnoordhuis I don't think I can make tests for this PR (I don't have so experience in node and in "writing pure code"), so I'll leave it and I will hope that someone will continue it.

[refack]

@Let0s is it still a real problem for you?
If solving this helps you, I can pick it up from here.

[Let0s]

@refack No, it's not a problem for me. I encountered with this in forked project, and I decided to inform base repo about this. Maybe it don't need to be solved if noone else met this problem.

[refack]

@refack No, it's not a problem for me. I encountered with this in forked project, and I decided to inform base repo about this. Maybe it don't need to be solved if noone else met this problem.

@Let0s thank you for the answer. I've added this to my TODO list, but at a low priority...