----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30484/#review70531 -----------------------------------------------------------
Ship it! Hey Rafi, Nice one! I do believe that you are right, how on earth did you spot it? I think I was very much in "couldn't see the wood from the trees" mode. Looking at the emscripten documentation here: http://kripken.github.io/emscripten-site/docs/porting/connecting_cpp_and_javascript/Interacting-with-code.html#access-memory-from-javascript The relevant bit is You can access memory using getValue(ptr, type) and setValue(ptr, value, type). The first argument is a pointer (a number representing a memory address). type must be an LLVM IR type, one of i8, i16, i32, i64, float, double or a pointer type like i8* (or just *). So you are absolutely correct, the first argument *should* be a pointer so, as you observe passing in 1024 as an initial value is using 1024 as the *address* and is rather playing Russian Roulette with whatever may or may not already be there. That'll account for why you and I were seeing different things then :-) I just did a grep for setValue and there is a similar bug in message.js 'encode' if you fancy fixing that too ;-> The setValue in data-binary.js *is* using a pointer, so at least I've only got a 66.6% #epicfail rate :-( Looks like my use of getValue is OK too except in data.js 'format' and message.js 'encode' too. Nice work! - Fraser Adams On Feb. 1, 2015, 11:34 a.m., Rafael Schloming wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30484/ > ----------------------------------------------------------- > > (Updated Feb. 1, 2015, 11:34 a.m.) > > > Review request for qpid and Fraser Adams. > > > Repository: qpid-proton-git > > > Description > ------- > > This patch modifies the wrapper for pn_data_format to pass a stack allocated > pointer to setValue rather than using the size directly. I don't fully > understand the details of emscripten, but my theory is that previously the > call to setValue was treating the size value (initially 1024) as a pointer > and corrupting the heap at that location. I'm guessing at higher optimization > levels, (or with different compiler versions) whatever happens to be sitting > at address 1024 is not as critical and so the memory corruption doesn't have > the same impact. > > I'm mostly guessing at a few details of the emscripten API here, so I'd > appreciate a thourough review. > > > Diffs > ----- > > proton-c/bindings/javascript/data.js 018c5fb > > Diff: https://reviews.apache.org/r/30484/diff/ > > > Testing > ------- > > With this change, the send.js/recv.js examples work fine in Debug mode. The > javascript tests also work fine. > > > Thanks, > > Rafael Schloming > >
