Bug 1430438 - Remove ref argument from JS_NewExternalArrayBuffer(). r=jorendorff

We actually don't want to allow aliased array buffers at all, so this
removes the ref argument from JS_NewExternalArrayBuffer() and renames
"unref" to "free".

Adjusts some comments, and also adds tests to make sure buffers are not
aliased when cloning.

js/src/jsapi-tests/testArrayBuffer.cpp

@@ -226,44 +226,15 @@ hasExpectedLength(JSContext* cx, JS::HandleObject obj, uint32_t* len)
 
 END_TEST(testArrayBuffer_externalize)
 
-BEGIN_TEST(testArrayBuffer_refcountedContents)
-{
-    RefCountedData data("One two three four");
-    JS::RootedObject buffer(cx, JS_NewExternalArrayBuffer(cx, data.len(), data.contents(),
-        &RefCountedData::incCallback, &RefCountedData::decCallback, &data));
-    CHECK(buffer);
-    CHECK_EQUAL(data.refcount(), size_t(2));
-
-    uint32_t len;
-    bool isShared;
-    uint8_t* bufferData;
-    js::GetArrayBufferLengthAndData(buffer, &len, &isShared, &bufferData);
-    CHECK_EQUAL(len, data.len());
-    CHECK(bufferData == data.contents());
-    CHECK(strcmp(reinterpret_cast<char*>(bufferData), data.asString()) == 0);
-
-    buffer = nullptr;
-    JS_GC(cx);
-    JS_GC(cx);
-    CHECK_EQUAL(data.refcount(), size_t(1));
-
-    data.decref();
-    CHECK_NULL(data.contents());
-    CHECK_EQUAL(data.refcount(), size_t(0));
-
-    return true;
-}
-END_TEST(testArrayBuffer_refcountedContents)
-
 BEGIN_TEST(testArrayBuffer_customFreeFunc)
 {
-    RefCountedData data("One two three four");
-    // Without passing a ref function, the buffer takes over the one existing
-    // reference to the data.
+    ExternalData data("One two three four");
+
+    // The buffer takes ownership of the data.
     JS::RootedObject buffer(cx, JS_NewExternalArrayBuffer(cx, data.len(), data.contents(),
-        nullptr, &RefCountedData::decCallback, &data));
+        &ExternalData::freeCallback, &data));
     CHECK(buffer);
-    CHECK_EQUAL(data.refcount(), size_t(1));
+    CHECK(!data.wasFreed());
 
     uint32_t len;
     bool isShared;
@@ -276,8 +247,7 @@ BEGIN_TEST(testArrayBuffer_customFreeFunc)
     buffer = nullptr;
     JS_GC(cx);
     JS_GC(cx);
-    CHECK_NULL(data.contents());
-    CHECK_EQUAL(data.refcount(), size_t(0));
+    CHECK(data.wasFreed());
 
     return true;
 }
@@ -285,13 +255,13 @@ END_TEST(testArrayBuffer_customFreeFunc)
 
 BEGIN_TEST(testArrayBuffer_staticContents)
 {
-    RefCountedData data("One two three four");
-    // When passing neither a ref nor unref function, the buffer doesn't own
-    // any reference.
+    ExternalData data("One two three four");
+
+    // When not passing a free function, the buffer doesn't own the data.
     JS::RootedObject buffer(cx, JS_NewExternalArrayBuffer(cx, data.len(), data.contents(),
-        nullptr, nullptr));
+        nullptr));
     CHECK(buffer);
-    CHECK_EQUAL(data.refcount(), size_t(1));
+    CHECK(!data.wasFreed());
 
     uint32_t len;
     bool isShared;
@@ -304,21 +274,20 @@ BEGIN_TEST(testArrayBuffer_staticContents)
     buffer = nullptr;
     JS_GC(cx);
     JS_GC(cx);
-    CHECK_EQUAL(data.refcount(), size_t(1));
+    CHECK(!data.wasFreed());
 
