Bug 668915 - JSAutoStructuredCloneBuffer shouldn't require a 'cx' (r=bent,jorendorff)
☠☠ backed out by ac34cb0bb085 ☠ ☠
authorLuke Wagner <luke@mozilla.com>
Tue, 12 Jul 2011 10:42:24 -0700
changeset 73470 2e0fea2cbd9bb8e05e227828a63f5c0bb2d423e9
parent 73469 d146b9c0f92eef4292dcae584557d98bac68c0bb
child 73471 4f686c29a28e84a072f0a45460f3b9409fa09714
push id841
push userlwagner@mozilla.com
push dateThu, 28 Jul 2011 20:48:36 +0000
treeherdermozilla-inbound@2e0fea2cbd9b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbent, jorendorff
bugs668915
milestone8.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 668915 - JSAutoStructuredCloneBuffer shouldn't require a 'cx' (r=bent,jorendorff)
dom/base/nsGlobalWindow.cpp
dom/indexedDB/AsyncConnectionHelper.cpp
dom/indexedDB/IDBCursor.cpp
dom/indexedDB/IDBIndex.cpp
dom/indexedDB/IDBObjectStore.cpp
dom/workers/Events.cpp
dom/workers/WorkerPrivate.cpp
dom/workers/XMLHttpRequestPrivate.cpp
js/src/Makefile.in
js/src/jsapi.cpp
js/src/jsapi.h
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -5981,17 +5981,17 @@ PostMessageEvent::Run()
     }
   }
 
   // If we bailed before this point we're going to leak mMessage, but
   // that's probably better than crashing.
 
   // Ensure that the buffer is freed even if we fail to post the message
   JSAutoStructuredCloneBuffer buffer;
-  buffer.adopt(cx, mMessage, mMessageLen);
+  buffer.adopt(mMessage, mMessageLen);
   mMessage = nsnull;
   mMessageLen = 0;
 
   nsRefPtr<nsGlobalWindow> targetWindow;
   if (mTargetWindow->IsClosedOrClosing() ||
       !(targetWindow = mTargetWindow->GetCurrentInnerWindowInternal()) ||
       targetWindow->IsClosedOrClosing())
     return NS_OK;
@@ -6035,17 +6035,17 @@ PostMessageEvent::Run()
       return NS_OK;
   }
 
   // Deserialize the structured clone data
   jsval messageData;
   {
     JSAutoRequest ar(cx);
 
-    if (!buffer.read(&messageData, cx, nsnull))
+    if (!buffer.read(cx, &messageData, nsnull))
       return NS_ERROR_DOM_DATA_CLONE_ERR;
   }
 
   // Create the event
   nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(targetWindow->mDocument);
   if (!domDoc)
     return NS_OK;
   nsCOMPtr<nsIDOMEvent> event;
--- a/dom/indexedDB/AsyncConnectionHelper.cpp
+++ b/dom/indexedDB/AsyncConnectionHelper.cpp
@@ -501,17 +501,17 @@ AsyncConnectionHelper::ConvertCloneBuffe
   NS_ASSERTION(aCx, "Null context!");
   NS_ASSERTION(aResult, "Null pointer!");
 
   JSAutoRequest ar(aCx);
 
   nsresult rv = ConvertCloneBuffersToArrayInternal(aCx, aBuffers, aResult);
 
   for (PRUint32 index = 0; index < aBuffers.Length(); index++) {
-    aBuffers[index].clear(aCx);
+    aBuffers[index].clear();
   }
   aBuffers.Clear();
 
   return rv;
 }
 
 NS_IMETHODIMP_(nsrefcnt)
 TransactionPoolEventTarget::AddRef()
--- a/dom/indexedDB/IDBCursor.cpp
+++ b/dom/indexedDB/IDBCursor.cpp
@@ -470,17 +470,17 @@ IDBCursor::GetValue(JSContext* aCx,
       mRooted = true;
     }
 
     if (!IDBObjectStore::DeserializeValue(aCx, mCloneBuffer, &mCachedValue)) {
       mCachedValue = JSVAL_VOID;
       return NS_ERROR_DOM_DATA_CLONE_ERR;
     }
 
