Javascript DecoderBuffer Bindings Bug

[nlharr]

I recently encountered a fairly subtle bug with the javascript bindings of the DecoderBuffer. Unfortunately, I do not know of a quick fix that does not change the javascript DecoderBuffer API so I wanted to bring up the issue here rather than just submitting a bug fix.

For functions or methods declared in the Web IDL bindings that accept arrays as inputs, Web IDL copies the array into the web assembly heap and then calls the function with the "pointer" to the section of the heap the object is in. The lifetime of this array is only guaranteed while that function/method is running. With the DecoderBuffer API, initializing the decoder buffer with the Init method is done by passing an array and storing it in the object. This data is invalidated if another webassembly function that accepts an array as an argument is called.

To see what's going on, compile the javascript code with -O0 so the closure compiler doesn't run and then look at the code in decoder_glue.js. I've extracted the relevant portions below.

The DecoderBuffer Init method calls ensureInt8(arg0) which allocates memory for the array by calling ensureCache.alloc(value, HEAP8); The ensureCache object keeps a main buffer in memory that it attempts to use for storage. If the input array can fit in that buffer it copies it to the buffer. If the input array is too large, it allocates space on the heap for it and then uses that space. If it had to allocate a new section of memory it frees that memory the next time ensureCache.prepare() is called. A lot of calls to the web assembly code wrappers call the prepare() method, unfortunately.

In practice, the DecoderBuffer objects are often the largest array that's been passed to the DecoderBuffer object. This means they have memory allocated for them that is later freed. Freeing the memory just leaves it open to be overwritten in the future, it doesn't change the data stored at that portion of the heap. Code that is hitting this bug would work most of the time. I only was bitten by it because some of my other calls were causing the heap to be reallocated which was zeroing my decoder buffer.

There are a number of ways to fix this. You could remove the notion of the DecoderBuffer and just pass the arrays to the functions as needed. The way I went about fixing it was a bit more involved. I wanted some of the functionality from the embind based bindings so I rewrote the draco bindings using embind and made the memory management more explicit using the typed_memory_view.

Let me know if you need some code that hits this bug or if you want me to submit the embind based bindings as a pull request.

DecoderBuffer.prototype['Init'] = DecoderBuffer.prototype.Init = function(arg0, arg1) {
  var self = this.ptr;
  ensureCache.prepare();
  if (typeof arg0 == 'object') { arg0 = ensureInt8(arg0); }
  if (arg1 && typeof arg1 === 'object') arg1 = arg1.ptr;
    _emscripten_bind_DecoderBuffer_Init_2(self, arg0, arg1);
};;

function ensureInt8(value) {
  if (typeof value === 'object') {
    var offset = ensureCache.alloc(value, HEAP8);
    ensureCache.copy(value, HEAP8, offset);
    return offset;
  }
  return value;
}

var ensureCache = {
  buffer: 0,  // the main buffer of temporary storage
  size: 0,   // the size of buffer
  pos: 0,    // the next free offset in buffer
  temps: [], // extra allocations
  needed: 0, // the total size we need next time

  prepare: function() {
    if (ensureCache.needed) {
      // clear the temps
      for (var i = 0; i < ensureCache.temps.length; i++) {
        Module['_free'](ensureCache.temps[i]);
      }
      ensureCache.temps.length = 0;
      // prepare to allocate a bigger buffer
      Module['_free'](ensureCache.buffer);
      ensureCache.buffer = 0;
      ensureCache.size += ensureCache.needed;
      // clean up
      ensureCache.needed = 0;
    }
    if (!ensureCache.buffer) { // happens first time, or when we need to grow
      ensureCache.size += 128; // heuristic, avoid many small grow events
      ensureCache.buffer = Module['_malloc'](ensureCache.size);
      assert(ensureCache.buffer);
    }
    ensureCache.pos = 0;
  },
  alloc: function(array, view) {
    assert(ensureCache.buffer);
    var bytes = view.BYTES_PER_ELEMENT;
    var len = array.length * bytes;
    len = (len + 7) & -8; // keep things aligned to 8 byte boundaries
    var ret;
    if (ensureCache.pos + len >= ensureCache.size) {
      // we failed to allocate in the buffer, ensureCache time around :(
      assert(len > 0); // null terminator, at least
      ensureCache.needed += len;
      ret = Module['_malloc'](len);
      ensureCache.temps.push(ret);
    } else {
      // we can allocate in the buffer
      ret = ensureCache.buffer + ensureCache.pos;
      ensureCache.pos += len;
    }
    return ret;
  },
  copy: function(array, view, offset) {
    var offsetShifted = offset;
    var bytes = view.BYTES_PER_ELEMENT;
    switch (bytes) {
      case 2: offsetShifted >>= 1; break;
      case 4: offsetShifted >>= 2; break;
      case 8: offsetShifted >>= 3; break;
    }
    for (var i = 0; i < array.length; i++) {
      view[offsetShifted + i] = array[i];
    }
  },
};

[ondys]

thanks for the report, we will look into this

[FrankGalligan]

Fixed in 1.4.0