Bug 1633598 - Make AllocateArrayBufferContents return a UniquePtr to handle manually freeing memory in case of early error. r=sfink
☠☠ backed out by 16e24bb092f6 ☠ ☠
authorJeff Walden <jwalden@mit.edu>
Thu, 07 May 2020 20:52:29 +0000
changeset 528685 414d909e053acd4f04ed30d20f7f6b0c6773d68d
parent 528684 b76e3e988fdd20bb726468ab1684127b53ddf833
child 528686 78ae14106ac70e4815709a6a7a563914fb06760c
push id37393
push userrmaries@mozilla.com
push dateFri, 08 May 2020 03:38:07 +0000
treeherdermozilla-central@ead8f0367372 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1633598
milestone78.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 1633598 - Make AllocateArrayBufferContents return a UniquePtr to handle manually freeing memory in case of early error. r=sfink Differential Revision: https://phabricator.services.mozilla.com/D73553
js/src/vm/ArrayBufferObject.cpp
js/src/vm/JSContext.h
--- a/js/src/vm/ArrayBufferObject.cpp
+++ b/js/src/vm/ArrayBufferObject.cpp
@@ -406,31 +406,43 @@ bool ArrayBufferObject::class_constructo
   JSObject* bufobj = createZeroed(cx, uint32_t(byteLength), proto);
   if (!bufobj) {
     return false;
   }
   args.rval().setObject(*bufobj);
   return true;
 }
 
-static uint8_t* AllocateArrayBufferContents(JSContext* cx, uint32_t nbytes) {
-  auto* p =
-      cx->pod_arena_callocCanGC<uint8_t>(js::ArrayBufferContentsArena, nbytes);
-  if (!p) {
-    ReportOutOfMemory(cx);
+using ArrayBufferContents = UniquePtr<uint8_t[], JS::FreePolicy>;
+
+static ArrayBufferContents AllocateArrayBufferContents(JSContext* cx,
+                                                       uint32_t nbytes) {
+  // First attempt a normal allocation.
+  uint8_t* p =
+      cx->maybe_pod_arena_calloc<uint8_t>(js::ArrayBufferContentsArena, nbytes);
+  if (MOZ_UNLIKELY(!p)) {
+    // Otherwise attempt a large allocation, calling the
+    // large-allocation-failure callback if necessary.
+    p = static_cast<uint8_t*>(cx->runtime()->onOutOfMemoryCanGC(
+        js::AllocFunction::Calloc, js::ArrayBufferContentsArena, nbytes));
+    if (!p) {
+      ReportOutOfMemory(cx);
+    }
   }
-  return p;
+
+  return ArrayBufferContents(p);
 }
 
-static uint8_t* NewCopiedBufferContents(JSContext* cx,
-                                        Handle<ArrayBufferObject*> buffer) {
-  uint8_t* dataCopy = AllocateArrayBufferContents(cx, buffer->byteLength());
+static ArrayBufferContents NewCopiedBufferContents(
+    JSContext* cx, Handle<ArrayBufferObject*> buffer) {
+  ArrayBufferContents dataCopy =
+      AllocateArrayBufferContents(cx, buffer->byteLength());
   if (dataCopy) {
     if (auto count = buffer->byteLength()) {
-      memcpy(dataCopy, buffer->dataPointer(), count);
+      memcpy(dataCopy.get(), buffer->dataPointer(), count);
     }
   }
   return dataCopy;
 }
 
 /* static */
 void ArrayBufferObject::detach(JSContext* cx,
                                Handle<ArrayBufferObject*> buffer) {
@@ -1162,50 +1174,46 @@ ArrayBufferObject* ArrayBufferObject::cr
   if (!CheckArrayBufferTooLarge(cx, nbytes)) {
     return nullptr;
   }
 
   // Try fitting the data inline with the object by repurposing fixed-slot
   // storage.  Add extra fixed slots if necessary to accomplish this, but don't
   // exceed the maximum number of fixed slots!
   size_t nslots = JSCLASS_RESERVED_SLOTS(&class_);
-  uint8_t* data;
+  ArrayBufferContents data;
   if (nbytes <= MaxInlineBytes) {
     int newSlots = HowMany(nbytes, sizeof(Value));
     MOZ_ASSERT(int(nbytes) <= newSlots * int(sizeof(Value)));
 
     nslots += newSlots;
-    data = nullptr;
   } else {
     data = AllocateArrayBufferContents(cx, nbytes);
     if (!data) {
       return nullptr;
     }
   }
 
   MOZ_ASSERT(!(class_.flags & JSCLASS_HAS_PRIVATE));
   gc::AllocKind allocKind = GetArrayBufferGCObjectKind(nslots);
 
   AutoSetNewObjectMetadata metadata(cx);
   Rooted<ArrayBufferObject*> buffer(
       cx, NewObjectWithClassProto<ArrayBufferObject>(cx, proto, allocKind,
                                                      GenericObject));
   if (!buffer) {
-    if (data) {
-      js_free(data);
-    }
     return nullptr;
   }
 
   MOZ_ASSERT(!gc::IsInsideNursery(buffer),
              "ArrayBufferObject has a finalizer that must be called to not "
              "leak in some cases, so it can't be nursery-allocated");
 
   if (data) {
-    buffer->initialize(nbytes, BufferContents::createMalloced(data));
+    buffer->initialize(nbytes, BufferContents::createMalloced(data.release()));
     AddCellMemory(buffer, nbytes, MemoryUse::ArrayBufferContents);
   } else {
     void* inlineData = buffer->initializeToInlineData(nbytes);
     memset(inlineData, 0, nbytes);
   }
 
   return buffer;
 }
@@ -1275,25 +1283,25 @@ ArrayBufferObject* ArrayBufferObject::cr
     }
 
     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);
+      ArrayBufferContents 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);
-      return copiedData;
+      return copiedData.release();
     }
 
     case WASM:
       MOZ_ASSERT_UNREACHABLE(
           "wasm buffers aren't stealable except by a "
           "memory.grow operation that shouldn't call this "
           "function");
       return nullptr;