-    data.decref();
+    data.free();
     return true;
 }
 END_TEST(testArrayBuffer_staticContents)
 
 BEGIN_TEST(testArrayBuffer_stealDetachExternal)
 {
-    RefCountedData data("One two three four");
+    ExternalData data("One two three four");
     JS::RootedObject buffer(cx, JS_NewExternalArrayBuffer(cx, data.len(), data.contents(),
-        &RefCountedData::incCallback, &RefCountedData::decCallback, &data));
+        &ExternalData::freeCallback, &data));
     CHECK(buffer);
-    data.decref();
-    CHECK_EQUAL(data.refcount(), size_t(1));
+    CHECK(!data.wasFreed());
 
     void* stolenContents = JS_StealArrayBufferContents(cx, buffer);
     // External buffers are currently not stealable, since stealing only
@@ -329,13 +298,12 @@ BEGIN_TEST(testArrayBuffer_stealDetachExternal)
     // External buffers are currently not stealable, so this should keep the
     // reference to the data and just mark the buffer as detached.
     CHECK(JS_IsDetachedArrayBufferObject(buffer));
-    CHECK_EQUAL(data.refcount(), size_t(1));
+    CHECK(!data.wasFreed());
 
     buffer = nullptr;
     JS_GC(cx);
     JS_GC(cx);
-    CHECK_NULL(data.contents());
-    CHECK_EQUAL(data.refcount(), size_t(0));
+    CHECK(data.wasFreed());
 
     return true;
 }

js/src/jsapi-tests/testStructuredClone.cpp

@@ -84,7 +84,7 @@ END_TEST(testStructuredClone_string)
 
 BEGIN_TEST(testStructuredClone_externalArrayBuffer)
 {
-    RefCountedData data("One two three four");
+    ExternalData data("One two three four");
     JS::RootedObject g1(cx, createGlobal());
     JS::RootedObject g2(cx, createGlobal());
     CHECK(g1);
@@ -96,9 +96,8 @@ BEGIN_TEST(testStructuredClone_externalArrayBuffer)
         JSAutoCompartment ac(cx, g1);
 
         JS::RootedObject obj(cx, JS_NewExternalArrayBuffer(cx, data.len(), data.contents(),
-            &RefCountedData::incCallback, &RefCountedData::decCallback, &data));
-        data.decref();
-        CHECK_EQUAL(data.refcount(), size_t(1));
+            &ExternalData::freeCallback, &data));
+        CHECK(!data.wasFreed());
 
         v1 = JS::ObjectOrNullValue(obj);
         CHECK(v1.isObject());
@@ -120,11 +119,11 @@ BEGIN_TEST(testStructuredClone_externalArrayBuffer)
         js::GetArrayBufferLengthAndData(obj, &len, &isShared, &clonedData);
 
         // The contents of the two array buffers should be equal, but not the
-        // same pointer, and an extra reference should not be taken.
+        // same pointer.
         CHECK_EQUAL(len, data.len());
         CHECK(clonedData != data.contents());
         CHECK(strcmp(reinterpret_cast<char*>(clonedData), data.asString()) == 0);
-        CHECK_EQUAL(data.refcount(), size_t(1));
+        CHECK(!data.wasFreed());
     }
 
     // GC the array buffer before data goes out of scope
@@ -132,12 +131,67 @@ BEGIN_TEST(testStructuredClone_externalArrayBuffer)
     JS_GC(cx);
     JS_GC(cx); // Trigger another to wait for background finalization to end
 
-    CHECK_EQUAL(data.refcount(), size_t(0));
+    CHECK(data.wasFreed());
 
     return true;
 }
 END_TEST(testStructuredClone_externalArrayBuffer)
 
+BEGIN_TEST(testStructuredClone_externalArrayBufferDifferentThreadOrProcess)
+{
+    // SameProcessSameThread is tested above.
+    CHECK(testStructuredCloneCopy(JS::StructuredCloneScope::SameProcessDifferentThread));
+    CHECK(testStructuredCloneCopy(JS::StructuredCloneScope::DifferentProcess));
+    return true;
+}
+
+bool testStructuredCloneCopy(JS::StructuredCloneScope scope)
+{
+    ExternalData data("One two three four");
+    JS::RootedObject buffer(cx, JS_NewExternalArrayBuffer(cx, data.len(), data.contents(),
+        &ExternalData::freeCallback, &data));
+    CHECK(buffer);
+    CHECK(!data.wasFreed());
+
+    JS::RootedValue v1(cx, JS::ObjectValue(*buffer));
+    JS::RootedValue v2(cx);
+    CHECK(clone(scope, v1, &v2));
+    JS::RootedObject bufferOut(cx, v2.toObjectOrNull());
+    CHECK(bufferOut);
+    CHECK(JS_IsArrayBufferObject(bufferOut));
+
+    uint32_t len;
+    bool isShared;
+    uint8_t* clonedData;
+    js::GetArrayBufferLengthAndData(bufferOut, &len, &isShared, &clonedData);
+
+    // Cloning should copy the data, so the contents of the two array buffers
+    // should be equal, but not the same pointer.
+    CHECK_EQUAL(len, data.len());
+    CHECK(clonedData != data.contents());
+    CHECK(strcmp(reinterpret_cast<char*>(clonedData), data.asString()) == 0);
+    CHECK(!data.wasFreed());
+
+    buffer = nullptr;
+    bufferOut = nullptr;
+    v1.setNull();
+    v2.setNull();
+    JS_GC(cx);
+    JS_GC(cx);
+    CHECK(data.wasFreed());
+
+    return true;
+}
+
+bool clone(JS::StructuredCloneScope scope, JS::HandleValue v1, JS::MutableHandleValue v2)
+{
+    JSAutoStructuredCloneBuffer clonedBuffer(scope, nullptr, nullptr);
+    CHECK(clonedBuffer.write(cx, v1));
+    CHECK(clonedBuffer.read(cx, v2));
+    return true;
+}
+END_TEST(testStructuredClone_externalArrayBufferDifferentThreadOrProcess)
+
 struct StructuredCloneTestPrincipals final : public JSPrincipals {
     uint32_t rank;
 

js/src/jsapi-tests/tests.h

@@ -441,40 +441,30 @@ class TestJSPrincipals : public JSPrincipals
     }
 };
 
-// A class that simulates refcounted data, for testing with array buffers.
-class RefCountedData {
+// A class that simulates externally memory-managed data, for testing with
+// array buffers.
+class ExternalData {
     char* contents_;
     size_t len_;
-    size_t refcount_;
 
   public:
-    explicit RefCountedData(const char* str) : contents_(strdup(str)),
-        len_(strlen(str) + 1), refcount_(1) { }
+    explicit ExternalData(const char* str) : contents_(strdup(str)), len_(strlen(str) + 1) { }
 
     size_t len() const { return len_; }
     void* contents() const { return contents_; }
     char* asString() const { return contents_; }
-    size_t refcount() const { return refcount_; }
+    bool wasFreed() const { return !contents_; }
 
-    void incref() { refcount_++; }
-    void decref() {
-        refcount_--;
-        if (refcount_ == 0) {
-            free(contents_);
-            contents_ = nullptr;
-        }
+    void free() {
+        MOZ_ASSERT(!wasFreed());
+        ::free(contents_);
+        contents_ = nullptr;
     }
 
-    static void incCallback(void* contents, void* userData) {
-        auto self = static_cast<RefCountedData*>(userData);
+    static void freeCallback(void* contents, void* userData) {
+        auto self = static_cast<ExternalData*>(userData);
         MOZ_ASSERT(self->contents() == contents);
-        self->incref();
-    }
-
-    static void decCallback(void* contents, void* userData) {
-        auto self = static_cast<RefCountedData*>(userData);
-        MOZ_ASSERT(self->contents() == contents);
-        self->decref();
+        self->free();
     }
 };
 

js/src/jsapi.h

@@ -3225,39 +3225,39 @@ JS_NewArrayBufferWithContents(JSContext* cx, size_t nbytes, void* contents);
 
 namespace JS {
 
-using BufferContentsRefFunc = void (*)(void* contents, void* userData);
+using BufferContentsFreeFunc = void (*)(void* contents, void* userData);
 
 }  /* namespace JS */
 
 /**
- * Create a new array buffer with the given contents. The ref and unref
- * functions should increment or decrement the reference count of the contents.
- * These functions allow array buffers to be used with embedder objects that
- * use reference counting, for example. The contents must not be modified by
- * any reference holders, internal or external.
+ * Create a new array buffer with the given contents. The contents must not be
+ * modified by any other code, internal or external.
  *
- * On success, the new array buffer takes a reference, and |ref(contents,
- * refUserData)| will be called. When the array buffer is ready to be disposed
- * of, |unref(contents, refUserData)| will be called to release the array
- * buffer's reference on the contents.
+ * When the array buffer is ready to be disposed of, `freeFunc(contents,
+ * freeUserData)` will be called to release the array buffer's reference on the
+ * contents.
  *
- * The ref and unref functions must not call any JSAPI functions that could
- * cause a garbage collection.
+ * `freeFunc()` must not call any JSAPI functions that could cause a garbage
+ * collection.
  *
- * The ref function is optional. If it is nullptr, the caller is responsible
+ * The caller must keep the buffer alive until `freeFunc()` is called, or, if
+ * `freeFunc` is null, until the JSRuntime is destroyed.
+ *
+ * The caller must not access the buffer on other threads. The JS engine will
+ * not allow the buffer to be transferred to other threads. If you try to
+ * transfer an external ArrayBuffer to another thread, the data is copied to a
+ * new malloc buffer. `freeFunc()` must be threadsafe, and may be called from
+ * any thread.
+ *
+ * This allows array buffers to be used with embedder objects that use reference
+ * counting, for example. In that case the caller is responsible
  * for incrementing the reference count before passing the contents to this
  * function. This also allows using non-reference-counted contents that must be
  * freed with some function other than free().
- *
- * The ref function may also be called in case the buffer is cloned in some
- * way. Currently this is not used, but it may be in the future. If the ref
- * function is nullptr, any operation where an extra reference would otherwise
- * be taken, will either copy the data, or throw an exception.
  */
 extern JS_PUBLIC_API(JSObject*)
 JS_NewExternalArrayBuffer(JSContext* cx, size_t nbytes, void* contents,
-                          JS::BufferContentsRefFunc ref, JS::BufferContentsRefFunc unref,
-                          void* refUserData = nullptr);
+                          JS::BufferContentsFreeFunc freeFunc, void* freeUserData = nullptr);
 
 /**
  * Create a new array buffer with the given contents.  The array buffer does not take ownership of

js/src/vm/ArrayBufferObject.cpp

@@ -984,11 +984,11 @@ ArrayBufferObject::dataPointerShared() const
     return SharedMem<uint8_t*>::unshared(getFixedSlot(DATA_SLOT).toPrivate());
 }
 
-ArrayBufferObject::RefcountInfo*
-ArrayBufferObject::refcountInfo() const
+ArrayBufferObject::FreeInfo*
+ArrayBufferObject::freeInfo() const
 {
     MOZ_ASSERT(isExternal());
-    return reinterpret_cast<RefcountInfo*>(inlineDataPointer());
+    return reinterpret_cast<FreeInfo*>(inlineDataPointer());
 }
 
 void
@@ -1007,13 +1007,13 @@ ArrayBufferObject::releaseData(FreeOp* fop)
         WasmArrayRawBuffer::Release(dataPointer());
         break;
       case EXTERNAL:
-        if (refcountInfo()->unref) {
+        if (freeInfo()->freeFunc) {
             // The analyzer can't know for sure whether the embedder-supplied
-            // unref function will GC. We give the analyzer a hint here.
-            // (Doing a GC in the unref function is considered a programmer
+            // free function will GC. We give the analyzer a hint here.
+            // (Doing a GC in the free function is considered a programmer
             // error.)
             JS::AutoSuppressGCAnalysis nogc;
-            refcountInfo()->unref(dataPointer(), refcountInfo()->refUserData);
+            freeInfo()->freeFunc(dataPointer(), freeInfo()->freeUserData);
         }
         break;
     }
@@ -1027,15 +1027,9 @@ ArrayBufferObject::setDataPointer(BufferContents contents, OwnsState ownsData)
     setFlags((flags() & ~KIND_MASK) | contents.kind());
 
     if (isExternal()) {
-        auto info = refcountInfo();
-        info->ref = contents.refFunc();
-        info->unref = contents.unrefFunc();
-        info->refUserData = contents.refUserData();
-        if (info->ref) {
-            // See comment in releaseData() for the explanation for this.
-            JS::AutoSuppressGCAnalysis nogc;
-            info->ref(dataPointer(), info->refUserData);
-        }
+        auto info = freeInfo();
+        info->freeFunc = contents.freeFunc();
+        info->freeUserData = contents.freeUserData();
     }
 }
 
@@ -1206,12 +1200,12 @@ ArrayBufferObject::create(JSContext* cx, uint32_t nbytes, BufferContents content
     if (contents) {
         if (ownsState == OwnsData) {
             if (contents.kind() == EXTERNAL) {
-                // Store the RefcountInfo in the inline data slots so that we
+                // Store the FreeInfo in the inline data slots so that we
                 // don't use up slots for it in non-refcounted array buffers.
-                size_t refcountInfoSlots = JS_HOWMANY(sizeof(RefcountInfo), sizeof(Value));
-                MOZ_ASSERT(reservedSlots + refcountInfoSlots <= NativeObject::MAX_FIXED_SLOTS,
-                           "RefcountInfo must fit in inline slots");
-                nslots += refcountInfoSlots;
+                size_t freeInfoSlots = JS_HOWMANY(sizeof(FreeInfo), sizeof(Value));
+                MOZ_ASSERT(reservedSlots + freeInfoSlots <= NativeObject::MAX_FIXED_SLOTS,
+                           "FreeInfo must fit in inline slots");
+                nslots += freeInfoSlots;
             } else {
                 // The ABO is taking ownership, so account the bytes against
                 // the zone.
@@ -1870,8 +1864,7 @@ JS_NewArrayBufferWithContents(JSContext* cx, size_t nbytes, void* data)
 
 JS_PUBLIC_API(JSObject*)
 JS_NewExternalArrayBuffer(JSContext* cx, size_t nbytes, void* data,
-                          JS::BufferContentsRefFunc ref, JS::BufferContentsRefFunc unref,
-                          void* refUserData)
+                          JS::BufferContentsFreeFunc freeFunc, void* freeUserData)
 {
     AssertHeapIsIdle();
     CHECK_REQUEST(cx);
@@ -1880,7 +1873,7 @@ JS_NewExternalArrayBuffer(JSContext* cx, size_t nbytes, void* data,
     MOZ_ASSERT(nbytes > 0);
 
     ArrayBufferObject::BufferContents contents =
-        ArrayBufferObject::BufferContents::createExternal(data, ref, unref, refUserData);
+        ArrayBufferObject::BufferContents::createExternal(data, freeFunc, freeUserData);
     return ArrayBufferObject::create(cx, nbytes, contents, ArrayBufferObject::OwnsData,
                                      /* proto = */ nullptr, TenuredObject);
 }

