Bug 1529298 - Split PLAIN into PLAIN_DATA and USER_OWNED ArrayBuffer …

…data types to clearly segregate the two, rather than categorizing them both as the same thing. r=sfink

js/src/jsapi.h

@@ -2022,7 +2022,7 @@ JS_PUBLIC_API void JS_SetAllNonReservedSlotsToUndefined(JSContext* cx,
 
 /**
  * Create a new array buffer with the given contents. It must be legal to pass
- * these contents to free(). On success, the ownership is transferred to the
+ * these contents to JS_free(). On success, the ownership is transferred to the
  * new array buffer.
  */
 extern JS_PUBLIC_API JSObject* JS_NewArrayBufferWithContents(JSContext* cx,
@@ -2067,8 +2067,8 @@ extern JS_PUBLIC_API JSObject* JS_NewExternalArrayBuffer(
 
 /**
  * Create a new array buffer with the given contents.  The array buffer does not
- * take ownership of contents, and JS_DetachArrayBuffer must be called before
- * the contents are disposed of.
+ * take ownership of contents.  JS_DetachArrayBuffer must be called before
+ * the contents are disposed of by the user; this call will always succeed.
  */
 extern JS_PUBLIC_API JSObject* JS_NewArrayBufferWithExternalContents(
     JSContext* cx, size_t nbytes, void* contents);

js/src/jsfriendapi.h

@@ -1943,7 +1943,9 @@ extern JS_FRIEND_API JSObject* JS_GetArrayBufferViewBuffer(
  * Detach an ArrayBuffer, causing all associated views to no longer refer to
  * the ArrayBuffer's original attached memory.
  *
- * The |changeData| argument is obsolete and ignored.
+ * This function throws only if it is provided a non-ArrayBuffer object or if
+ * the provided ArrayBuffer is a WASM-backed ArrayBuffer or an ArrayBuffer used
+ * in asm.js code.
  */
 extern JS_FRIEND_API bool JS_DetachArrayBuffer(JSContext* cx,
                                                JS::HandleObject obj);

js/src/shell/js.cpp

@@ -1916,9 +1916,9 @@ static bool CacheEntry_setBytecode(JSContext* cx, HandleObject cache,
                                    uint8_t* buffer, uint32_t length) {
   MOZ_ASSERT(CacheEntry_isCacheEntry(cache));
 
-  ArrayBufferObject::BufferContents contents =
-      ArrayBufferObject::BufferContents::create<ArrayBufferObject::PLAIN>(
-          buffer);
+  using BufferContents = ArrayBufferObject::BufferContents;
+
+  BufferContents contents = BufferContents::createPlainData(buffer);
   Rooted<ArrayBufferObject*> arrayBuffer(
       cx, ArrayBufferObject::create(cx, length, contents));
   if (!arrayBuffer) {

js/src/vm/ArrayBufferObject.cpp

@@ -445,7 +445,7 @@ static ArrayBufferObject::BufferContents AllocateArrayBufferContents(
     JSContext* cx, uint32_t nbytes) {
   uint8_t* p =
       cx->pod_callocCanGC<uint8_t>(nbytes, js::ArrayBufferContentsArena);
-  return ArrayBufferObject::BufferContents::create<ArrayBufferObject::PLAIN>(p);
+  return ArrayBufferObject::BufferContents::createPlainData(p);
 }
 
 static void NoteViewBufferWasDetached(
@@ -932,7 +932,19 @@ bool js::CreateWasmBuffer(JSContext* cx, const wasm::Limits& memory,
     return false;
   }
 
-  MOZ_ASSERT(buffer->isPlain() || buffer->isMapped() || buffer->isExternal());
+  // asm.js code and associated buffers are potentially long-lived.  Yet if
+  // |buffer->hasUserOwnedData()|, |buffer| *must* be detached by the user
+  // before the user-provided data is disposed.  Eliminate the complexity of
+  // this edge case by not allowing buffers with user-provided content to be
+  // used with asm.js, as no callers exist that want to use such buffer with
+  // asm.js.
+  if (buffer->hasUserOwnedData()) {
+    MOZ_ASSERT(!buffer->isPreparedForAsmJS());
+    return false;
+  }
+
+  MOZ_ASSERT(buffer->isPlainData() || buffer->isMapped() ||
+             buffer->isExternal());
 
   // Buffers already prepared for asm.js need no further work.
   if (buffer->isPreparedForAsmJS()) {
@@ -981,9 +993,12 @@ void ArrayBufferObject::releaseData(FreeOp* fop) {
   MOZ_ASSERT(ownsData());
 
   switch (bufferKind()) {
-    case PLAIN:
+    case PLAIN_DATA:
       fop->free_(dataPointer());
       break;
+    case USER_OWNED:
+      MOZ_ASSERT_UNREACHABLE("user-owned data should never be owned by this");
+      break;
     case MAPPED:
       gc::DeallocateMappedContent(dataPointer(), byteLength());
       break;
@@ -1003,7 +1018,6 @@ void ArrayBufferObject::releaseData(FreeOp* fop) {
     case BAD1:
     case BAD2:
     case BAD3:
-    case BAD4:
       MOZ_CRASH("invalid BufferKind encountered");
       break;
   }
@@ -1124,7 +1138,8 @@ Maybe<uint32_t> js::WasmArrayBufferMaxSize(
   newBuf->initialize(newSize, contents, OwnsData);
 
   memcpy(newBuf->dataPointer(), oldBuf->dataPointer(), oldBuf->byteLength());
-  ArrayBufferObject::detach(cx, oldBuf, BufferContents::createPlain(nullptr));
+  ArrayBufferObject::detach(cx, oldBuf,
+                            BufferContents::createPlainData(nullptr));
   return true;
 }
 
@@ -1208,7 +1223,7 @@ ArrayBufferObject* ArrayBufferObject::create(
       int newSlots = JS_HOWMANY(nbytes, sizeof(Value));
       MOZ_ASSERT(int(nbytes) <= newSlots * int(sizeof(Value)));
       nslots = reservedSlots + newSlots;
-      contents = BufferContents::createPlain(nullptr);
+      contents = BufferContents::createPlainData(nullptr);
     } else {
       contents = AllocateArrayBufferContents(cx, nbytes);
       if (!contents) {
@@ -1238,7 +1253,8 @@ ArrayBufferObject* ArrayBufferObject::create(
   if (!contents) {
     void* data = obj->inlineDataPointer();
     memset(data, 0, nbytes);
-    obj->initialize(nbytes, BufferContents::createPlain(data), DoesntOwnData);
+    obj->initialize(nbytes, BufferContents::createPlainData(data),
+                    DoesntOwnData);
   } else {
     obj->initialize(nbytes, contents, ownsState);
   }
@@ -1248,7 +1264,7 @@ ArrayBufferObject* ArrayBufferObject::create(
 
 ArrayBufferObject* ArrayBufferObject::create(
     JSContext* cx, uint32_t nbytes, HandleObject proto /* = nullptr */) {
-  return create(cx, nbytes, BufferContents::createPlain(nullptr),
+  return create(cx, nbytes, BufferContents::createPlainData(nullptr),
                 OwnsState::OwnsData, proto);
 }
 
@@ -1262,7 +1278,7 @@ ArrayBufferObject* ArrayBufferObject::createEmpty(JSContext* cx) {
   obj->setByteLength(0);
   obj->setFlags(0);
   obj->setFirstView(nullptr);
-  obj->setDataPointer(BufferContents::createPlain(nullptr), DoesntOwnData);
+  obj->setDataPointer(BufferContents::createPlainData(nullptr), DoesntOwnData);
 
   return obj;
 }
@@ -1292,7 +1308,8 @@ ArrayBufferObject* ArrayBufferObject::createFromNewRawBuffer(
 ArrayBufferObject::externalizeContents(JSContext* cx,
                                        Handle<ArrayBufferObject*> buffer,
                                        bool hasStealableContents) {
-  MOZ_ASSERT(buffer->isPlain(), "Only support doing this on plain ABOs");
+  MOZ_ASSERT(buffer->isPlainData(),
+             "only support doing this on ABOs containing plain data");
   MOZ_ASSERT(!buffer->isDetached(), "must have contents to externalize");
   MOZ_ASSERT_IF(hasStealableContents, buffer->hasStealableContents());
 
@@ -1331,7 +1348,7 @@ ArrayBufferObject::externalizeContents(JSContext* cx,
   if (hasStealableContents) {
     // Return the old contents and reset the detached buffer's data
     // pointer. This pointer should never be accessed.
-    auto newContents = BufferContents::createPlain(nullptr);
+    auto newContents = BufferContents::createPlainData(nullptr);
     buffer->setOwnsData(DoesntOwnData);  // Do not free the stolen data.
     ArrayBufferObject::detach(cx, buffer, newContents);
     buffer->setOwnsData(DoesntOwnData);  // Do not free the nullptr.
@@ -1362,7 +1379,7 @@ ArrayBufferObject::externalizeContents(JSContext* cx,
   }
 
   switch (buffer.bufferKind()) {
-    case PLAIN:
+    case PLAIN_DATA:
       if (buffer.isPreparedForAsmJS()) {
         info->objectsMallocHeapElementsAsmJS +=
             mallocSizeOf(buffer.dataPointer());
@@ -1371,6 +1388,11 @@ ArrayBufferObject::externalizeContents(JSContext* cx,
             mallocSizeOf(buffer.dataPointer());
       }
       break;
+    case USER_OWNED:
+      MOZ_ASSERT_UNREACHABLE(
+          "user-owned data should never be owned by this, and such memory "
+          "should be accounted for by the code that provided it");
+      break;
     case MAPPED:
       info->objectsNonHeapElementsNormal += buffer.byteLength();
       break;
@@ -1385,7 +1407,6 @@ ArrayBufferObject::externalizeContents(JSContext* cx,
     case BAD1:
     case BAD2:
     case BAD3:
-    case BAD4:
       MOZ_CRASH("bad bufferKind()");
   }
 }
@@ -1622,7 +1643,7 @@ JS_FRIEND_API bool JS_DetachArrayBuffer(JSContext* cx, HandleObject obj) {
 
   ArrayBufferObject::BufferContents newContents =
       buffer->hasStealableContents()
-          ? ArrayBufferObject::BufferContents::createPlain(nullptr)
+          ? ArrayBufferObject::BufferContents::createPlainData(nullptr)
           : buffer->contents();
 
   ArrayBufferObject::detach(cx, buffer, newContents);
@@ -1653,8 +1674,9 @@ JS_PUBLIC_API JSObject* JS_NewArrayBufferWithContents(JSContext* cx,
   CHECK_THREAD(cx);
   MOZ_ASSERT_IF(!data, nbytes == 0);
 
-  ArrayBufferObject::BufferContents contents =
-      ArrayBufferObject::BufferContents::create<ArrayBufferObject::PLAIN>(data);
+  using BufferContents = ArrayBufferObject::BufferContents;
+
+  BufferContents contents = BufferContents::createPlainData(data);
   return ArrayBufferObject::create(cx, nbytes, contents,
                                    ArrayBufferObject::OwnsData,
                                    /* proto = */ nullptr, TenuredObject);
@@ -1683,8 +1705,10 @@ JS_PUBLIC_API JSObject* JS_NewArrayBufferWithExternalContents(JSContext* cx,
   AssertHeapIsIdle();
   CHECK_THREAD(cx);
   MOZ_ASSERT_IF(!data, nbytes == 0);
-  ArrayBufferObject::BufferContents contents =
-      ArrayBufferObject::BufferContents::create<ArrayBufferObject::PLAIN>(data);
+
+  using BufferContents = ArrayBufferObject::BufferContents;
+
+  BufferContents contents = BufferContents::createUserOwned(data);
   return ArrayBufferObject::create(cx, nbytes, contents,
                                    ArrayBufferObject::DoesntOwnData,
                                    /* proto = */ nullptr, TenuredObject);
@@ -1719,8 +1743,9 @@ JS_PUBLIC_API void* JS_ExternalizeArrayBufferContents(JSContext* cx,
   }
 
   Handle<ArrayBufferObject*> buffer = obj.as<ArrayBufferObject>();
-  if (!buffer->isPlain()) {
-    // This operation isn't supported on mapped or wsm ArrayBufferObjects.
+  if (!buffer->isPlainData()) {
+    // This operation isn't supported on mapped or wasm ArrayBufferObjects, or
+    // on ArrayBufferObjects with user-provided data.
     JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                               JSMSG_TYPED_ARRAY_BAD_ARGS);
     return nullptr;
@@ -1773,12 +1798,16 @@ JS_PUBLIC_API void* JS_StealArrayBufferContents(JSContext* cx,
     return nullptr;
   }
 
-  // The caller assumes that a plain malloc'd buffer is returned.
-  // hasStealableContents is true for mapped buffers, so we must additionally
-  // require that the buffer is plain. In the future, we could consider
-  // returning something that handles releasing the memory.
+  // The caller assumes that a plain malloc'd buffer is returned.  To steal
+  // actual contents, then, we must have |hasStealableContents()| *and* the
+  // contents must be |isPlainData()|.  (Mapped data would not be malloc'd;
+  // user-provided data we flat-out know nothing about at all -- although it
+  // *should* have not passed the |hasStealableContents()| check anyway.)
+  //
+  // In the future, we could consider returning something that handles
+  // releasing the memory, in the mapped-data case.
   bool hasStealableContents =
-      buffer->hasStealableContents() && buffer->isPlain();
+      buffer->hasStealableContents() && buffer->isPlainData();
 
   AutoRealm ar(cx, buffer);
   return ArrayBufferObject::stealContents(cx, buffer, hasStealableContents)

js/src/vm/ArrayBufferObject.h

@@ -175,17 +175,25 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared {
   };
 
   enum BufferKind {
-    PLAIN = 0b000,  // malloced or inline data
-    WASM = 0b001,
-    MAPPED = 0b010,
-    EXTERNAL = 0b011,
+    /** Malloced or inline data. */
+    PLAIN_DATA = 0b000,
+
+    /**
+     * User-owned memory.  The associated buffer must be manually detached
+     * before the user invalidates (deallocates, reuses the storage of, &c.)
+     * the user-owned memory.
+     */
+    USER_OWNED = 0b001,
+
+    WASM = 0b010,
+    MAPPED = 0b011,
+    EXTERNAL = 0b100,
 
     // These kind-values are currently invalid.  We intend to expand valid
     // BufferKinds in the future to either partly or fully use these values.
-    BAD1 = 0b100,
-    BAD2 = 0b101,
-    BAD3 = 0b110,
-    BAD4 = 0b111,
+    BAD1 = 0b101,
+    BAD2 = 0b110,
+    BAD3 = 0b111,
 
     KIND_MASK = 0b111
   };
@@ -211,8 +219,9 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared {
     // Views of this buffer might include typed objects.
     TYPED_OBJECT_VIEWS = 0b10'0000,
 
-    // This PLAIN, MAPPED, or EXTERNAL buffer (only WASM is excluded) has been
-    // prepared for asm.js and cannot henceforth be transferred/detached.
+    // This PLAIN_DATA, MAPPED, or EXTERNAL buffer (only WASM and USER_OWNED
+    // are excluded) has been prepared for asm.js and cannot henceforth be
+    // transferred/detached.
     FOR_ASMJS = 0b100'0000,
   };
 
@@ -249,8 +258,12 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared {
       return BufferContents(static_cast<uint8_t*>(data), Kind);
     }
 
-    static BufferContents createPlain(void* data) {
-      return BufferContents(static_cast<uint8_t*>(data), PLAIN);
+    static BufferContents createPlainData(void* data) {
+      return BufferContents(static_cast<uint8_t*>(data), PLAIN_DATA);
+    }
+
+    static BufferContents createUserOwned(void* data) {
+      return BufferContents(static_cast<uint8_t*>(data), USER_OWNED);
     }
 
     static BufferContents createExternal(void* data,
@@ -261,7 +274,7 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared {
     }
 
     static BufferContents createFailed() {
-      return BufferContents(nullptr, PLAIN);
+      return BufferContents(nullptr, PLAIN_DATA);
     }
 
     uint8_t* data() const { return data_; }
@@ -383,7 +396,10 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared {
   BufferKind bufferKind() const {
     return BufferKind(flags() & BUFFER_KIND_MASK);
   }
-  bool isPlain() const { return bufferKind() == PLAIN; }
+
+  bool isPlainData() const { return bufferKind() == PLAIN_DATA; }
+  bool hasUserOwnedData() const { return bufferKind() == USER_OWNED; }
+
   bool isWasm() const { return bufferKind() == WASM; }
   bool isMapped() const { return bufferKind() == MAPPED; }
   bool isExternal() const { return bufferKind() == EXTERNAL; }
@@ -432,7 +448,8 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared {
   void setIsDetached() { setFlags(flags() | DETACHED); }
   void setIsPreparedForAsmJS() {
     MOZ_ASSERT(!isWasm());
-    MOZ_ASSERT(isPlain() || isMapped() || isExternal());
+    MOZ_ASSERT(!hasUserOwnedData());
+    MOZ_ASSERT(isPlainData() || isMapped() || isExternal());
     setFlags(flags() | FOR_ASMJS);
   }
 

toolkit/recordreplay/ipc/ParentGraphics.cpp

@@ -10,6 +10,7 @@
 #include "ParentInternal.h"
 
 #include "chrome/common/mach_ipc_mac.h"
+#include "mozilla/Assertions.h"
 #include "mozilla/ClearOnShutdown.h"
 #include "mozilla/StaticPtr.h"
 #include "mozilla/dom/TabChild.h"
@@ -163,7 +164,9 @@ void UpdateGraphicsInUIProcess(const PaintMessage* aMsg) {
   AutoSafeJSContext cx;
   JSAutoRealm ar(cx, xpc::PrivilegedJunkScope());
 
-  JSObject* bufferObject =
+  // Create an ArrayBuffer whose contents are the externally-provided |memory|.
+  JS::RootedObject bufferObject(cx);
+  bufferObject =
       JS_NewArrayBufferWithExternalContents(cx, width * height * 4, memory);
   MOZ_RELEASE_ASSERT(bufferObject);
 
@@ -173,6 +176,11 @@ void UpdateGraphicsInUIProcess(const PaintMessage* aMsg) {
   if (NS_FAILED(gGraphics->UpdateCanvas(buffer, width, height, hadFailure))) {
     MOZ_CRASH("UpdateGraphicsInUIProcess");
   }
+
+  // Manually detach this ArrayBuffer once this update completes, as the
+  // JS_NewArrayBufferWithExternalContents API mandates.  (The API also
+  // guarantees that this call always succeeds.)
+  MOZ_ALWAYS_TRUE(JS_DetachArrayBuffer(cx, bufferObject));
 }
 
 static void MaybeTriggerExplicitPaint() {