Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

msgpack.decode() not working for chrome >=60 #57

Open
peeyushsrj opened this issue Aug 21, 2017 · 21 comments
Open

msgpack.decode() not working for chrome >=60 #57

peeyushsrj opened this issue Aug 21, 2017 · 21 comments

Comments

@peeyushsrj
Copy link

screenshot from 2017-08-14 12-06-56

It's working fine for chrome versions, before 60. Also not working on latest firefox! Please do review decode() function.

@mcollina
Copy link
Owner

Can you please provide some cases to reproduce? Thanks!

@peeyushsrj
Copy link
Author

peeyushsrj commented Aug 21, 2017

You can check over here https://backbench.github.io/repl/. This works fine in chrome <= 59! This is repl for https://bblang.org

@mcollina
Copy link
Owner

I can see that. However all the unit test suite is passing locally on Chrome 60 with msgpack v3.5.0.

@peeyushsrj
Copy link
Author

@mcollina I did changed to version 3.5.0 in console, still the same error. You can check yourself over https://backbench.github.io/repl/

@mcollina
Copy link
Owner

Can you reproduce without all of that?

@peeyushsrj
Copy link
Author

I will try to.

@antlafarge
Copy link

antlafarge commented Aug 28, 2017

Same bug, reproduced here in Chrome 60, Firefox 55 and Edge 40 : https://jsfiddle.net/antlafarge/wrjzt1zy/

@antlafarge
Copy link

antlafarge commented Aug 28, 2017

The method BufferList.readUInt8(offset) returns always the first value of the internal buffer.

var first = buf.readUInt8(offset)

@mcollina
Copy link
Owner

So, is this a bug on bl?

I'm a bit lost, as the browser tests are passing.

@antlafarge
Copy link

Maybe, I'm not sure.
But I'm sure the lib is actually broken on most of the recent browsers.

@mcollina
Copy link
Owner

@antlafarge how are you including the library?

@antlafarge
Copy link

antlafarge commented Aug 29, 2017

In my projects with <script src="./libs/msgpack5.js"></script>
In jsfiddle I did an "ugly" copy paste to avoid file hosting ^^

@mcollina
Copy link
Owner

Have you built that bundle yourself, or is it the one from this repo?

@antlafarge
Copy link

From this repo :)

@mcollina
Copy link
Owner

mcollina commented Aug 29, 2017 via email

@antlafarge
Copy link

msgpack5.js and msgpack5.min.js in the dist directory are unchanged. And I have the same problem.

@mcollina
Copy link
Owner

it's now pushed, please check.

@peeyushsrj
Copy link
Author

@mcollina please do a release for current updates in msgpack5. As I am using cdn in production.

@mcollina
Copy link
Owner

v3.5.1 is published. Let me know how it goes.

@antlafarge
Copy link

antlafarge commented Aug 30, 2017

Okay we get further. But there is a problem by decoding with a js array as parameter.
msgpack5().decode([146, 13, 37]); should result to [13, 37], but results to 49.

Here is the call stack :

> BufferList.prototype._appendBuffer( Uint8Array(3) [ 49, 52, 54 ] )
  BufferList.prototype.append( 146 )
  BufferList.prototype.append( [146, 13, 37] )
  msgpack5.decode( [146, 13, 37] )

Here the strange Buffer I got :
image

So my question : Is the js array as input always supported?

Edit : By using an array as input, I expect the BufferList uses the function fromArrayLike internally.

@antlafarge
Copy link

antlafarge commented Aug 30, 2017

By using an Uint8Array as input, the IncompleteBufferError is gone and the result is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants