diff --git a/js/src/jsapi-tests/testArrayBuffer.cpp b/js/src/jsapi-tests/testArrayBuffer.cpp index 538cde665939..5963db525527 100644 --- a/js/src/jsapi-tests/testArrayBuffer.cpp +++ b/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(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; } diff --git a/js/src/jsapi-tests/testStructuredClone.cpp b/js/src/jsapi-tests/testStructuredClone.cpp index 8433712b5bb1..69e241d39107 100644 --- a/js/src/jsapi-tests/testStructuredClone.cpp +++ b/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(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(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; diff --git a/js/src/jsapi-tests/tests.h b/js/src/jsapi-tests/tests.h index f1db7f4ce176..d513b02014c8 100644 --- a/js/src/jsapi-tests/tests.h +++ b/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(userData); + static void freeCallback(void* contents, void* userData) { + auto self = static_cast(userData); MOZ_ASSERT(self->contents() == contents); - self->incref(); - } - - static void decCallback(void* contents, void* userData) { - auto self = static_cast(userData); - MOZ_ASSERT(self->contents() == contents); - self->decref(); + self->free(); } }; diff --git a/js/src/jsapi.h b/js/src/jsapi.h index f6ada0a9ebce..ebd6dad2fe3f 100644 --- a/js/src/jsapi.h +++ b/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 diff --git a/js/src/vm/ArrayBufferObject.cpp b/js/src/vm/ArrayBufferObject.cpp index 597302e45ba1..430a38163fd0 100644 --- a/js/src/vm/ArrayBufferObject.cpp +++ b/js/src/vm/ArrayBufferObject.cpp @@ -984,11 +984,11 @@ ArrayBufferObject::dataPointerShared() const return SharedMem::unshared(getFixedSlot(DATA_SLOT).toPrivate()); } -ArrayBufferObject::RefcountInfo* -ArrayBufferObject::refcountInfo() const +ArrayBufferObject::FreeInfo* +ArrayBufferObject::freeInfo() const { MOZ_ASSERT(isExternal()); - return reinterpret_cast(inlineDataPointer()); + return reinterpret_cast(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); } diff --git a/js/src/vm/ArrayBufferObject.h b/js/src/vm/ArrayBufferObject.h index a6bbd7a29bd1..638ff2679451 100644 --- a/js/src/vm/ArrayBufferObject.h +++ b/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(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(data), EXTERNAL, ref, unref, refUserData); + return BufferContents(static_cast(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()); }