Bug 1633598 - Make AllocateArrayBufferContents return a UniquePtr to handle manually freeing memory in case of early error. r=sfink
☠☠ backed out by 5ffbc6026452 ☠ ☠
authorJeff Walden <jwalden@mit.edu>
Thu, 07 May 2020 19:29:17 +0000
changeset 528669 7fe73b300a5ef54d63c05c142ddb7e04f44269fb
parent 528668 5e54b29e93d4ff891bcb3d7461c57ddef351e437
child 528670 a662b2c07b3a6c607e71a470d080774fcfbd5fad
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_;
   }