js/src/vm/ArrayBufferObject.h

@@ -193,8 +193,8 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared
 
         // The dataPointer() is owned by this buffer and should be released
         // when no longer in use. Releasing the pointer may be done by freeing,
-        // invoking a dereference callback function, or unmapping, as
-        // determined by the buffer's other flags.
+        // invoking a free callback function, or unmapping, as determined by the
+        // buffer's other flags.
         //
         // Array buffers which do not own their data include buffers that
         // allocate their data inline, and buffers that are created lazily for
@@ -224,22 +224,21 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared
     class BufferContents {
         uint8_t* data_;
         BufferKind kind_;
-        JS::BufferContentsRefFunc ref_;
-        JS::BufferContentsRefFunc unref_;
-        void* refUserData_;
+        JS::BufferContentsFreeFunc free_;
+        void* freeUserData_;
 
         friend class ArrayBufferObject;
 
-        BufferContents(uint8_t* data, BufferKind kind, JS::BufferContentsRefFunc ref = nullptr,
-                       JS::BufferContentsRefFunc unref = nullptr, void* refUserData = nullptr)
-        : data_(data), kind_(kind), ref_(ref), unref_(unref), refUserData_(refUserData)
+        BufferContents(uint8_t* data, BufferKind kind,
+                       JS::BufferContentsFreeFunc freeFunc = nullptr,
+                       void* freeUserData = nullptr)
+        : data_(data), kind_(kind), free_(freeFunc), freeUserData_(freeUserData)
         {
             MOZ_ASSERT((kind_ & ~KIND_MASK) == 0);
-            MOZ_ASSERT_IF(ref_ || unref_ || refUserData_, kind_ == EXTERNAL);
+            MOZ_ASSERT_IF(free_ || freeUserData_, kind_ == EXTERNAL);
 
-            // BufferContents does not ref or unref the data since it is
-            // internal and short-lived. It is the caller's responsibility to
-            // ensure that the BufferContents does not outlive the data.
+            // It is the caller's responsibility to ensure that the
+            // BufferContents does not outlive the data.
         }
 
       public:
@@ -255,18 +254,16 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared
             return BufferContents(static_cast<uint8_t*>(data), PLAIN);
         }
 
