Bug 1529298 - Remove |ArrayBufferObject::hasStealableContents()| and replace it with its contents, appropriately simplified for each calling location. r=sfink
authorJeff Walden <jwalden@mit.edu>
Mon, 18 Feb 2019 23:06:33 -0800
changeset 519483 ac3ad3942d6791b7df823d87b23f68efdfa7317a
parent 519270 f07eec8b4f2c820d2f1f15c0976f77cd0f7d9321
child 519484 d8a4ce77f2f10639b3a5783b5961319d35342711
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [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 |ArrayBufferObject::hasStealableContents()| and replace it with its contents, appropriately simplified for each calling location. r=sfink
js/src/vm/ArrayBufferObject.cpp
js/src/vm/ArrayBufferObject.h
js/src/vm/StructuredClone.cpp
--- a/js/src/vm/ArrayBufferObject.cpp
+++ b/js/src/vm/ArrayBufferObject.cpp
@@ -1678,17 +1678,17 @@ JS_FRIEND_API bool JS_DetachArrayBuffer(
     JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                               JSMSG_WASM_NO_TRANSFER);
     return false;
   }
 
   using BufferContents = ArrayBufferObject::BufferContents;
 
   BufferContents newContents =
-      buffer->hasStealableContents()
+      buffer->ownsData()
           ? BufferContents::createNoData()
           : buffer->contents();
 
   ArrayBufferObject::detach(cx, buffer, newContents);
 
   return true;
 }
 
@@ -1801,30 +1801,22 @@ JS_PUBLIC_API void* JS_StealArrayBufferC
   }
 
   if (buffer->isWasm() || buffer->isPreparedForAsmJS()) {
     JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                               JSMSG_WASM_NO_TRANSFER);
     return nullptr;
   }
 
-  // The caller assumes that a plain malloc'd buffer is returned.  To steal
-  // actual contents, then, we must have |hasStealableContents()| *and* the
-  // contents must be |isMalloced()|.  (Inline data would not be malloc'd and
-  // shouldn't pass |hasStealableContents()| anyway; no-data would be nullptr
-  // and would signal failure if we returned it, plus |hasStealableContents()|
-  // specifically excludes it; user-provided data we know nothing about at all
-  // -- although it *should* have not passed the |hasStealableContents()| check
-  // anyway because it's not owned; mapped data wouldn't be malloc'd; external
-  // data has to be freed using a provided function.)
+  // This function returns a plain malloc'd buffer for the user to own, so
+  // stealing actual contents requires |ownsData() && isMalloced()|.
   //
-  // In the future, we could consider returning something that handles
-  // releasing the memory, in the mapped-data case.
-  bool hasStealableContents =
-      buffer->hasStealableContents() && buffer->isMalloced();
+  // We might consider returning something that handles releasing data of more
+  // exotic kind at some point.
+  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,
                                                             size_t nbytes,
--- a/js/src/vm/ArrayBufferObject.h
+++ b/js/src/vm/ArrayBufferObject.h
@@ -350,29 +350,16 @@ class ArrayBufferObject : public ArrayBu
                        uint32_t fromIndex, uint32_t count);
 
   static size_t objectMoved(JSObject* obj, JSObject* old);
 
   static BufferContents stealContents(JSContext* cx,
                                       Handle<ArrayBufferObject*> buffer,
                                       bool hasStealableContents);
 
-  bool hasStealableContents() const {
-    // Inline data is always DoesntOwnData and so will fail the first test.
-    if (ownsData()) {
-      MOZ_ASSERT(!isInlineData(), "inline data is always DoesntOwnData");
-
-      // Making no data stealable is tricky, because it'd be null and usually
-      // that signals failure, so directly exclude it here.
-      return !isPreparedForAsmJS() && !isNoData() && !isWasm();
-    }
-
-    return false;
-  }
-
   static void addSizeOfExcludingThis(JSObject* obj,
                                      mozilla::MallocSizeOf mallocSizeOf,
                                      JS::ClassInfo* info);
 
   // ArrayBufferObjects (strongly) store the first view added to them, while
   // later views are (weakly) stored in the compartment's InnerViewTable
   // below. Buffers usually only have one view, so this slot optimizes for
   // the common case. Avoiding entries in the InnerViewTable saves memory and
@@ -454,24 +441,25 @@ class ArrayBufferObject : public ArrayBu
 
   static BufferContents createMappedContents(int fd, size_t offset,
                                              size_t length);
 
   static size_t offsetOfDataSlot() { return getFixedSlotOffset(DATA_SLOT); }
 
   void setHasTypedObjectViews() { setFlags(flags() | TYPED_OBJECT_VIEWS); }
 
+  bool ownsData() const { return flags() & OWNS_DATA; }
+
  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 setIsDetached() { setFlags(flags() | DETACHED); }
   void setIsPreparedForAsmJS() {
--- a/js/src/vm/StructuredClone.cpp
+++ b/js/src/vm/StructuredClone.cpp
@@ -1869,17 +1869,28 @@ bool JSStructuredCloneWriter::transferOw
         // been modified.
         point = out.iter();
         point += pointOffset;
 
         if (!JS_DetachArrayBuffer(cx, arrayBuffer)) {
           return false;
         }
       } else {
-        bool hasStealableContents = arrayBuffer->hasStealableContents();
+        // No data isn't stealable because it would fail |!bufContents|: actual
+        // malloc'd data must be created to return.
+        //
+        // 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.
+        bool hasStealableContents =
+          arrayBuffer->ownsData() && !arrayBuffer->isNoData();
 
         ArrayBufferObject::BufferContents bufContents =
             ArrayBufferObject::stealContents(cx, arrayBuffer,
                                              hasStealableContents);
         if (!bufContents) {
           return false;  // out of memory
         }