Bug 1529298 - Remove most uses of DoesntOwnData for NO_DATA and USER_OWNED and make current users that depend on |ownsData()| not do so. r=sfink
authorJeff Walden <jwalden@mit.edu>
Mon, 18 Feb 2019 23:13:48 -0800
changeset 461602 d8a4ce77f2f10639b3a5783b5961319d35342711
parent 461601 ac3ad3942d6791b7df823d87b23f68efdfa7317a
child 461603 f487c864d2abb77498667161baf187b3ecb7f995
push id35626
push usercsabou@mozilla.com
push dateThu, 28 Feb 2019 11:31:08 +0000
treeherdermozilla-central@2ea0c1db7e60 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1529298
milestone67.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 1529298 - Remove most uses of DoesntOwnData for NO_DATA and USER_OWNED and make current users that depend on |ownsData()| not do so. r=sfink
js/src/jsapi-tests/testArrayBufferWithUserOwnedContents.cpp
js/src/vm/ArrayBufferObject.cpp
js/src/vm/ArrayBufferObject.h
js/src/vm/StructuredClone.cpp
--- 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
         }