author | Jeff Walden <jwalden@mit.edu> |
Mon, 18 Feb 2019 23:13:48 -0800 | |
changeset 461602 | d8a4ce77f2f10639b3a5783b5961319d35342711 |
parent 461601 | ac3ad3942d6791b7df823d87b23f68efdfa7317a |
child 461603 | f487c864d2abb77498667161baf187b3ecb7f995 |
push id | 35626 |
push user | csabou@mozilla.com |
push date | Thu, 28 Feb 2019 11:31:08 +0000 |
treeherder | mozilla-central@2ea0c1db7e60 [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | sfink |
bugs | 1529298 |
milestone | 67.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
|
--- a/js/src/jsapi-tests/testArrayBufferWithUserOwnedContents.cpp +++ b/js/src/jsapi-tests/testArrayBufferWithUserOwnedContents.cpp @@ -1,31 +1,32 @@ /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- * vim: set ts=8 sts=2 et sw=2 tw=80: */ #include "jsfriendapi.h" #include "jsapi-tests/tests.h" #include "vm/ArrayBufferObject.h" -char test_data[] = +char testData[] = "1234567890ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; +constexpr size_t testDataLength = sizeof(testData); + static void GC(JSContext* cx) { JS_GC(cx); // Trigger another to wait for background finalization to end. JS_GC(cx); } BEGIN_TEST(testArrayBufferWithUserOwnedContents) { - size_t length = sizeof(test_data); JS::RootedObject obj( - cx, JS_NewArrayBufferWithUserOwnedContents(cx, length, test_data)); + cx, JS_NewArrayBufferWithUserOwnedContents(cx, testDataLength, testData)); GC(cx); - CHECK(VerifyObject(obj, length)); + CHECK(VerifyObject(obj, testDataLength)); GC(cx); JS_DetachArrayBuffer(cx, obj); GC(cx); CHECK(VerifyObject(obj, 0)); return true; } @@ -33,15 +34,17 @@ bool VerifyObject(JS::HandleObject obj, JS::AutoCheckCannotGC nogc; CHECK(obj); CHECK(JS_IsArrayBufferObject(obj)); CHECK_EQUAL(JS_GetArrayBufferByteLength(obj), length); bool sharedDummy; const char* data = reinterpret_cast<const char*>( JS_GetArrayBufferData(obj, &sharedDummy, nogc)); - CHECK(data); - CHECK(test_data == data); + if (length == testDataLength) { + CHECK(data); + CHECK(testData == data); + } return true; } END_TEST(testArrayBufferWithUserOwnedContents)
--- a/js/src/vm/ArrayBufferObject.cpp +++ b/js/src/vm/ArrayBufferObject.cpp @@ -514,16 +514,22 @@ static void NoteViewBufferWasDetached( buffer->setByteLength(0); buffer->setIsDetached(); } void ArrayBufferObject::setNewData(FreeOp* fop, BufferContents newContents, OwnsState ownsState) { if (ownsData()) { + // XXX As data kinds gradually transition to *all* being owned, this + // assertion could become troublesome. But right *now*, it isn't -- + // the call just above in ABO::detach occurs only if this assertion is + // true, and the call in ABO::changeContents happens only if + // ABO::prepareForAsmJS calls ABO::changeContents, but *that* call is + // guarded by an owns-data check. So no need to touch this right now. MOZ_ASSERT(newContents.data() != dataPointer()); releaseData(fop); } setDataPointer(newContents, ownsState); } // This is called *only* from changeContents(), below. @@ -959,16 +965,19 @@ bool js::CreateWasmBuffer(JSContext* cx, MOZ_ASSERT(buffer->isMalloced() || buffer->isMapped() || buffer->isExternal()); // Buffers already prepared for asm.js need no further work. if (buffer->isPreparedForAsmJS()) { return true; } + // XXX User-owned, no-data, and is-inline were excluded above, so + // |ownsData()| having changed semantics in this patch doesn't matter + // here. if (!buffer->ownsData()) { uint8_t* data = AllocateArrayBufferContents(cx, buffer->byteLength()); if (!data) { return false; } memcpy(data, buffer->dataPointer(), buffer->byteLength()); buffer->changeContents(cx, BufferContents::createMalloced(data), OwnsData); } @@ -1001,26 +1010,27 @@ ArrayBufferObject::FreeInfo* ArrayBuffer return reinterpret_cast<FreeInfo*>(inlineDataPointer()); } void ArrayBufferObject::releaseData(FreeOp* fop) { MOZ_ASSERT(ownsData()); switch (bufferKind()) { case INLINE_DATA: - MOZ_ASSERT_UNREACHABLE("inline data should never be owned"); + // Inline data doesn't require releasing. break; case MALLOCED: fop->free_(dataPointer()); break; case NO_DATA: + // There's nothing to release if there's no data. MOZ_ASSERT(dataPointer() == nullptr); break; case USER_OWNED: - MOZ_ASSERT_UNREACHABLE("user-owned data should never be owned by this"); + // User-owned data is released by, well, the user. break; case MAPPED: gc::DeallocateMappedContent(dataPointer(), byteLength()); break; case WASM: WasmArrayRawBuffer::Release(dataPointer()); break; case EXTERNAL: @@ -1216,17 +1226,19 @@ ArrayBufferObject* ArrayBufferObject::cr // Some |contents| kinds need to store extra data in the ArrayBuffer beyond a // data pointer. If needed for the particular kind, add extra fixed slots to // the ArrayBuffer for use as raw storage to store such information. size_t reservedSlots = JSCLASS_RESERVED_SLOTS(&class_); size_t nslots = reservedSlots; if (ownsState == OwnsData) { - if (contents.kind() == EXTERNAL) { + if (contents.kind() == USER_OWNED) { + // No accounting to do in this case. + } else if (contents.kind() == EXTERNAL) { // 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 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. @@ -1357,17 +1369,17 @@ ArrayBufferObject* ArrayBufferObject::cr cx->check(buffer); MOZ_ASSERT(!buffer->isPreparedForAsmJS(), "asm.js-prepared buffers don't have detachable/stealable data"); #ifdef DEBUG if (hasStealableContents) { MOZ_ASSERT(!buffer->isInlineData(), - "inline data is DoesntOwnData and isn't malloc-allocated"); + "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"); MOZ_ASSERT(!buffer->hasUserOwnedData(), "user-owned data isn't stealable or necessarily malloc'd"); // wasm buffers can't be stolen by |JS_StealArrayBufferContents|, but // but *this* function is used internally by the impl of memory growth, so @@ -1409,35 +1421,34 @@ ArrayBufferObject* ArrayBufferObject::cr ArrayBufferObject& buffer = AsArrayBuffer(obj); if (!buffer.ownsData()) { return; } switch (buffer.bufferKind()) { case INLINE_DATA: - MOZ_ASSERT_UNREACHABLE("inline data should never be owned and should be " - "accounted for in |this|'s memory"); + // Inline data's size should be reported by this object's size-class + // reporting. break; case MALLOCED: if (buffer.isPreparedForAsmJS()) { info->objectsMallocHeapElementsAsmJS += mallocSizeOf(buffer.dataPointer()); } else { info->objectsMallocHeapElementsNormal += mallocSizeOf(buffer.dataPointer()); } break; case NO_DATA: + // No data is no memory. MOZ_ASSERT(buffer.dataPointer() == nullptr); break; case USER_OWNED: - MOZ_ASSERT_UNREACHABLE( - "user-owned data should never be owned by this, and such memory " - "should be accounted for by the code that provided it"); + // User-owned data should be accounted for by the user. break; case MAPPED: info->objectsNonHeapElementsNormal += buffer.byteLength(); break; case WASM: info->objectsNonHeapElementsWasm += buffer.byteLength(); MOZ_ASSERT(buffer.wasmMappedSize() >= buffer.byteLength()); info->wasmGuardPages += buffer.wasmMappedSize() - buffer.byteLength(); @@ -1677,16 +1688,47 @@ JS_FRIEND_API bool JS_DetachArrayBuffer( if (buffer->isWasm() || buffer->isPreparedForAsmJS()) { JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_WASM_NO_TRANSFER); return false; } using BufferContents = ArrayBufferObject::BufferContents; + // This patch makes NO_DATA, INLINE_DATA, and USER_OWNED all be owned. I + // argue this change should be safe. First, the easy NO_DATA case: + // + // * NO_DATA in ABO::detach will see that no data pointer change occurs, so + // it won't call ABO::setNewData and will leave this exactly as it was + // (save for view-notification and byte length and detachment changing). + // This is trivially fine. + // + // Making INLINE_DATA and USER_OWNED be owned, if we keep doing the same + // steps, *will* change stuff. These pre-states + // + // (user-owned pointer, OwnsData, USER_OWNED | remaining) + // (inline data pointer, OwnsData, INLINE_DATA | remaining) + // + // will change to + // + // (nullptr, OwnsData, NO_DATA | remaining) + // + // This changes the behavior of JS_GetArrayBufferData, but the calling code + // already has to be cognizant of byte length if it might index into that + // pointer, so all that changes is the pointer's plain identity, and it seems + // okay to say "don't do that". Ownership state is unchanged. And the + // kind-change shouldn't matter because we only test + // |buffer->hasUserOwnedData()| in ABO::prepareForAsmJS -- but only *after* + // anything length-zero would have been filtered out, so no change there. + // Nothing tests for |isInlineData()| except assertions. + // + // With |ownsData()| becoming increasingly true for everything (at present + // there's only one way to have |!ownsData()|, soon this can just be + // |BufferContents::createNoData()| in all cases. (And, of course, this + // comment and a few others like it can then die. XXX) BufferContents newContents = buffer->ownsData() ? BufferContents::createNoData() : buffer->contents(); ArrayBufferObject::detach(cx, buffer, newContents); return true; @@ -1751,17 +1793,17 @@ JS_PUBLIC_API JSObject* JS_NewArrayBuffe CHECK_THREAD(cx); MOZ_ASSERT(data); using BufferContents = ArrayBufferObject::BufferContents; BufferContents contents = BufferContents::createUserOwned(data); return ArrayBufferObject::createForContents(cx, nbytes, contents, - ArrayBufferObject::DoesntOwnData); + ArrayBufferObject::OwnsData); } JS_FRIEND_API bool JS_IsArrayBufferObject(JSObject* obj) { return obj->canUnwrapAs<ArrayBufferObject>(); } JS_FRIEND_API bool JS_ArrayBufferHasData(JSObject* obj) { return !obj->unwrapAs<ArrayBufferObject>().isDetached(); @@ -1806,16 +1848,19 @@ JS_PUBLIC_API void* JS_StealArrayBufferC 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(); } JS_PUBLIC_API JSObject* JS_NewMappedArrayBufferWithContents(JSContext* cx,
--- a/js/src/vm/ArrayBufferObject.h +++ b/js/src/vm/ArrayBufferObject.h @@ -475,18 +475,17 @@ class ArrayBufferObject : public ArrayBu setByteLength(byteLength); setFlags(0); setFirstView(nullptr); setDataPointer(contents, ownsState); } void* initializeToInlineData(size_t byteLength) { void* data = inlineDataPointer(); - initialize(byteLength, BufferContents::createInlineData(data), - DoesntOwnData); + initialize(byteLength, BufferContents::createInlineData(data), OwnsData); return data; } }; typedef Rooted<ArrayBufferObject*> RootedArrayBufferObject; typedef Handle<ArrayBufferObject*> HandleArrayBufferObject; typedef MutableHandle<ArrayBufferObject*> MutableHandleArrayBufferObject;
--- a/js/src/vm/StructuredClone.cpp +++ b/js/src/vm/StructuredClone.cpp @@ -1879,18 +1879,23 @@ bool JSStructuredCloneWriter::transferOw // // XXX This extra caution *may* not actually be necessary: no-data // ArrayBuffers exist only as the result of detaching ArrayBuffers // (but these are excluded by the test above) or as the result of a // wasm memory.grow operation (but contents are then immediately // replaced with wasm contents). But it seems better to play it // safe and deal with *all* states, not just the ones that appear // possible here. + // + // Inline data obviously has to be copied. So too user-owned data bool hasStealableContents = - arrayBuffer->ownsData() && !arrayBuffer->isNoData(); + arrayBuffer->ownsData() && + !arrayBuffer->isInlineData() && + !arrayBuffer->isNoData() && + !arrayBuffer->hasUserOwnedData(); ArrayBufferObject::BufferContents bufContents = ArrayBufferObject::stealContents(cx, arrayBuffer, hasStealableContents); if (!bufContents) { return false; // out of memory }