Bug 761552 - Part 2: Avoid external JS strings in XPCConvert. r=bholley
authorNils Maier <maierman@web.de>
Tue, 29 Jan 2013 10:50:04 -0500
changeset 120230 42c786efb5d67768f348462234a235d6b808b1e4
parent 120229 a502aa076a9416d2af5fc5eddb30f98a5d7dfe56
child 120231 d8d79ba17527141923665d79843b7b9fd1dc87a3
push id24243
push userryanvm@gmail.com
push dateWed, 30 Jan 2013 00:49:21 +0000
treeherdermozilla-central@5c248ef0fe62 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs761552
milestone21.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 761552 - Part 2: Avoid external JS strings in XPCConvert. r=bholley XPCConvert needs to convert some string types to JS strings in order to call from C++ into JS land. Make those converted JS strings allocate the buffers from the JS compartment memory pool, instead of making them external and allocated from the main heap. This improves memory reporting and may also influence memory fragmentation and/or performance.
js/xpconnect/src/XPCConvert.cpp
--- a/js/xpconnect/src/XPCConvert.cpp
+++ b/js/xpconnect/src/XPCConvert.cpp
@@ -74,24 +74,16 @@ XPCConvert::GetISupportsFromJSObject(JSO
         *iface = (nsISupports*) xpc_GetJSPrivate(obj);
         return true;
     }
     return UnwrapDOMObjectToISupports(obj, *iface);
 }
 
 /***************************************************************************/
 
-static void
-FinalizeXPCOMUCString(const JSStringFinalizer *fin, jschar *chars)
-{
-    nsMemory::Free(chars);
-}
-
-static const JSStringFinalizer sXPCOMUCStringFinalizer = { FinalizeXPCOMUCString };
-
 // static
 JSBool
 XPCConvert::NativeData2JS(XPCLazyCallContext& lccx, jsval* d, const void* s,
                           const nsXPTType& type, const nsID* iid, nsresult* pErr)
 {
     NS_PRECONDITION(s, "bad param");
     NS_PRECONDITION(d, "bad param");
 
@@ -240,68 +232,80 @@ XPCConvert::NativeData2JS(XPCLazyCallCon
                 JSString* str;
                 if (!(str = JS_NewUCStringCopyZ(cx, p)))
                     return false;
                 *d = STRING_TO_JSVAL(str);
                 break;
             }
         case nsXPTType::T_UTF8STRING:
             {
-                const nsACString* cString = *((const nsACString**)s);
+                const nsACString* utf8String = *((const nsACString**)s);
 
-                if (!cString)
+                if (!utf8String || utf8String->IsVoid())
                     break;
 
-                if (!cString->IsVoid()) {
-                    uint32_t len;
-                    jschar *p = (jschar *)UTF8ToNewUnicode(*cString, &len);
-
-                    if (!p)
-                        return false;
-
-                    JSString* jsString =
-                        JS_NewExternalString(cx, p, len,
-                                             &sXPCOMUCStringFinalizer);
-
-                    if (!jsString) {
-                        nsMemory::Free(p);
-                        return false;
-                    }
-
-                    *d = STRING_TO_JSVAL(jsString);
+                if (utf8String->IsEmpty()) {
+                    *d = JS_GetEmptyStringValue(cx);
+                    break;
                 }
 
+                const uint32_t len = CalcUTF8ToUnicodeLength(*utf8String);
+                // The cString is not empty at this point, but the calculated
+                // UTF-16 length is zero, meaning no valid conversion exists.
+                if (!len)
+                    return false;
+
+                const size_t buffer_size = (len + 1) * sizeof(PRUnichar);
+                PRUnichar* buffer =
+                    static_cast<PRUnichar*>(JS_malloc(cx, buffer_size));
+                if (!buffer)
+                    return false;
+
+                uint32_t copied;
+                if (!UTF8ToUnicodeBuffer(*utf8String, buffer, &copied) ||
+                    len != copied) {
+                    // Copy or conversion during copy failed. Did not copy the
+                    // whole string.
+                    JS_free(cx, buffer);
+                    return false;
+                }
+
+                // JS_NewUCString takes ownership on success, i.e. a
+                // successful call will make it the responsiblity of the JS VM
+                // to free the buffer.
+                JSString* str = JS_NewUCString(cx, (jschar*)buffer, len);
+                if (!str) {
+                    JS_free(cx, buffer);
+                    return false;
+                }
+
+                *d = STRING_TO_JSVAL(str);
                 break;
-
             }
         case nsXPTType::T_CSTRING:
             {
                 const nsACString* cString = *((const nsACString**)s);
 
-                if (!cString)
+                if (!cString || cString->IsVoid())
                     break;
 
-                if (!cString->IsVoid()) {
-                    PRUnichar* unicodeString = ToNewUnicode(*cString);
-                    if (!unicodeString)
-                        return false;
-
-                    JSString* jsString = JS_NewExternalString(cx,
-                                                              (jschar*)unicodeString,
-                                                              cString->Length(),
-                                                              &sXPCOMUCStringFinalizer);
-
-                    if (!jsString) {
-                        nsMemory::Free(unicodeString);
-                        return false;
-                    }
-
-                    *d = STRING_TO_JSVAL(jsString);
+                if (cString->IsEmpty()) {
+                    *d = JS_GetEmptyStringValue(cx);
+                    break;
                 }
 
+                // c-strings (binary blobs) are deliberately not converted from
+                // UTF-8 to UTF-16. T_UTF8Sting is for UTF-8 encoded strings
+                // with automatic conversion.
+                JSString* str = JS_NewStringCopyN(cx, cString->Data(),
+                                                  cString->Length());
+                if (!str)
+                    return false;
+
+                *d = STRING_TO_JSVAL(str);
                 break;
             }
 
         case nsXPTType::T_INTERFACE:
         case nsXPTType::T_INTERFACE_IS:
             {
                 nsISupports* iface = *((nsISupports**)s);
                 if (iface) {