Issue decoding after migrating from 4.x to 6.6.3

[johnnyreilly]

protobuf.js version: 6.6.3

Hey it's me again 😉

I've just attempted an upgrade from v4 to v6. With v4 I was generating js files like so:

pbjs ./proto/bcl.proto ./proto/MonitoringClientMessage.proto ./proto/MonitoringServerMessage.proto --target commonjs > ./proto/proto.js

With v6 I'm doing it like so: (I've also attempted with commonjs as well)

pbjs --target static-module --wrap es6 --out ./proto/proto6.js ./proto/bcl.proto ./proto/MonitoringClientMessage.proto ./proto/MonitoringServerMessage.proto
pbts --out ./proto/proto6.d.ts ./proto/proto6.js'

However, decoding always fails for me. Specifically, when I debug Reader.prototype.uint32 always produces 0:

/**
 * Reads a varint as an unsigned 32 bit value.
 * @function
 * @returns {number} Value read
 */
Reader.prototype.uint32 = (function read_uint32_setup() {
    var value = 4294967295; // optimizer type-hint, tends to deopt otherwise (?!)
    return function read_uint32() {
        value = (         this.buf[this.pos] & 127       ) >>> 0; if (this.buf[this.pos++] < 128) return value;
        value = (value | (this.buf[this.pos] & 127) <<  7) >>> 0; if (this.buf[this.pos++] < 128) return value;
        value = (value | (this.buf[this.pos] & 127) << 14) >>> 0; if (this.buf[this.pos++] < 128) return value;
        value = (value | (this.buf[this.pos] & 127) << 21) >>> 0; if (this.buf[this.pos++] < 128) return value;
        value = (value | (this.buf[this.pos] &  15) << 28) >>> 0; if (this.buf[this.pos++] < 128) return value;

        /* istanbul ignore next */
        if ((this.pos += 5) > this.len) {
            this.pos = this.len;
            throw indexOutOfRange(this, 10);
        }
        return value;
    };
})();

It's rather strange and so I wanted to report in case I'm doing anything dozy... If there's any ideas I'd greatly appreciate them!

[dcodeIO]

Do you have an example buffer / proto at hand?

[johnnyreilly]

Yup - I'll share one shortly

…

________________________________ From: Daniel Wirtz <notifications@github.com> Sent: 01 February 2017 16:26:05 To: dcodeIO/protobuf.js Cc: John Reilly; Author Subject: Re: [dcodeIO/protobuf.js] Issue decoding after migrating from 4.x to 6.6.3 (#670) Do you have an example buffer at hand? - You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#670 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AA9rXUHMMJVwxgtdHjxnPJbJOv_HILkjks5rYLIdgaJpZM4L0Cci>.

[johnnyreilly]

Actually there's nothing that I'm allowed to share.... Frustrating. I'll see if I can get a repro with one of the examples and share that.

[dcodeIO]

There is also a little howto on how to reverse engineer a buffer by hand in order to spot the underlying issue.

[johnnyreilly]

Thanks I'll take a look!

[johnnyreilly]

I'm not at a keyboard and so can't test but I just noticed the presence of a --keep-case option. I'm relying on the original (not camel) case for my app and I haven't used it in my new generation. Is that something worth checking out? I can't see it listed in breaking changes since v4 so perhaps not?

[dcodeIO]

ParseOptions, goes there: https://github.com/dcodeIO/protobuf.js/blob/master/src/root.js#L70

[johnnyreilly]

So worth trying? Has the camelCase thing only be around since v6 then?

[dcodeIO]

v5 had a global option. v6 uses camel case by default, which can still be overridden with { keepCase: true }.

[johnnyreilly]

And --keep-case if using pbjs presumably? Great I'll give it a try in the morning. Thanks!

[johnnyreilly]

Nope --keep-case wasn't the issue

[johnnyreilly]

OK - it seems likely that the problem lies with ArrayBuffer. My project is using ArrayBuffer from the babel polyfill - and it seems there might be some issues with that:

babel/babel#5140

Namely I can't actually read from the ArrayBuffer and it doesn't have the API you would expect. (It lacks a length property for instance)

[johnnyreilly]

OK. It's a little strange but I have a workaround. I take the data I receive from the websocket and treat the supplying ArrayBuffer as if it is diseased. I rob of its data and leave it breathing its last on the pavement:

    const clonedData = new Uint8Array(data);

This cloned array will give me what I need. Very strange but I thought it was probably worth sharing...

[dcodeIO]

Yeah, using Uint8Array in the browser / Buffer under node.js is correct. ArrayBuffer alone does not provide ways to manipulate the data (at least not within all environments).

Uint8Array is not cloning, by the way, it is creating a view on the raw binary data.

From MDN:

The ArrayBuffer object is used to represent a generic, fixed-length raw binary data buffer. You cannot directly manipulate the contents of an ArrayBuffer; instead, you create one of the typed array objects or a DataView object which represents the buffer in a specific format, and use that to read and write the contents of the buffer.

[johnnyreilly]

Thanks for the clarification - I'd missed that

[johnnyreilly]

Is it worth mentioning somewhere in the docs that users might need this approach if they are consuming ArrayBuffers?

[dcodeIO]

The API documentation always refers to Uint8Array respectively Buffer. It's not mentioned in the README, though, hmm.

[johnnyreilly]

I was actually more thinking about the decode side of things; a user may be handed an ArrayBuffer via a websocket without realising that they need to Uint8Array-ify things before they can decode. That was my scenario.

[dcodeIO]

Yeah, added that to the first example in README. It's intentional that it doesn't accept an ArrayBuffer for perf reasons (avoids the additional assertion).

[johnnyreilly]

Oh I saw - my point was more that people may not realise that they have an ArrayBuffer on their hands that they need to transform. I didn't and I'd say it's not obvious..

[johnnyreilly]

Actually it's covered here: https://github.com/dcodeIO/protobuf.js/wiki/How-to-read-binary-data-in-the-browser-or-under-node.js%3F

I wonder if this section merits promotion to the readme? Or a link off to it perhaps?

[johnnyreilly]

Closing as all my issues are resolved; thanks for your help @dcodeIO!