Bug 1240984 - Remove dummy ArrayBufferContents backstop, r=Waldo
☠☠ backed out by 56faf83bf0c3 ☠ ☠
authorSteve Fink <sfink@mozilla.com>
Wed, 09 Mar 2016 14:39:35 -0800
changeset 331031 f2cf760a466829efa2b6182fa2243211f73429eb
parent 331030 6bb7f6c316bb8c0c5a898439250288a7bd306827
child 331032 8ccb505a1cfcfb688ec6f038db50211b43640cfe
push id9858
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 14:37:10 +0000
treeherdermozilla-aurora@203106ef6cb6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersWaldo
bugs1240984
milestone50.0a1
Bug 1240984 - Remove dummy ArrayBufferContents backstop, r=Waldo MozReview-Commit-ID: h4oF04rDZn
js/src/jit-test/tests/asm.js/testNeuter.js
js/src/jsfriendapi.h
js/src/vm/ArrayBufferObject.cpp
js/src/vm/ArrayBufferObject.h
js/src/vm/StructuredClone.cpp
--- a/js/src/jit-test/tests/asm.js/testNeuter.js
+++ b/js/src/jit-test/tests/asm.js/testNeuter.js
@@ -18,32 +18,34 @@ var m = asmCompile('stdlib', 'foreign', 
                        return i32[i>>2]|0
                    }
                    return {get:get, set:set}`);
 
 var buffer = new ArrayBuffer(BUF_MIN);
 var {get, set} = asmLink(m, this, null, buffer);
 set(4, 42);
 assertEq(get(4), 42);
-assertThrowsInstanceOf(() => detachArrayBuffer(buffer, "change-data"),
-                       InternalError);
-assertThrowsInstanceOf(() => detachArrayBuffer(buffer, "same-data"),
-                       InternalError);
+assertThrowsInstanceOf(() => detachArrayBuffer(buffer), Error);
 
 var m = asmCompile('stdlib', 'foreign', 'buffer',
                   `"use asm";
                    var i32 = new stdlib.Int32Array(buffer);
                    var ffi = foreign.ffi;
                    function inner(i) {
                        i=i|0;
                        ffi();
                        return i32[i>>2]|0
                    }
                    return inner`);
 
 var buffer = new ArrayBuffer(BUF_MIN);
 function ffi1()
 {
-    assertThrowsInstanceOf(() => detachArrayBuffer(buffer, "change-data"),
-                           InternalError);
+    assertThrowsInstanceOf(() => detachArrayBuffer(buffer), Error);
 }
 
 var inner = asmLink(m, this, {ffi: ffi1}, buffer);
+
+function ffi2()
+{
+    assertThrowsInstanceOf(() => serialize([], [buffer]), Error);
+}
+var inner2 = asmLink(m, this, {ffi: ffi2}, buffer);
--- a/js/src/jsfriendapi.h
+++ b/js/src/jsfriendapi.h
@@ -2102,22 +2102,17 @@ enum DetachDataDisposition {
     ChangeData,
     KeepData
 };
 
 /**
  * Detach an ArrayBuffer, causing all associated views to no longer refer to
  * the ArrayBuffer's original attached memory.
  *
- * The |changeData| argument is a hint to inform internal behavior with respect
- * to the ArrayBuffer's internal pointer to associated data.  |ChangeData|
- * attempts to set the internal pointer to fresh memory of the same size as the
- * original memory; |KeepData| attempts to preserve the original pointer, even
- * while the ArrayBuffer appears observably detached.  There is no guarantee
- * this parameter is respected -- it's only a hint.
+ * The |changeData| argument is obsolete and ignored.
  */
 extern JS_FRIEND_API(bool)
 JS_DetachArrayBuffer(JSContext* cx, JS::HandleObject obj,
                      DetachDataDisposition changeData);
 
 /**
  * Check whether the obj is a detached ArrayBufferObject. Note that this may
  * return false if a security wrapper is encountered that denies the
--- a/js/src/vm/ArrayBufferObject.cpp
+++ b/js/src/vm/ArrayBufferObject.cpp
@@ -255,24 +255,22 @@ NoteViewBufferWasDetached(ArrayBufferVie
                           JSContext* cx)
 {
     view->notifyBufferDetached(newContents.data());
 
     // Notify compiled jit code that the base pointer has moved.
     MarkObjectStateChange(cx, view);
 }
 
-/* static */ bool
+/* static */ void
 ArrayBufferObject::detach(JSContext* cx, Handle<ArrayBufferObject*> buffer,
                           BufferContents newContents)
 {
-    if (buffer->isWasm()) {
-        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_OUT_OF_MEMORY);
-        return false;
-    }
+    MOZ_ASSERT(!buffer->isWasm(), "WASM/asm.js ArrayBuffers cannot be detached");
+    MOZ_ASSERT(buffer->hasDetachableContents(), "detaching the non-detachable");
 
     // When detaching buffers where we don't know all views, the new data must
     // match the old data. All missing views are typed objects, which do not
     // expect their data to ever change.
     MOZ_ASSERT_IF(buffer->forInlineTypedObject(),
                   newContents.data() == buffer->dataPointer());
 
     // When detaching a buffer with typed object views, any jitcode accessing
@@ -309,17 +307,16 @@ ArrayBufferObject::detach(JSContext* cx,
         }
     }
 
     if (newContents.data() != buffer->dataPointer())
         buffer->setNewOwnedData(cx->runtime()->defaultFreeOp(), newContents);
 
     buffer->setByteLength(0);
     buffer->setIsDetached();
-    return true;
 }
 
 void
 ArrayBufferObject::setNewOwnedData(FreeOp* fop, BufferContents newContents)
 {
     if (ownsData()) {
         MOZ_ASSERT(newContents.data() != dataPointer());
         releaseData(fop);
@@ -724,45 +721,49 @@ bool
 ArrayBufferObject::createDataViewForThis(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
     return CallNonGenericMethod<IsArrayBuffer, createDataViewForThisImpl>(cx, args);
 }
 
 /* static */ ArrayBufferObject::BufferContents
 ArrayBufferObject::stealContents(JSContext* cx, Handle<ArrayBufferObject*> buffer,
-                                 bool hasStealableContents)
+                                 bool forceCopy)
 {
-    MOZ_ASSERT_IF(hasStealableContents, buffer->hasStealableContents());
+    if (buffer->isDetached()) {
+        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_TYPED_ARRAY_DETACHED);
+        return BufferContents::createPlain(nullptr);
+    }
+    if (!buffer->hasDetachableContents()) {
+        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_TYPED_ARRAY_BAD_ARGS);
+        return BufferContents::createPlain(nullptr);
+    }
 
     BufferContents oldContents(buffer->dataPointer(), buffer->bufferKind());
-    BufferContents newContents = AllocateArrayBufferContents(cx, buffer->byteLength());
-    if (!newContents)
-        return BufferContents::createPlain(nullptr);
 
-    if (hasStealableContents) {
-        // Return the old contents and give the detached buffer a pointer to
-        // freshly allocated memory that we will never write to and should
-        // never get committed.
-        buffer->setOwnsData(DoesntOwnData);
-        if (!ArrayBufferObject::detach(cx, buffer, newContents)) {
-            js_free(newContents.data());
-            return BufferContents::createPlain(nullptr);
-        }
+    if (!forceCopy && buffer->ownsData()) {
+        // Return the old contents and reset the detached buffer's data
+        // pointer. This pointer should never be accessed.
+        auto newContents = BufferContents::createPlain(nullptr);
+        buffer->setOwnsData(DoesntOwnData); // Do not free the stolen data.
+        ArrayBufferObject::detach(cx, buffer, newContents);
+        buffer->setOwnsData(DoesntOwnData); // Do not free the nullptr.
         return oldContents;
     }
 
-    // Create a new chunk of memory to return since we cannot steal the
-    // existing contents away from the buffer.
-    memcpy(newContents.data(), oldContents.data(), buffer->byteLength());
-    if (!ArrayBufferObject::detach(cx, buffer, oldContents)) {
-        js_free(newContents.data());
+    // Create a new chunk of memory to return since we cannot remove the
+    // existing contents pointer from the buffer.
+    BufferContents contentsCopy = AllocateArrayBufferContents(cx, buffer->byteLength());
+    if (!contentsCopy)
         return BufferContents::createPlain(nullptr);
-    }
-    return newContents;
+
+    if (buffer->byteLength() > 0)
+        memcpy(contentsCopy.data(), oldContents.data(), buffer->byteLength());
+    ArrayBufferObject::detach(cx, buffer, oldContents);
+    return contentsCopy;
 }
 
 /* static */ void
 ArrayBufferObject::addSizeOfExcludingThis(JSObject* obj, mozilla::MallocSizeOf mallocSizeOf,
                                           JS::ClassInfo* info)
 {
     ArrayBufferObject& buffer = AsArrayBuffer(obj);
 
@@ -1027,20 +1028,21 @@ ArrayBufferViewObject::trace(JSTracer* t
     HeapSlot& bufSlot = obj->getFixedSlotRef(TypedArrayObject::BUFFER_SLOT);
     TraceEdge(trc, &bufSlot, "typedarray.buffer");
 
     // Update obj's data pointer if it moved.
     if (bufSlot.isObject()) {
         if (IsArrayBuffer(&bufSlot.toObject())) {
             ArrayBufferObject& buf = AsArrayBuffer(MaybeForwarded(&bufSlot.toObject()));
             uint32_t offset = uint32_t(obj->getFixedSlot(TypedArrayObject::BYTEOFFSET_SLOT).toInt32());
-            MOZ_ASSERT(buf.dataPointer() != nullptr);
             MOZ_ASSERT(offset <= INT32_MAX);
 
             if (buf.forInlineTypedObject()) {
+                MOZ_ASSERT(buf.dataPointer() != nullptr);
+
                 // The data is inline with an InlineTypedObject associated with the
                 // buffer. Get a new address for the typed object if it moved.
                 JSObject* view = buf.firstView();
 
                 // Mark the object to move it into the tenured space.
                 TraceManuallyBarrieredEdge(trc, &view, "typed array nursery owner");
                 MOZ_ASSERT(view->is<InlineTypedObject>());
                 MOZ_ASSERT(view != obj);
@@ -1050,16 +1052,18 @@ ArrayBufferViewObject::trace(JSTracer* t
                 obj->setPrivateUnbarriered(dstData);
 
                 // We can't use a direct forwarding pointer here, as there might
                 // not be enough bytes available, and other views might have data
                 // pointers whose forwarding pointers would overlap this one.
                 trc->runtime()->gc.nursery.maybeSetForwardingPointer(trc, srcData, dstData,
                                                                      /* direct = */ false);
             } else {
+                MOZ_ASSERT_IF(buf.dataPointer() == nullptr, offset == 0);
+
                 // The data may or may not be inline with the buffer. The buffer
                 // can only move during a compacting GC, in which case its
                 // objectMoved hook has already updated the buffer's data pointer.
                 obj->initPrivate(buf.dataPointer() + offset);
             }
         }
     }
 }
@@ -1076,17 +1080,16 @@ bool
 JSObject::is<js::ArrayBufferObjectMaybeShared>() const
 {
     return is<ArrayBufferObject>() || is<SharedArrayBufferObject>();
 }
 
 void
 ArrayBufferViewObject::notifyBufferDetached(void* newData)
 {
-    MOZ_ASSERT(newData != nullptr);
     if (is<DataViewObject>()) {
         as<DataViewObject>().notifyBufferDetached(newData);
     } else if (is<TypedArrayObject>()) {
         if (as<TypedArrayObject>().isSharedMemory())
             return;
         as<TypedArrayObject>().notifyBufferDetached(newData);
     } else {
         as<OutlineTypedObject>().notifyBufferDetached(newData);
@@ -1185,30 +1188,31 @@ JS_DetachArrayBuffer(JSContext* cx, Hand
 {
     if (!obj->is<ArrayBufferObject>()) {
         JS_ReportError(cx, "ArrayBuffer object required");
         return false;
     }
 
     Rooted<ArrayBufferObject*> buffer(cx, &obj->as<ArrayBufferObject>());
 
-    if (changeData == ChangeData && buffer->hasStealableContents()) {
-        ArrayBufferObject::BufferContents newContents =
-            AllocateArrayBufferContents(cx, buffer->byteLength());
-        if (!newContents)
-            return false;
-        if (!ArrayBufferObject::detach(cx, buffer, newContents)) {
-            js_free(newContents.data());
-            return false;
-        }
-    } else {
-        if (!ArrayBufferObject::detach(cx, buffer, buffer->contents()))
-            return false;
+    if (buffer->isDetached())
+        return true;
+
+    if (buffer->isWasm()) {
+        JS_ReportError(cx, "Cannot detach WASM ArrayBuffer");
+        return false;
     }
 
+    if (!buffer->hasDetachableContents()) {
+        JS_ReportError(cx, "Cannot detach ArrayBuffer");
+        return false;
+    }
+
+    ArrayBufferObject::detach(cx, buffer, buffer->contents());
+
     return true;
 }
 
 JS_FRIEND_API(bool)
 JS_IsDetachedArrayBufferObject(JSObject* obj)
 {
     obj = CheckedUnwrap(obj);
     if (!obj)
@@ -1281,28 +1285,24 @@ JS_StealArrayBufferContents(JSContext* c
         return nullptr;
 
     if (!obj->is<ArrayBufferObject>()) {
         JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_TYPED_ARRAY_BAD_ARGS);
         return nullptr;
     }
 
     Rooted<ArrayBufferObject*> buffer(cx, &obj->as<ArrayBufferObject>());
-    if (buffer->isDetached()) {
-        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_TYPED_ARRAY_DETACHED);
-        return nullptr;
-    }
 
-    // The caller assumes that a plain malloc'd buffer is returned.
-    // hasStealableContents is true for mapped buffers, so we must additionally
-    // require that the buffer is plain. In the future, we could consider
-    // returning something that handles releasing the memory.
-    bool hasStealableContents = buffer->hasStealableContents() && buffer->hasMallocedContents();
+    // The caller assumes that a plain malloc'd buffer is returned. ownsData is
+    // true for (owned) mapped buffers, so we must additionally require that
+    // the buffer is plain. In the future, we could consider returning
+    // something that handles releasing the memory.
+    bool forceCopy = !buffer->ownsData() || !buffer->hasMallocedContents();
 
-    return ArrayBufferObject::stealContents(cx, buffer, hasStealableContents).data();
+    return ArrayBufferObject::stealContents(cx, buffer, forceCopy).data();
 }
 
 JS_PUBLIC_API(JSObject*)
 JS_NewMappedArrayBufferWithContents(JSContext* cx, size_t nbytes, void* data)
 {
     MOZ_ASSERT(data);
     ArrayBufferObject::BufferContents contents =
         ArrayBufferObject::BufferContents::create<ArrayBufferObject::MAPPED>(data);
--- a/js/src/vm/ArrayBufferObject.h
+++ b/js/src/vm/ArrayBufferObject.h
@@ -208,19 +208,16 @@ class ArrayBufferObject : public ArrayBu
     static const JSFunctionSpec jsstaticfuncs[];
     static const JSPropertySpec jsstaticprops[];
 
     static bool byteLengthGetter(JSContext* cx, unsigned argc, Value* vp);
 
     static bool fun_slice(JSContext* cx, unsigned argc, Value* vp);
 
     static bool fun_isView(JSContext* cx, unsigned argc, Value* vp);
-#ifdef NIGHTLY_BUILD
-    static bool fun_transfer(JSContext* cx, unsigned argc, Value* vp);
-#endif
 
     static bool fun_species(JSContext* cx, unsigned argc, Value* vp);
 
     static bool class_constructor(JSContext* cx, unsigned argc, Value* vp);
 
     static ArrayBufferObject* create(JSContext* cx, uint32_t nbytes,
                                      BufferContents contents,
                                      OwnsState ownsState = OwnsData,
@@ -241,31 +238,29 @@ class ArrayBufferObject : public ArrayBu
 
     static void copyData(Handle<ArrayBufferObject*> toBuffer,
                          Handle<ArrayBufferObject*> fromBuffer,
                          uint32_t fromIndex, uint32_t count);
 
     static void trace(JSTracer* trc, JSObject* obj);
     static void objectMoved(JSObject* obj, const JSObject* old);
 
+    // Detach the ArrayBuffer and return its contents (if possible), or
+    // possibly a copy of its contents (if forceCopy is passed or the buffer
+    // does not own the data).
     static BufferContents stealContents(JSContext* cx,
                                         Handle<ArrayBufferObject*> buffer,
-                                        bool hasStealableContents);
-
-    bool hasStealableContents() const {
-        // Inline elements strictly adhere to the corresponding buffer.
-        if (!ownsData())
-            return false;
+                                        bool forceCopy);
 
-        // Detached contents aren't transferrable because we want a detached
-        // buffer's contents to be backed by zeroed memory equal in length to
-        // the original buffer contents.  Transferring these contents would
-        // allocate new ones based on the current byteLength, which is 0 for a
-        // detached buffer -- not the original byteLength.
-        return !isDetached();
+    // Some types of buffer are simply not willing to be detached, eg Wasm
+    // buffers.
+    bool hasDetachableContents() const {
+        if (isDetached())
+            return false;
+        return isPlain() || isMapped();
     }
 
     // Return whether the buffer is allocated by js_malloc and should be freed
     // with js_free.
     bool hasMallocedContents() const {
         return (ownsData() && isPlain()) || isWasmMalloced();
     }
 
@@ -281,18 +276,18 @@ class ArrayBufferObject : public ArrayBu
 
     bool addView(JSContext* cx, JSObject* view);
 
     void setNewOwnedData(FreeOp* fop, BufferContents newContents);
     void changeContents(JSContext* cx, BufferContents newContents);
 
     // Detach this buffer from its original memory.  (This necessarily makes
     // views of this buffer unusable for modifying that original memory.)
-    static MOZ_MUST_USE bool
-    detach(JSContext* cx, Handle<ArrayBufferObject*> buffer, BufferContents newContents);
+    static void detach(JSContext* cx, Handle<ArrayBufferObject*> buffer,
+                       BufferContents newContents);
 
   private:
     void changeViewContents(JSContext* cx, ArrayBufferViewObject* view,
                             uint8_t* oldDataPointer, BufferContents newContents);
     void setFirstView(ArrayBufferViewObject* view);
 
     uint8_t* inlineDataPointer() const;
 
@@ -320,16 +315,18 @@ class ArrayBufferObject : public ArrayBu
     BufferKind bufferKind() const { return BufferKind(flags() & BUFFER_KIND_MASK); }
     bool isPlain() const { return bufferKind() == PLAIN; }
     bool isWasmMapped() const { return bufferKind() == WASM_MAPPED; }
     bool isWasmMalloced() const { return bufferKind() == WASM_MALLOCED; }
     bool isWasm() const { return isWasmMapped() || isWasmMalloced(); }
     bool isMapped() const { return bufferKind() == MAPPED; }
     bool isDetached() const { return flags() & DETACHED; }
 
+    bool ownsData() const { return flags() & OWNS_DATA; }
+
     static ArrayBufferObject* createForWasm(JSContext* cx, uint32_t numBytes, bool signalsForOOB);
     static bool prepareForAsmJS(JSContext* cx, Handle<ArrayBufferObject*> buffer, bool signalsForOOB);
 
     static void finalize(FreeOp* fop, JSObject* obj);
 
     static BufferContents createMappedContents(int fd, size_t offset, size_t length);
 
     static size_t offsetOfFlagsSlot() {
@@ -350,17 +347,16 @@ class ArrayBufferObject : public ArrayBu
 
   protected:
     void setDataPointer(BufferContents contents, OwnsState ownsState);
     void setByteLength(uint32_t length);
 
     uint32_t flags() const;
     void setFlags(uint32_t flags);
 
-    bool ownsData() const { return flags() & OWNS_DATA; }
     void setOwnsData(OwnsState owns) {
         setFlags(owns ? (flags() | OWNS_DATA) : (flags() & ~OWNS_DATA));
     }
 
     bool hasTypedObjectViews() const { return flags() & TYPED_OBJECT_VIEWS; }
 
     void setIsWasmMalloced() { setFlags((flags() & ~KIND_MASK) | WASM_MALLOCED); }
     void setIsDetached() { setFlags(flags() | DETACHED); }
--- a/js/src/vm/StructuredClone.cpp
+++ b/js/src/vm/StructuredClone.cpp
@@ -1334,23 +1334,20 @@ JSStructuredCloneWriter::transferOwnersh
         if (cls == ESClass::ArrayBuffer) {
             // The current setup of the array buffer inheritance hierarchy doesn't
             // lend itself well to generic manipulation via proxies.
             Rooted<ArrayBufferObject*> arrayBuffer(context(), &CheckedUnwrap(obj)->as<ArrayBufferObject>());
             size_t nbytes = arrayBuffer->byteLength();
 
             // Structured cloning currently only has optimizations for mapped
             // and malloc'd buffers, not asm.js-ified buffers.
-            bool hasStealableContents = arrayBuffer->hasStealableContents() &&
-                                        (arrayBuffer->isMapped() || arrayBuffer->hasMallocedContents());
-
             ArrayBufferObject::BufferContents bufContents =
-                ArrayBufferObject::stealContents(context(), arrayBuffer, hasStealableContents);
+                ArrayBufferObject::stealContents(context(), arrayBuffer, false);
             if (!bufContents)
-                return false; // Destructor will clean up the already-transferred data.
+                return false; // already transferred data or non-transferable buffer type
 
             content = bufContents.data();
             tag = SCTAG_TRANSFER_MAP_ARRAY_BUFFER;
             if (bufContents.kind() == ArrayBufferObject::MAPPED)
                 ownership = JS::SCTAG_TMO_MAPPED_DATA;
             else
                 ownership = JS::SCTAG_TMO_ALLOC_DATA;
             extraData = nbytes;