Bug 1529298 - Remove JS_ExternalizeArrayBufferContents because it has no users except in tests, implements complicated ownership semantics, and is definite implementation complexity. r=sfink
☠☠ backed out by eed1098d0d6c ☠ ☠
authorJeff Walden <jwalden@mit.edu>
Mon, 18 Feb 2019 22:52:41 -0800
changeset 460363 432b2e88c651908122e95cc5bdca63aed1758217
parent 460362 3512de18097c797cdbeec56b56efdf3e36eb3399
child 460364 5be8cb19ad3d9f581bacf4a6acf9a7dba43dd5da
push id112082
push userjwalden@mit.edu
push dateFri, 22 Feb 2019 02:22:21 +0000
treeherdermozilla-inbound@d80b681a68e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1529298
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1529298 - Remove JS_ExternalizeArrayBufferContents because it has no users except in tests, implements complicated ownership semantics, and is definite implementation complexity. r=sfink
js/src/jsapi-tests/testArrayBuffer.cpp
js/src/jsapi.h
js/src/vm/ArrayBufferObject.cpp
js/src/vm/ArrayBufferObject.h
--- a/js/src/jsapi-tests/testArrayBuffer.cpp
+++ b/js/src/jsapi-tests/testArrayBuffer.cpp
@@ -154,83 +154,16 @@ static void GC(JSContext* cx) {
 
 bool hasDetachedBuffer(JS::HandleObject obj) {
   JS::RootedValue v(cx);
   return JS_GetProperty(cx, obj, "byteLength", &v) && v.toInt32() == 0;
 }
 
 END_TEST(testArrayBuffer_bug720949_viewList)
 
-BEGIN_TEST(testArrayBuffer_externalize) {
-  if (!testWithSize(cx, 2)) {  // ArrayBuffer data stored inline in the object.
-    return false;
-  }
-  if (!testWithSize(cx, 2000)) {  // ArrayBuffer data stored out-of-line in a
-                                  // separate heap allocation.
-    return false;
-  }
-
-  return true;
-}
-
-bool testWithSize(JSContext* cx, size_t n) {
-  JS::RootedObject buffer(cx, JS_NewArrayBuffer(cx, n));
-  CHECK(buffer != nullptr);
-
-  JS::RootedObject view(cx, JS_NewUint8ArrayWithBuffer(cx, buffer, 0, -1));
-  CHECK(view != nullptr);
-
-  void* contents = JS_ExternalizeArrayBufferContents(cx, buffer);
-  CHECK(contents != nullptr);
-  uint32_t actualLength;
-  CHECK(hasExpectedLength(cx, view, &actualLength));
-  CHECK(actualLength == n);
-  CHECK(!JS_IsDetachedArrayBufferObject(buffer));
-  CHECK(JS_GetArrayBufferByteLength(buffer) == uint32_t(n));
-
-  uint8_t* uint8Contents = static_cast<uint8_t*>(contents);
-  CHECK(*uint8Contents == 0);
-  uint8_t randomByte(rand() % UINT8_MAX);
-  *uint8Contents = randomByte;
-
-  JS::RootedValue v(cx);
-  CHECK(JS_GetElement(cx, view, 0, &v));
-  CHECK(v.toInt32() == randomByte);
-
-  view = nullptr;
-  GC(cx);
-
-  CHECK(JS_DetachArrayBuffer(cx, buffer));
-  GC(cx);
-  CHECK(*uint8Contents == randomByte);
-  JS_free(cx, contents);
-  GC(cx);
-  buffer = nullptr;
-  GC(cx);
-
-  return true;
-}
-
-static void GC(JSContext* cx) {
-  JS_GC(cx);
-  JS_GC(cx);  // Trigger another to wait for background finalization to end
-}
-
-static bool hasExpectedLength(JSContext* cx, JS::HandleObject obj,
-                              uint32_t* len) {
-  JS::RootedValue v(cx);
-  if (!JS_GetProperty(cx, obj, "byteLength", &v)) {
-    return false;
-  }
-  *len = v.toInt32();
-  return true;
-}
-
-END_TEST(testArrayBuffer_externalize)
-
 BEGIN_TEST(testArrayBuffer_customFreeFunc) {
   ExternalData data("One two three four");
 
   // The buffer takes ownership of the data.
   JS::RootedObject buffer(
       cx, JS_NewExternalArrayBuffer(cx, data.len(), data.contents(),
                                     &ExternalData::freeCallback, &data));
   CHECK(buffer);
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -2078,40 +2078,16 @@ extern JS_PUBLIC_API JSObject* JS_NewArr
  * length set to 0 and its contents array cleared. The caller takes ownership
  * of the return value and must free it or transfer ownership via
  * JS_NewArrayBufferWithContents when done using it.
  */
 extern JS_PUBLIC_API void* JS_StealArrayBufferContents(JSContext* cx,
                                                        JS::HandleObject obj);
 
 /**
- * Returns a pointer to the ArrayBuffer |obj|'s data.  |obj| and its views will
- * store and expose the data in the returned pointer: assigning into the
- * returned pointer will affect values exposed by views of |obj| and vice versa.
- * Writes into the pointer must be synchronized with the current thread,
- * typically by occurring *only on* the current thread.
- *
- * The caller must ultimately deallocate the returned pointer to avoid leaking.
- * The memory is *not* garbage-collected with |obj|.  These steps must be
- * followed to deallocate:
- *
- * 1. The ArrayBuffer |obj| must be detached using JS_DetachArrayBuffer.
- * 2. The returned pointer must be freed using JS_free.
- *
- * To perform step 1, callers *must* hold a reference to |obj| until they finish
- * using the returned pointer.  They *must not* attempt to let |obj| be GC'd,
- * then JS_free the pointer.
- *
- * If |obj| isn't an ArrayBuffer, this function returns null and reports an
- * error.
- */
-extern JS_PUBLIC_API void* JS_ExternalizeArrayBufferContents(
-    JSContext* cx, JS::HandleObject obj);
-
-/**
  * Create a new mapped array buffer with the given memory mapped contents. It
  * must be legal to free the contents pointer by unmapping it. On success,
  * ownership is transferred to the new mapped array buffer.
  */
 extern JS_PUBLIC_API JSObject* JS_NewMappedArrayBufferWithContents(
     JSContext* cx, size_t nbytes, void* contents);
 
 /**
--- a/js/src/vm/ArrayBufferObject.cpp
+++ b/js/src/vm/ArrayBufferObject.cpp
@@ -1313,83 +1313,16 @@ ArrayBufferObject* ArrayBufferObject::cr
   auto contents = BufferContents::create<WASM>(buffer->dataPointer());
   obj->setDataPointer(contents, OwnsData);
 
   cx->updateMallocCounter(initialSize);
 
   return obj;
 }
 
-/* static */ void*
-ArrayBufferObject::exposeMallocedContents(JSContext* cx,
-                                          Handle<ArrayBufferObject*> buffer) {
-  // A detached ArrayBuffer lacks contents to expose.
-  if (buffer->isDetached()) {
-    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
-                              JSMSG_TYPED_ARRAY_DETACHED);
-    return nullptr;
-  }
-
-  // The contents must be malloc'd or stored inline, because this function
-  // returns a JS_free-able pointer.
-  //
-  // Mapped and wasm ArrayBufferObjects' memory is not so allocated, and we
-  // can't blithely allocate a copy of the data and swap it into |buffer|
-  // because JIT code may hold pointers into the mapped/wasm data.
-  //
-  // We also don't support replacing user-owned data (with malloced data)
-  // because such ArrayBuffers must be passed to JS_DetachArrayBuffer before
-  // the associated memory is freed, and that call is guaranteed to succeed
-  // -- but malloced data can be used with asm.js, and JS_DetachArrayBuffer
-  // will fail on an ArrayBuffer used with asm.js.
-  //
-  // These restrictions are very hoary.  Possibly, in a future where
-  // ArrayBuffer's contents are not represented so trickily, we might relax
-  // them -- maybe by returning something that has the ability to release the
-  // returned memory.
-  if (!buffer->isMalloced() && !buffer->isInlineData()) {
-    MOZ_ASSERT(buffer->hasUserOwnedData() || buffer->isWasm() ||
-               buffer->isMapped() || buffer->isExternal());
-
-    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
-                              JSMSG_TYPED_ARRAY_BAD_ARGS);
-    return nullptr;
-  }
-
-  MOZ_ASSERT_IF(buffer->isInlineData(), !buffer->ownsData());
-  MOZ_ASSERT_IF(buffer->ownsData(), buffer->isMalloced());
-
-  BufferContents contents = buffer->contents();
-
-  // If the buffer owns its data and the data isn't needed for an asm.js
-  // compilation, we can just mark the data as unowned and return it -- no need
-  // to actively replace.
-  if (buffer->ownsData() && !buffer->isPreparedForAsmJS()) {
-    MOZ_ASSERT(buffer->hasStealableContents());
-    buffer->setOwnsData(DoesntOwnData);
-    return contents.data();
-  }
-
-  MOZ_ASSERT(!buffer->hasStealableContents());
-
-  // Otherwise we must allocate new contents, fill them with the old contents,
-  // slot them into place as unowned contents (freeing the old contents in the
-  // process), then return an owning pointer to the new contents.
-  BufferContents newContents =
-      AllocateArrayBufferContents(cx, buffer->byteLength());
-  if (!newContents) {
-    return nullptr;
-  }
-
-  memcpy(newContents.data(), contents.data(), buffer->byteLength());
-  buffer->changeContents(cx, newContents, DoesntOwnData);
-
-  return newContents.data();
-}
-
 /* static */ ArrayBufferObject::BufferContents ArrayBufferObject::stealContents(
     JSContext* cx, Handle<ArrayBufferObject*> buffer,
     bool hasStealableContents) {
   // While wasm buffers cannot generally be transferred by content, the
   // stealContents() is used internally by the impl of memory growth.
   MOZ_ASSERT_IF(hasStealableContents,
                 buffer->hasStealableContents() ||
                     (buffer->isWasm() && !buffer->isPreparedForAsmJS()));
@@ -1780,32 +1713,16 @@ JS_FRIEND_API bool JS_ArrayBufferHasData
 JS_FRIEND_API JSObject* js::UnwrapArrayBuffer(JSObject* obj) {
   return obj->maybeUnwrapIf<ArrayBufferObject>();
 }
 
 JS_FRIEND_API JSObject* js::UnwrapSharedArrayBuffer(JSObject* obj) {
   return obj->maybeUnwrapIf<SharedArrayBufferObject>();
 }
 
-JS_PUBLIC_API void* JS_ExternalizeArrayBufferContents(JSContext* cx,
-                                                      HandleObject obj) {
-  AssertHeapIsIdle();
-  CHECK_THREAD(cx);
-  cx->check(obj);
-
-  if (!obj->is<ArrayBufferObject>()) {
-    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
-                              JSMSG_TYPED_ARRAY_BAD_ARGS);
-    return nullptr;
-  }
-
-  Handle<ArrayBufferObject*> buffer = obj.as<ArrayBufferObject>();
-  return ArrayBufferObject::exposeMallocedContents(cx, buffer);
-}
-
 JS_PUBLIC_API void* JS_StealArrayBufferContents(JSContext* cx,
                                                 HandleObject objArg) {
   AssertHeapIsIdle();
   CHECK_THREAD(cx);
   cx->check(objArg);
 
   JSObject* obj = CheckedUnwrapStatic(objArg);
   if (!obj) {
--- a/js/src/vm/ArrayBufferObject.h
+++ b/js/src/vm/ArrayBufferObject.h
@@ -334,27 +334,16 @@ class ArrayBufferObject : public ArrayBu
                                                    uint32_t initialSize);
 
   static void copyData(Handle<ArrayBufferObject*> toBuffer, uint32_t toIndex,
                        Handle<ArrayBufferObject*> fromBuffer,
                        uint32_t fromIndex, uint32_t count);
 
   static size_t objectMoved(JSObject* obj, JSObject* old);
 
-  // Convert this ArrayBuffer's storage to memory allocated using the standard
-  // SpiderMonkey allocator (if the storage *is* memory so allocated, this will
-  // be a no-op), then return a pointer to that memory *that the caller now
-  // owns*, for same-thread manipulation by the caller.  The caller must at a
-  // later time call |JS_DetachArrayBuffer| on |buffer|, then free the
-  // SpiderMonkey-allocated pointer in the usual way.
-  //
-  // Returns null leaving |buffer| unaltered on failure.
-  static void* exposeMallocedContents(JSContext* cx,
-                                      Handle<ArrayBufferObject*> buffer);
-
   static BufferContents stealContents(JSContext* cx,
                                       Handle<ArrayBufferObject*> buffer,
                                       bool hasStealableContents);
 
   bool hasStealableContents() const {
     // Inline elements strictly adhere to the corresponding buffer.
     return ownsData() && !isPreparedForAsmJS() && !isWasm();
   }