mcollina / msgpack5

A msgpack v5 implementation for node.js, with extension points / msgpack.org[Node]
MIT License
493 stars 76 forks source link

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

Open peeyushsrj opened 7 years ago

peeyushsrj commented 7 years ago

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 commented 7 years ago

Can you please provide some cases to reproduce? Thanks!

peeyushsrj commented 7 years ago

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

mcollina commented 7 years ago

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

peeyushsrj commented 7 years ago

@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 commented 7 years ago

Can you reproduce without all of that?

peeyushsrj commented 7 years ago

I will try to.

antlafarge commented 7 years ago

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

antlafarge commented 7 years ago

The method BufferList.readUInt8(offset) returns always the first value of the internal buffer. https://github.com/mcollina/msgpack5/blob/5fd4ef72603d99ab139fd570e841dc72c8b274a2/lib/decoder.js#L119

mcollina commented 7 years ago

So, is this a bug on bl?

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

antlafarge commented 7 years ago

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

mcollina commented 7 years ago

@antlafarge how are you including the library?

antlafarge commented 7 years ago

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

mcollina commented 7 years ago

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

antlafarge commented 7 years ago

From this repo :)

mcollina commented 7 years ago

I just pushed an updated version of that bundle. Please check. Il giorno mar 29 ago 2017 alle 19:32 antlafarge notifications@github.com ha scritto:

From this repo :)

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mcollina/msgpack5/issues/57#issuecomment-325649310, or mute the thread https://github.com/notifications/unsubscribe-auth/AADL4xhFA1iNpzuDVN_wpTh3ZmGHCCvJks5sdATygaJpZM4O8-T9 .

antlafarge commented 7 years ago

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

mcollina commented 7 years ago

it's now pushed, please check.

peeyushsrj commented 7 years ago

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

mcollina commented 7 years ago

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

antlafarge commented 7 years ago

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 commented 7 years ago

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