-        static BufferContents createExternal(void *data, JS::BufferContentsRefFunc ref,
-                                             JS::BufferContentsRefFunc unref,
-                                             void* refUserData = nullptr)
+        static BufferContents createExternal(void* data, JS::BufferContentsFreeFunc freeFunc,
+                                             void* freeUserData = nullptr)
         {
-            return BufferContents(static_cast<uint8_t*>(data), EXTERNAL, ref, unref, refUserData);
+            return BufferContents(static_cast<uint8_t*>(data), EXTERNAL, freeFunc, freeUserData);
         }
 
         uint8_t* data() const { return data_; }
         BufferKind kind() const { return kind_; }
-        JS::BufferContentsRefFunc refFunc() const { return ref_; }
-        JS::BufferContentsRefFunc unrefFunc() const { return unref_; }
-        void* refUserData() const { return refUserData_; }
+        JS::BufferContentsFreeFunc freeFunc() const { return free_; }
+        void* freeUserData() const { return freeUserData_; }
 
         explicit operator bool() const { return data_ != nullptr; }
         WasmArrayRawBuffer* wasmBuffer() const;
@@ -349,12 +346,11 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared
 
     uint8_t* inlineDataPointer() const;
 
-    struct RefcountInfo {
-        JS::BufferContentsRefFunc ref;
-        JS::BufferContentsRefFunc unref;
-        void* refUserData;
+    struct FreeInfo {
+        JS::BufferContentsFreeFunc freeFunc;
+        void* freeUserData;
     };
-    RefcountInfo* refcountInfo() const;
+    FreeInfo* freeInfo() const;
 
   public:
     uint8_t* dataPointer() const;
@@ -363,8 +359,8 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared
 
     BufferContents contents() const {
         if (isExternal()) {
-            return BufferContents(dataPointer(), EXTERNAL, refcountInfo()->ref,
-                                  refcountInfo()->unref, refcountInfo()->refUserData);
+            return BufferContents(dataPointer(), EXTERNAL, freeInfo()->freeFunc,
+                                  freeInfo()->freeUserData);
         }
         return BufferContents(dataPointer(), bufferKind());
     }