@@ -1313,23 +1321,23 @@ ArrayBufferObject::extractStructuredClon
   CheckStealPreconditions(buffer, cx);
 
   BufferContents contents = buffer->contents();
 
   switch (contents.kind()) {
     case INLINE_DATA:
     case NO_DATA:
     case USER_OWNED: {
-      uint8_t* copiedData = NewCopiedBufferContents(cx, buffer);
+      ArrayBufferContents copiedData = NewCopiedBufferContents(cx, buffer);
       if (!copiedData) {
         return BufferContents::createFailed();
       }
 
       ArrayBufferObject::detach(cx, buffer);
-      return BufferContents::createMalloced(copiedData);
+      return BufferContents::createMalloced(copiedData.release());
     }
 
     case MALLOCED:
     case MAPPED: {
       MOZ_ASSERT(contents);
 
       RemoveCellMemory(buffer, buffer->associatedBytes(),
                        MemoryUse::ArrayBufferContents);
--- a/js/src/vm/JSContext.h
+++ b/js/src/vm/JSContext.h
@@ -246,39 +246,16 @@ struct JS_PUBLIC_API JSContext : public 
       return nullptr;
     }
     return runtime_->onOutOfMemory(allocFunc, arena, nbytes, reallocPtr, this);
   }
 
   /* Clear the pending exception (if any) due to OOM. */
   void recoverFromOutOfMemory();
 
-  /*
-   * This variation of calloc will call the large-allocation-failure callback
-   * on OOM and retry the allocation.
-   */
-  template <typename T>
-  T* pod_arena_callocCanGC(arena_id_t arena, size_t numElems) {
-    T* p = maybe_pod_arena_calloc<T>(arena, numElems);
-    if (MOZ_LIKELY(!!p)) {
-      return p;
-    }
-    size_t bytes;
-    if (MOZ_UNLIKELY(!js::CalculateAllocSize<T>(numElems, &bytes))) {
-      reportAllocationOverflow();
-      return nullptr;
-    }
-    p = static_cast<T*>(
-        runtime()->onOutOfMemoryCanGC(js::AllocFunction::Calloc, arena, bytes));
-    if (!p) {
-      return nullptr;
-    }
-    return p;
-  }
-
   void reportAllocationOverflow() { js::ReportAllocationOverflow(this); }
 
   void noteTenuredAlloc() { allocsThisZoneSinceMinorGC_++; }
 
   uint32_t* addressOfTenuredAllocCount() {
     return &allocsThisZoneSinceMinorGC_;
   }