-    mCloneBuffer.clear(aCx);
+    mCloneBuffer.clear();
     mHaveCachedValue = true;
   }
 
   *aValue = mCachedValue;
   return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -715,17 +715,17 @@ ContinueHelper::GetSuccessResult(JSConte
                  !mObjectKey.IsUnset(), "Bad key!");
 
     // Set new values.
     mCursor->mKey = mKey;
     mCursor->mObjectKey = mObjectKey;
     mCursor->mContinueToKey = Key::UNSETKEY;
 
     mCursor->mCloneBuffer.swap(mCloneBuffer);
-    mCloneBuffer.clear(aCx);
+    mCloneBuffer.clear();
 
     nsresult rv = WrapNative(aCx, mCursor, aVal);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   return NS_OK;
 }
 
--- a/dom/indexedDB/IDBIndex.cpp
+++ b/dom/indexedDB/IDBIndex.cpp
@@ -777,17 +777,17 @@ GetHelper::DoDatabaseWork(mozIStorageCon
 }
 
 nsresult
 GetHelper::GetSuccessResult(JSContext* aCx,
                             jsval* aVal)
 {
   bool result = IDBObjectStore::DeserializeValue(aCx, mCloneBuffer, aVal);
 
-  mCloneBuffer.clear(aCx);
+  mCloneBuffer.clear();
 
   NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
   return NS_OK;
 }
 
 nsresult
 GetAllKeysHelper::DoDatabaseWork(mozIStorageConnection* aConnection)
 {
@@ -1051,17 +1051,17 @@ nsresult
 GetAllHelper::GetSuccessResult(JSContext* aCx,
                                jsval* aVal)
 {
   NS_ASSERTION(mCloneBuffers.Length() <= mLimit, "Too many results!");
 
   nsresult rv = ConvertCloneBuffersToArray(aCx, mCloneBuffers, aVal);
 
   for (PRUint32 index = 0; index < mCloneBuffers.Length(); index++) {
-    mCloneBuffers[index].clear(aCx);
+    mCloneBuffers[index].clear();
   }
 
   NS_ENSURE_SUCCESS(rv, rv);
   return NS_OK;
 }
 
 nsresult
 OpenKeyCursorHelper::DoDatabaseWork(mozIStorageConnection* aConnection)
--- a/dom/indexedDB/IDBObjectStore.cpp
+++ b/dom/indexedDB/IDBObjectStore.cpp
@@ -834,48 +834,27 @@ IDBObjectStore::GetStructuredCloneDataFr
   }
 #endif
 
   const PRUint8* data;
   PRUint32 dataLength;
   nsresult rv = aStatement->GetSharedBlob(aIndex, &dataLength, &data);
   NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
 
