tree 51b71c3bc1ee0ee090b7c00a0ddc71f19c4b80b0
parent 746473b904e33715a8b148047eedb7aeb2facb0e
author Kevin Lubick <kjlubick@google.com> 1591355628 -0400
committer Kevin Lubick <kjlubick@google.com> 1591363966 +0000

[canvaskit] Fix infrequent crash in SkFontMgr.FromData

The bug here is very subtle, as is the mitigation.

Quick background on WASM memory, there is an object
called wasmMemory (which might be hoisted into scope for
CanvasKit's pre-js functions), of type WebAssembly.Memory
which is a resizable ArrayBuffer. Emscripten provides the
JS code to initialize this and handle size increases.
Emscripten also provides TypedArray "views" into this buffer.
These are called CanvasKit.HEAPU8, CanvasKit.HEAPF32, etc.

When there is a call to CanvasKit._malloc, wasmMemory may
be resized. If that happens, the previous TypedArray views
become invalid. However, in the same call to _malloc,
emscripten will refresh the views [1]. So, dealing with
CanvasKit.HEAPU8 directly (quick aside, we never expect clients
to mess with these views, only us in our glue JS code
[e.g. interface.js]), should always be safe because if they
were to be invalidated in a call to _malloc, the views would
be refreshed before _malloc continues.

The problem that existed before was when we were passing
CanvasKit.HEAP* as a parameter to a function, in which the
function would call _malloc before using the typed array
parameter:
  //... let us suppose wasmMemory is backed by ArrayBuffer D
  copy1dArray(arr, HEAPU32);
  // The HEAPU32 TypedArray (backed by ArrayBuffer D) is stored
  // to a function parameter "dest"
    function copy1dArray(arr, dest, ptr) {
    // ...
    if (!ptr) {
      ptr = CanvasKit._malloc(arr.length * dest.BYTES_PER_ELEMENT);
      // Suppose _malloc needs to resize wasmMemory and is
      // now backed by ArrayBuffer E.
      // Note: The field CanvasKit.HEAPU32 is correctly backed
      // by ArrayBuffer E, but variable dest still points to a
      // TypedArray backed by ArrayBuffer D.
    }
    // dest.set will fail with a "neutered ArrayBuffer" error
    // because ArrayBuffer D is effectively gone (replaced by E).
    dest.set(arr, ptr / dest.BYTES_PER_ELEMENT);

The fix here is to pass in the field name indicating the TypedArray
view we want to write our data into instead of using the
view itself as the parameter.

[1] https://github.com/emscripten-core/emscripten/blob/e4271595539cf1ca81128280cdc72f7245e700a0/src/preamble.js#L344


Change-Id: I46cfb98f8bdf928b61690a5ced034a5961356398
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/294516
Reviewed-by: Nathaniel Nifong <nifong@google.com>