Incorrect double encoding in google-protobuf js module

[srubin]

What version of protobuf and what language are you using?
Version: v3.9.2
Language: Javascript

What operating system (Linux, Windows, ...) and version?
Any (but I am specifically using macOS 10.14.5).

What runtime / compiler are you using (e.g., python version or gcc version)
Node 10.16.3.

What did you do?
I encoded the double 3.9999999999999997 in a protobuf message.

What did you expect to see
I expected the field's value to be encoded as 3.9999999999999997 (i.e., [ 255, 255, 255, 255, 255, 255, 15, 64 ]).

What did you see instead?
The field's value was encoded as 7.999999999999999 (i.e., [ 255, 255, 255, 255, 255, 255, 31, 64 ])

Here's some code that demonstrates the issue:

const googleProtobuf = require("google-protobuf");
const field = 1;

function testEncodeDecode(value) {
    const writer = new googleProtobuf.BinaryWriter();
    writer.writeDouble(field, value);
    const buf = writer.getResultBuffer();
    
    const reader = new googleProtobuf.BinaryReader(buf, 0, buf.length);
    reader.nextField()
    const decoded = reader.readDouble()
    
    const encoded = new Float64Array(buf.slice(1).buffer);
    
    const target = new Float64Array([value]);
    
    console.log(
        'Value', value, 
        'Target encoded', target[0], 
        'Actual encoded', encoded[0], 
        'Equal?', target[0] === encoded[0], value === decoded
    );
}

testEncodeDecode(3.999999999999999);  // succeeds
testEncodeDecode(3.9999999999999999); // succeeds
testEncodeDecode(3.9999999999999997); // fails
testEncodeDecode(7.999999999999999);  // fails
testEncodeDecode(15.999999999999998); // fails

splitFloat64 is the culprit here. Regardless of if the bug is fixed within that function, would it make sense to prefer to use TypedArray (Float64Array) if it's available? I noticed that protobufjs takes this approach.

[srubin]

Oh, interesting - it looks like this bug was actually fixed ~20 days ago: https://github.com/protocolbuffers/protobuf/pull/6634/files#diff-9c7d91628bbeb6b4b24ce8d265aba996 and #6634 but I guess the new code hasn't made its way into a release yet.

Any idea why this doesn't seem to be included in 3.9.2 or 3.10.0-rc.1?

[rafi-kamal]

@srubin the branch cut for 3.10.x was done before the issue was fixed on master. @TeBoring is it something worth cherry-picking to 3.10.x branch?

[elharo]

fixed