-  JSContext* cx;
-  rv = nsContentUtils::ThreadJSContextStack()->GetSafeJSContext(&cx);
-  NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
-
-  JSAutoRequest ar(cx);
-
-  uint64* newData = static_cast<uint64*>(JS_malloc(cx, dataLength));
-  NS_ENSURE_TRUE(newData, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
-
-  memcpy(newData, data, dataLength);
-  aBuffer.adopt(cx, newData, dataLength);
-
-  return NS_OK;
+  return aBuffer.copy(reinterpret_cast<const uint64 *>(data), dataLength) ?
+         NS_OK :
+         NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
 }
 
 // static
 void
 IDBObjectStore::ClearStructuredCloneBuffer(JSAutoStructuredCloneBuffer& aBuffer)
 {
   if (aBuffer.data()) {
-    JSContext* cx;
-    if (NS_SUCCEEDED(nsContentUtils::ThreadJSContextStack()->
-                     GetSafeJSContext(&cx))) {
-      JSAutoRequest ar(cx);
-      aBuffer.clear(cx);
-    }
-    else {
-      NS_WARNING("Couldn't get safe JSContext! Leaking data!");
-      uint64* data;
-      size_t length;
-      aBuffer.steal(&data, &length);
-    }
+    aBuffer.clear();
   }
 }
 
 // static
 bool
 IDBObjectStore::DeserializeValue(JSContext* aCx,
                                  JSAutoStructuredCloneBuffer& aBuffer,
                                  jsval* aValue,
@@ -888,17 +867,17 @@ IDBObjectStore::DeserializeValue(JSConte
 
   if (!aBuffer.data()) {
     *aValue = JSVAL_VOID;
     return true;
   }
 
   JSAutoRequest ar(aCx);
 
-  return aBuffer.read(aValue, aCx, aCallbacks, aClosure);
+  return aBuffer.read(aCx, aValue, aCallbacks, aClosure);
 }
 
 // static
 bool
 IDBObjectStore::SerializeValue(JSContext* aCx,
                                JSAutoStructuredCloneBuffer& aBuffer,
                                jsval aValue,
                                JSStructuredCloneCallbacks* aCallbacks,
@@ -1895,17 +1874,17 @@ AddHelper::DoDatabaseWork(mozIStorageCon
 }
 
 nsresult
 AddHelper::GetSuccessResult(JSContext* aCx,
                             jsval* aVal)
 {
   NS_ASSERTION(!mKey.IsUnset(), "Badness!");
 
-  mCloneBuffer.clear(aCx);
+  mCloneBuffer.clear();
 
   return IDBObjectStore::GetJSValFromKey(mKey, aCx, aVal);
 }
 
 nsresult
 GetHelper::DoDatabaseWork(mozIStorageConnection* aConnection)
 {
   NS_PRECONDITION(aConnection, "Passed a null connection!");
@@ -1950,17 +1929,17 @@ GetHelper::DoDatabaseWork(mozIStorageCon
 }
 
 nsresult
 GetHelper::GetSuccessResult(JSContext* aCx,
                             jsval* aVal)
 {
   bool result = IDBObjectStore::DeserializeValue(aCx, mCloneBuffer, aVal);
 
-  mCloneBuffer.clear(aCx);
+  mCloneBuffer.clear();
 
   NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
   return NS_OK;
 }
 
 nsresult
 DeleteHelper::DoDatabaseWork(mozIStorageConnection* aConnection)
 {
@@ -2525,14 +2504,14 @@ nsresult
 GetAllHelper::GetSuccessResult(JSContext* aCx,
                                jsval* aVal)
 {
   NS_ASSERTION(mCloneBuffers.Length() <= mLimit, "Too many results!");
 
   nsresult rv = ConvertCloneBuffersToArray(aCx, mCloneBuffers, aVal);
 
   for (PRUint32 index = 0; index < mCloneBuffers.Length(); index++) {
-    mCloneBuffers[index].clear(aCx);
+    mCloneBuffers[index].clear();
   }
 
   NS_ENSURE_SUCCESS(rv, rv);
   return NS_OK;
 }
--- a/dom/workers/Events.cpp
+++ b/dom/workers/Events.cpp
@@ -560,23 +560,23 @@ private:
     MessageEvent* event = GetInstancePrivate(aCx, aObj, name);
     if (!event) {
       return false;
     }
 
     // Deserialize and save the data value if we can.
     if (slot == SLOT_data && event->mData) {
       JSAutoStructuredCloneBuffer buffer;
-      buffer.adopt(aCx, event->mData, event->mDataByteCount);
+      buffer.adopt(event->mData, event->mDataByteCount);
 
       event->mData = NULL;
       event->mDataByteCount = 0;
 
       jsval data;
-      if (!buffer.read(&data) ||
+      if (!buffer.read(aCx, &data) ||
           !JS_SetReservedSlot(aCx, aObj, slot, data)) {
         return false;
       }
 
       *aVp = data;
       return true;
     }
 
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -527,17 +527,17 @@ public:
   {
     aData.steal(&mData, &mDataByteCount);
   }
 
   bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
   {
     JSAutoStructuredCloneBuffer buffer;
-    buffer.adopt(aCx, mData, mDataByteCount);
+    buffer.adopt(mData, mDataByteCount);
 
     mData = nsnull;
     mDataByteCount = 0;
 
     bool mainRuntime;
     JSObject* target;
     if (mTarget == ParentThread) {
       mainRuntime = !aWorkerPrivate->GetParent();
--- a/dom/workers/XMLHttpRequestPrivate.cpp
+++ b/dom/workers/XMLHttpRequestPrivate.cpp
@@ -950,27 +950,27 @@ public:
       nsIXPConnect* xpc = nsContentUtils::XPConnect();
       NS_ASSERTION(xpc, "This should never be null!");
 
       RuntimeService::AutoSafeJSContext cx;
 
       intN error = 0;
 
       jsval body;
-      if (mBody.read(&body, cx)) {
+      if (mBody.read(cx, &body)) {
         if (NS_FAILED(xpc->JSValToVariant(cx, &body,
                                           getter_AddRefs(variant)))) {
           error = INVALID_STATE_ERR;
         }
       }
       else {
         error = DATA_CLONE_ERR;
       }
 
-      mBody.clear(cx);
+      mBody.clear();
 
       if (error) {
         return error;
       }
     }
 
     if (mHasUploadListeners) {
       NS_ASSERTION(!mProxy->mUploadEventListenersAttached, "Huh?!");
--- a/js/src/Makefile.in
+++ b/js/src/Makefile.in
@@ -670,17 +670,17 @@ check-malloc-function-usage: $(filter-ou
 		"in Makefile.in" "cx->calloc_ or rt->calloc_" $^
 	$(srcdir)/config/check_source_count.py "\bjs_realloc\b" 0 \
 		"in Makefile.in" "cx->realloc_ or rt->realloc_" $^
 	$(srcdir)/config/check_source_count.py "\bjs_free\b" 0 \
 		"in Makefile.in" "cx->free_" $^
 
 	# We desire these numbers to go down, not up. See "User guide to memory
 	# management within SpiderMonkey" in jsutil.h.
-	$(srcdir)/config/check_source_count.py OffTheBooks:: 58 \
+	$(srcdir)/config/check_source_count.py OffTheBooks:: 59 \
 		"in Makefile.in" "{cx,rt}->{new_,new_array,malloc_,calloc_,realloc_}" $^
 	# This should go to zero, if possible.
 	$(srcdir)/config/check_source_count.py UnwantedForeground:: 31 \
 		"in Makefile.in" "{cx,rt}->{free_,delete_,array_delete}" $^
 
 ifneq ($(OS_ARCH),WINNT) # FIXME: this should be made work on Windows too.
 check:: check-malloc-function-usage
 endif
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -5593,17 +5593,108 @@ JS_StructuredClone(JSContext *cx, jsval 
                    void *closure)
 {
     const JSStructuredCloneCallbacks *callbacks =
         optionalCallbacks ?
         optionalCallbacks :
         cx->runtime->structuredCloneCallbacks;
     JSAutoStructuredCloneBuffer buf;
     return buf.write(cx, v, callbacks, closure) &&
-           buf.read(vp, cx, callbacks, closure);
+           buf.read(cx, vp, callbacks, closure);
+}
+
+void
+JSAutoStructuredCloneBuffer::clear()
+{
+    if (data_) {
+        Foreground::free_(data_);
+        data_ = NULL;
+        nbytes_ = 0;
+        version_ = 0;
+    }
+}
+
+void
+JSAutoStructuredCloneBuffer::adopt(uint64 *data, size_t nbytes, uint32 version)
+{
+    clear();
+    data_ = data;
+    nbytes_ = nbytes;
+    version_ = version;
+}
+
+bool
+JSAutoStructuredCloneBuffer::copy(const uint64 *srcData, size_t nbytes, uint32 version)
+{
+    uint64 *newData = static_cast<uint64 *>(OffTheBooks::malloc_(nbytes));
+    if (!newData)
+        return false;
+
+    memcpy(newData, srcData, nbytes);
+
+    clear();
+    data_ = newData;
+    nbytes_ = nbytes;
+    version_ = version;
+    return true;
+}
+void
+JSAutoStructuredCloneBuffer::steal(uint64 **datap, size_t *nbytesp, uint32 *versionp)
+{
+    *datap = data_;
+    *nbytesp = nbytes_;
+    if (versionp)
+        *versionp = version_;
+
+    data_ = NULL;
+    nbytes_ = 0;
+    version_ = 0;
+}
+
+bool
+JSAutoStructuredCloneBuffer::read(JSContext *cx, jsval *vp,
+                                  const JSStructuredCloneCallbacks *optionalCallbacks,
+                                  void *closure) const
+{
+    JS_ASSERT(cx);
+    JS_ASSERT(data_);
+    return !!JS_ReadStructuredClone(cx, data_, nbytes_, version_, vp,
+                                    optionalCallbacks, closure);
+}
+
+bool
+JSAutoStructuredCloneBuffer::write(JSContext *cx, jsval v,
+                                   const JSStructuredCloneCallbacks *optionalCallbacks,
+                                   void *closure)
+{
+    clear();
+    bool ok = !!JS_WriteStructuredClone(cx, v, &data_, &nbytes_,
+                                        optionalCallbacks, closure);
+    if (!ok) {
+        data_ = NULL;
+        nbytes_ = 0;
+        version_ = JS_STRUCTURED_CLONE_VERSION;
+    }
+    return ok;
+}
+
+void
+JSAutoStructuredCloneBuffer::swap(JSAutoStructuredCloneBuffer &other)
+{
+    uint64 *data = other.data_;
+    size_t nbytes = other.nbytes_;
+    uint32 version = other.version_;
+
+    other.data_ = this->data_;
+    other.nbytes_ = this->nbytes_;
+    other.version_ = this->version_;
+
+    this->data_ = data;
+    this->nbytes_ = nbytes;
+    this->version_ = version;
 }
 
 JS_PUBLIC_API(void)
 JS_SetStructuredCloneCallbacks(JSRuntime *rt, const JSStructuredCloneCallbacks *callbacks)
 {
     rt->structuredCloneCallbacks = callbacks;
 }
 
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -3370,122 +3370,61 @@ JS_WriteStructuredClone(JSContext *cx, j
 
 JS_PUBLIC_API(JSBool)
 JS_StructuredClone(JSContext *cx, jsval v, jsval *vp,
                    const JSStructuredCloneCallbacks *optionalCallbacks,
                    void *closure);
 
 #ifdef __cplusplus
 /* RAII sugar for JS_WriteStructuredClone. */
-class JSAutoStructuredCloneBuffer {
-    JSContext *cx_;
+class JS_PUBLIC_API(JSAutoStructuredCloneBuffer) {
     uint64 *data_;
     size_t nbytes_;
     uint32 version_;
 
   public:
     JSAutoStructuredCloneBuffer()
-        : cx_(NULL), data_(NULL), nbytes_(0), version_(JS_STRUCTURED_CLONE_VERSION) {}
+        : data_(NULL), nbytes_(0), version_(JS_STRUCTURED_CLONE_VERSION) {}
 
     ~JSAutoStructuredCloneBuffer() { clear(); }
 
-    JSContext *cx() const { return cx_; }
     uint64 *data() const { return data_; }
     size_t nbytes() const { return nbytes_; }
 
-    void clear(JSContext *cx=NULL) {
-        if (data_) {
-            if (!cx)
-                cx = cx_;
-            JS_ASSERT(cx);
-            JS_free(cx, data_);
-            cx_ = NULL;
-            data_ = NULL;
-            nbytes_ = 0;
-            version_ = 0;
-        }
-    }
+    void clear();
+
+    /* Copy some memory. It will be automatically freed by the destructor. */
+    bool copy(const uint64 *data, size_t nbytes, uint32 version=JS_STRUCTURED_CLONE_VERSION);
 
     /*
      * Adopt some memory. It will be automatically freed by the destructor.
-     * data must have been allocated using JS_malloc.
+     * data must have been allocated by the JS engine (e.g., extracted via
+     * JSAutoStructuredCloneBuffer::steal).
      */
-    void adopt(JSContext *cx, uint64 *data, size_t nbytes,
-               uint32 version=JS_STRUCTURED_CLONE_VERSION) {
-        clear(cx);
-        cx_ = cx;
-        data_ = data;
-        nbytes_ = nbytes;
-        version_ = version;
-    }
+    void adopt(uint64 *data, size_t nbytes, uint32 version=JS_STRUCTURED_CLONE_VERSION);
 
     /*
      * Remove the buffer so that it will not be automatically freed.
-     * After this, the caller is responsible for calling JS_free(*datap).
+     * After this, the caller is responsible for feeding the memory back to
+     * JSAutoStructuredCloneBuffer::adopt.
      */
-    void steal(uint64 **datap, size_t *nbytesp, JSContext **cxp=NULL,
-               uint32 *versionp=NULL) {
-        *datap = data_;
-        *nbytesp = nbytes_;
-        if (cxp)
-            *cxp = cx_;
-        if (versionp)
-            *versionp = version_;
-
-        cx_ = NULL;
-        data_ = NULL;
-        nbytes_ = 0;
-        version_ = 0;
-    }
-
-    bool read(jsval *vp, JSContext *cx=NULL,
+    void steal(uint64 **datap, size_t *nbytesp, uint32 *versionp=NULL);
+
+    bool read(JSContext *cx, jsval *vp,
               const JSStructuredCloneCallbacks *optionalCallbacks=NULL,
-              void *closure=NULL) const {
-        if (!cx)
-            cx = cx_;
-        JS_ASSERT(cx);
-        JS_ASSERT(data_);
-        return !!JS_ReadStructuredClone(cx, data_, nbytes_, version_, vp,
-                                        optionalCallbacks, closure);
-    }
+              void *closure=NULL) const;
 
     bool write(JSContext *cx, jsval v,
                const JSStructuredCloneCallbacks *optionalCallbacks=NULL,
-               void *closure=NULL) {
-        clear(cx);
-        cx_ = cx;
-        bool ok = !!JS_WriteStructuredClone(cx, v, &data_, &nbytes_,
-                                            optionalCallbacks, closure);
-        if (!ok) {
-            data_ = NULL;
-            nbytes_ = 0;
-            version_ = JS_STRUCTURED_CLONE_VERSION;
-        }
-        return ok;
-    }
+               void *closure=NULL);
 
     /**
      * Swap ownership with another JSAutoStructuredCloneBuffer.
      */
-    void swap(JSAutoStructuredCloneBuffer &other) {
-        JSContext *cx = other.cx_;
-        uint64 *data = other.data_;
-        size_t nbytes = other.nbytes_;
-        uint32 version = other.version_;
-
-        other.cx_ = this->cx_;
-        other.data_ = this->data_;
-        other.nbytes_ = this->nbytes_;
-        other.version_ = this->version_;
-
-        this->cx_ = cx;
-        this->data_ = data;
-        this->nbytes_ = nbytes;
-        this->version_ = version;
-    }
+    void swap(JSAutoStructuredCloneBuffer &other);
 
   private:
     /* Copy and assignment are not supported. */
     JSAutoStructuredCloneBuffer(const JSAutoStructuredCloneBuffer &other);
     JSAutoStructuredCloneBuffer &operator=(const JSAutoStructuredCloneBuffer &other);
 };
 #endif