Bug 1529298 - Implement an ArrayBufferObject::stealMallocedContents for use in JS_StealArrayBufferContents, rather than reusing the hoary ArrayBufferObject::stealContents with finicky caller-side should-this-steal logic. r=sfink
authorJeff Walden <jwalden@mit.edu>
Wed, 20 Feb 2019 13:33:15 -0800
changeset 461604 715e9b139ebbd407ac64b5ee8737d0f042f74b4c
parent 461603 f487c864d2abb77498667161baf187b3ecb7f995
child 461605 a9638eeea75714425a28e5bcd9edb9ae55dd13b9
push id35626
push usercsabou@mozilla.com
push dateThu, 28 Feb 2019 11:31:08 +0000
treeherdermozilla-central@2ea0c1db7e60 [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 - Implement an ArrayBufferObject::stealMallocedContents for use in JS_StealArrayBufferContents, rather than reusing the hoary ArrayBufferObject::stealContents with finicky caller-side should-this-steal logic. r=sfink
js/src/jsapi-tests/testArrayBuffer.cpp
js/src/vm/ArrayBufferObject.cpp
js/src/vm/ArrayBufferObject.h
--- a/js/src/jsapi-tests/testArrayBuffer.cpp
+++ b/js/src/jsapi-tests/testArrayBuffer.cpp
@@ -209,39 +209,35 @@ BEGIN_TEST(testArrayBuffer_staticContent
   CHECK(!data.wasFreed());
 
   data.free();
   return true;
 }
 END_TEST(testArrayBuffer_staticContents)
 
 BEGIN_TEST(testArrayBuffer_stealDetachExternal) {
-  ExternalData data("One two three four");
+  static const char dataBytes[] = "One two three four";
+  ExternalData data(dataBytes);
   JS::RootedObject buffer(
       cx, JS_NewExternalArrayBuffer(cx, data.len(), data.contents(),
                                     &ExternalData::freeCallback, &data));
   CHECK(buffer);
   CHECK(!data.wasFreed());
 
   void* stolenContents = JS_StealArrayBufferContents(cx, buffer);
-  // External buffers are currently not stealable, since stealing only
-  // gives you a pointer with no indication how to free it. So this should
-  // copy the data.
+
+  // External buffers are stealable: the data is copied into freshly allocated
+  // memory, and the buffer's data pointer is cleared (immediately freeing the
+  // data) and the buffer is marked as detached.
   CHECK(stolenContents != data.contents());
-  CHECK(strcmp(reinterpret_cast<char*>(stolenContents), data.asString()) == 0);
-  // External buffers are currently not stealable, so this should keep the
-  // reference to the data and just mark the buffer as detached.
+  CHECK(strcmp(reinterpret_cast<char*>(stolenContents), dataBytes) == 0);
+  CHECK(data.wasFreed());
   CHECK(JS_IsDetachedArrayBufferObject(buffer));
-  CHECK(!data.wasFreed());
 
-  buffer = nullptr;
-  JS_GC(cx);
-  JS_GC(cx);
-  CHECK(data.wasFreed());
-
+  JS_free(cx, stolenContents);
   return true;
 }
 END_TEST(testArrayBuffer_stealDetachExternal)
 
 BEGIN_TEST(testArrayBuffer_serializeExternal) {
   JS::RootedValue serializeValue(cx);
 
   {
--- a/js/src/vm/ArrayBufferObject.cpp
+++ b/js/src/vm/ArrayBufferObject.cpp
@@ -3,16 +3,18 @@
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "vm/ArrayBufferObject-inl.h"
 #include "vm/ArrayBufferObject.h"
 
 #include "mozilla/Alignment.h"
+#include "mozilla/Assertions.h"
+#include "mozilla/Attributes.h"
 #include "mozilla/CheckedInt.h"
 #include "mozilla/FloatingPoint.h"
 #include "mozilla/Likely.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/PodOperations.h"
 #include "mozilla/TaggedAnonymousMemory.h"
 
 #include <string.h>
@@ -1366,23 +1368,83 @@ ArrayBufferObject* ArrayBufferObject::cr
   auto contents = BufferContents::createWasm(rawBuffer->dataPointer());
   buffer->setDataPointer(contents, OwnsData);
 
   cx->updateMallocCounter(initialSize);
 
   return buffer;
 }
 
+static void CheckStealPreconditions(Handle<ArrayBufferObject*> buffer,
+                                    JSContext* cx) {
+  cx->check(buffer);
+
+  MOZ_ASSERT(!buffer->isDetached(), "can't steal from a detached buffer");
+  MOZ_ASSERT(!buffer->isPreparedForAsmJS(),
+             "asm.js-prepared buffers don't have detachable/stealable data");
+}
+
+/* static */ uint8_t* ArrayBufferObject::stealMallocedContents(
+    JSContext* cx, Handle<ArrayBufferObject*> buffer) {
+  CheckStealPreconditions(buffer, cx);
+
+  switch (buffer->bufferKind()) {
+    case MALLOCED:
+      if (buffer->ownsData()) {
+        uint8_t* stolenData = buffer->dataPointer();
+        MOZ_ASSERT(stolenData);
+
+        // Overwrite the old data pointer *without* releasing the contents
+        // being stolen.
+        BufferContents newContents = BufferContents::createNoData();
+        buffer->setDataPointer(newContents, OwnsData);
+
+        // Detach |buffer| now that doing so won't free |stolenData|.
+        ArrayBufferObject::detach(cx, buffer, newContents);
+        return stolenData;
+      }
+      MOZ_FALLTHROUGH;
+
+    case INLINE_DATA:
+    case NO_DATA:
+    case USER_OWNED:
+    case MAPPED:
+    case EXTERNAL: {
+      // We can't use these data types directly.  Make a copy to return.
+      uint8_t* copiedData = NewCopiedBufferContents(cx, buffer);
+      if (!copiedData) {
+        return nullptr;
+      }
+
+      // Detach |buffer|.  This immediately releases the currently owned
+      // contents, freeing or unmapping data in the MAPPED and EXTERNAL cases.
+      ArrayBufferObject::detach(cx, buffer, BufferContents::createNoData());
+      return copiedData;
+    }
+
+    case WASM:
+      MOZ_ASSERT_UNREACHABLE(
+          "wasm buffers aren't stealable except by a "
+          "memory.grow operation that shouldn't call this "
+          "function");
+      return nullptr;
+
+    case BAD1:
+      MOZ_ASSERT_UNREACHABLE("bad kind when stealing malloc'd data");
+      return nullptr;
+  }
+
+  MOZ_ASSERT_UNREACHABLE("garbage kind computed");
+  return nullptr;
+}
+
 /* static */ ArrayBufferObject::BufferContents ArrayBufferObject::stealContents(
     JSContext* cx, Handle<ArrayBufferObject*> buffer,
     bool hasStealableContents) {
-  cx->check(buffer);
-
-  MOZ_ASSERT(!buffer->isPreparedForAsmJS(),
-             "asm.js-prepared buffers don't have detachable/stealable data");
+  CheckStealPreconditions(buffer, cx);
 
 #ifdef DEBUG
   if (hasStealableContents) {
     MOZ_ASSERT(!buffer->isInlineData(),
                "inline data is OwnsData, but it isn't malloc-allocated");
     MOZ_ASSERT(!buffer->isNoData(),
                "null |dataPointer()| for the no-data case isn't stealable "
                "because it would be confused with failure");
@@ -1848,29 +1910,18 @@ JS_PUBLIC_API void* JS_StealArrayBufferC
   }
 
   if (buffer->isWasm() || buffer->isPreparedForAsmJS()) {
     JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                               JSMSG_WASM_NO_TRANSFER);
     return nullptr;
   }
 
-  // This function returns a plain malloc'd buffer for the user to own, so
-  // stealing actual contents requires |ownsData() && isMalloced()|.
-  //
-  // We might consider returning something that handles releasing data of more
-  // exotic kind at some point.
-  //
-  // XXX The three kinds made |ownsData()| in this patch would all fail the
-  //     second test here, so the line below isn't broken by this patch.
-  bool hasStealableContents = buffer->ownsData() && buffer->isMalloced();
-
   AutoRealm ar(cx, buffer);
-  return ArrayBufferObject::stealContents(cx, buffer, hasStealableContents)
-      .data();
+  return ArrayBufferObject::stealMallocedContents(cx, buffer);
 }
 
 JS_PUBLIC_API JSObject* JS_NewMappedArrayBufferWithContents(JSContext* cx,
                                                             size_t nbytes,
                                                             void* data) {
   AssertHeapIsIdle();
   CHECK_THREAD(cx);
 
--- a/js/src/vm/ArrayBufferObject.h
+++ b/js/src/vm/ArrayBufferObject.h
@@ -346,16 +346,19 @@ 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);
 
+  static uint8_t* stealMallocedContents(JSContext* cx,
+                                        Handle<ArrayBufferObject*> buffer);
+
   static BufferContents stealContents(JSContext* cx,
                                       Handle<ArrayBufferObject*> buffer,
                                       bool hasStealableContents);
 
   static void addSizeOfExcludingThis(JSObject* obj,
                                      mozilla::MallocSizeOf mallocSizeOf,
                                      JS::ClassInfo* info);