Bug 724586 - Use modern tools to clean up obj_ToSource; r=Waldo

This patch replaces the fiddly strcat logic with StringBuilder usage and cleans
up the error handling control flow using RAII.

--HG--
extra : rebase_source : d42753a880a8d242627534cd5827075ea4a8966d

js/src/jscntxt.cpp

@@ -1007,7 +1007,6 @@ JSContext::JSContext(JSRuntime *rt)
     , stackIterAssertionEnabled(true)
 #endif
 {
-    PodZero(&sharpObjectMap);
     PodZero(&link);
 #ifdef JS_THREADSAFE
     PodZero(&threadLinks);

js/src/jscntxt.h

@@ -81,6 +81,8 @@ struct JSSharpObjectMap {
     jsrefcount  depth;
     uint32_t    sharpgen;
     JSHashTable *table;
+
+    JSSharpObjectMap() : depth(0), sharpgen(0), table(NULL) {}
 };
 
 namespace js {

js/src/jsobj.cpp

@@ -454,15 +454,11 @@ js_TraceSharpMap(JSTracer *trc, JSSharpObjectMap *map)
 static JSBool
 obj_toSource(JSContext *cx, uintN argc, Value *vp)
 {
-    JSBool ok;
-    jschar *ochars, *vsharp;
-    const jschar *idstrchars, *vchars;
-    size_t nchars, idstrlength, gsoplength, vlength, vsharplength, curlen;
-    const char *comma;
+    bool comma = false;
+    const jschar *vchars;
+    size_t vlength;
     Value *val;
     JSString *gsop[2];
-    JSString *valstr, *str;
-    JSLinearString *idstr;
 
     JS_CHECK_RECURSION(cx, return JS_FALSE);
 
@@ -477,7 +473,6 @@ obj_toSource(JSContext *cx, uintN argc, Value *vp)
     if (!obj)
         return false;
 
-    jschar *chars;
     JSIdArray *ida;
     bool alreadySeen = false;
     JSHashEntry *he = js_EnterSharpObject(cx, obj, &ida, &alreadySeen);
@@ -497,22 +492,26 @@ obj_toSource(JSContext *cx, uintN argc, Value *vp)
         return true;
     }
     JS_ASSERT(!IS_SHARP(he));
-    ok = JS_TRUE;
 
     if (alreadySeen)
         MAKE_SHARP(he);
 
-    /* If outermost, allocate 4 + 1 for "({})" and the terminator. */
-    chars = (jschar *) cx->malloc_(((outermost ? 4 : 2) + 1) * sizeof(jschar));
-    nchars = 0;
-    if (!chars)
-        goto error;
-    if (outermost)
-        chars[nchars++] = '(';
+    /* Automatically call js_LeaveSharpObject when we leave this frame. */
+    class AutoLeaveSharpObject {
+        JSContext *cx;
+        JSIdArray *ida;
+      public:
+        AutoLeaveSharpObject(JSContext *cx, JSIdArray *ida) : cx(cx), ida(ida) {}
+        ~AutoLeaveSharpObject() {
+            js_LeaveSharpObject(cx, &ida);
+        }
+    } autoLeaveSharpObject(cx, ida);
 
-    chars[nchars++] = '{';
-
-    comma = NULL;
+    StringBuffer buf(cx);
+    if (outermost && !buf.append('('))
+        return false;
+    if (!buf.append('{'))
+        return false;
 
     /*
      * We have four local roots for cooked and raw value GC safety.  Hoist the
@@ -521,28 +520,26 @@ obj_toSource(JSContext *cx, uintN argc, Value *vp)
      */
     val = localroot + 2;
 
-    for (jsint i = 0, length = ida->length; i < length; i++) {
+    for (jsint i = 0; i < ida->length; i++) {
         /* Get strings for id and value and GC-root them via vp. */
         jsid id = ida->vector[i];
+        JSLinearString *idstr;
 
         JSObject *obj2;
         JSProperty *prop;
-        ok = obj->lookupGeneric(cx, id, &obj2, &prop);
-        if (!ok)
-            goto error;
+        if (!obj->lookupGeneric(cx, id, &obj2, &prop))
+            return false;
 
         /*
          * Convert id to a value and then to a string.  Decide early whether we
          * prefer get/set or old getter/setter syntax.
          */
         JSString *s = ToString(cx, IdToValue(id));
-        if (!s || !(idstr = s->ensureLinear(cx))) {
-            ok = JS_FALSE;
-            goto error;
-        }
+        if (!s || !(idstr = s->ensureLinear(cx)))
+            return false;
         vp->setString(idstr);                           /* local root */
 
-        jsint valcnt = 0;
+        int valcnt = 0;
         if (prop) {
             bool doGet = true;
             if (obj2->isNative()) {
@@ -564,9 +561,8 @@ obj_toSource(JSContext *cx, uintN argc, Value *vp)
             if (doGet) {
                 valcnt = 1;
                 gsop[0] = NULL;
-                ok = obj->getGeneric(cx, id, &val[0]);
-                if (!ok)
-                    goto error;
+                if (!obj->getGeneric(cx, id, &val[0]))
+                    return false;
             }
         }
 
@@ -574,25 +570,16 @@ obj_toSource(JSContext *cx, uintN argc, Value *vp)
          * If id is a string that's not an identifier, or if it's a negative
          * integer, then it must be quoted.
          */
-        bool idIsLexicalIdentifier = IsIdentifier(idstr);
         if (JSID_IS_ATOM(id)
-            ? !idIsLexicalIdentifier
+            ? !IsIdentifier(idstr)
             : (!JSID_IS_INT(id) || JSID_TO_INT(id) < 0)) {
             s = js_QuoteString(cx, idstr, jschar('\''));
-            if (!s || !(idstr = s->ensureLinear(cx))) {
-                ok = JS_FALSE;
-                goto error;
-            }
+            if (!s || !(idstr = s->ensureLinear(cx)))
+                return false;
             vp->setString(idstr);                       /* local root */
         }
-        idstrlength = idstr->length();
-        idstrchars = idstr->getChars(cx);
-        if (!idstrchars) {
-            ok = JS_FALSE;
-            goto error;
-        }
 
-        for (jsint j = 0; j < valcnt; j++) {
+        for (int j = 0; j < valcnt; j++) {
             /*
              * Censor an accessor descriptor getter or setter part if it's
              * undefined.
@@ -601,27 +588,15 @@ obj_toSource(JSContext *cx, uintN argc, Value *vp)
                 continue;
 
             /* Convert val[j] to its canonical source form. */
-            valstr = js_ValueToSource(cx, val[j]);
-            if (!valstr) {
-                ok = JS_FALSE;
-                goto error;
-            }
+            JSString *valstr = js_ValueToSource(cx, val[j]);
+            if (!valstr)
+                return false;
             localroot[j].setString(valstr);             /* local root */
             vchars = valstr->getChars(cx);
-            if (!vchars) {
-                ok = JS_FALSE;
-                goto error;
-            }
+            if (!vchars)
+                return false;
             vlength = valstr->length();
 
-            /*
-             * If val[j] is a non-sharp object, and we're not serializing an
-             * accessor (ECMA syntax can't accommodate sharpened accessors),
-             * consider sharpening it.
-             */
-            vsharp = NULL;
-            vsharplength = 0;
-
             /*
              * Remove '(function ' from the beginning of valstr and ')' from the
              * end so that we can put "get" in front of the function definition.
@@ -657,100 +632,34 @@ obj_toSource(JSContext *cx, uintN argc, Value *vp)
                 }
             }
 
-#define SAFE_ADD(n)                                                          \
-    JS_BEGIN_MACRO                                                           \
-        size_t n_ = (n);                                                     \
-        curlen += n_;                                                        \
-        if (curlen < n_)                                                     \
-            goto overflow;                                                   \
-    JS_END_MACRO
+            if (comma && !buf.append(", "))
+                return false;
+            comma = true;
 
-            curlen = nchars;
-            if (comma)
-                SAFE_ADD(2);
-            SAFE_ADD(idstrlength + 1);
             if (gsop[j])
-                SAFE_ADD(gsop[j]->length() + 1);
-            SAFE_ADD(vsharplength);
-            SAFE_ADD(vlength);
-            /* Account for the trailing null. */
-            SAFE_ADD((outermost ? 2 : 1) + 1);
-#undef SAFE_ADD
+                if (!buf.append(gsop[j]) || !buf.append(' '))
+                    return false;
 
-            if (curlen > size_t(-1) / sizeof(jschar))
-                goto overflow;
+            if (!buf.append(idstr))
+                return false;
+            if (!buf.append(gsop[j] ? ' ' : ':'))
+                return false;
 
-            /* Allocate 1 + 1 at end for closing brace and terminating 0. */
-            chars = (jschar *) cx->realloc_((ochars = chars), curlen * sizeof(jschar));
-            if (!chars) {
-                chars = ochars;
-                goto overflow;
-            }
-
-            if (comma) {
-                chars[nchars++] = comma[0];
-                chars[nchars++] = comma[1];
-            }
-            comma = ", ";
-
-            if (gsop[j]) {
-                gsoplength = gsop[j]->length();
-                const jschar *gsopchars = gsop[j]->getChars(cx);
-                if (!gsopchars)
-                    goto overflow;
-                js_strncpy(&chars[nchars], gsopchars, gsoplength);
-                nchars += gsoplength;
-                chars[nchars++] = ' ';
-            }
-            js_strncpy(&chars[nchars], idstrchars, idstrlength);
-            nchars += idstrlength;
-            /* Extraneous space after id here will be extracted later */
-            chars[nchars++] = gsop[j] ? ' ' : ':';
-
-            if (vsharplength) {
-                js_strncpy(&chars[nchars], vsharp, vsharplength);
-                nchars += vsharplength;
-            }
-            js_strncpy(&chars[nchars], vchars, vlength);
-            nchars += vlength;
-
-            if (vsharp)
-                cx->free_(vsharp);
+            if (!buf.append(vchars, vlength))
+                return false;
         }
     }
 
-    chars[nchars++] = '}';
-    if (outermost)
-        chars[nchars++] = ')';
-    chars[nchars] = 0;
-
-  error:
-    js_LeaveSharpObject(cx, &ida);
-
-    if (!ok) {
-        if (chars)
-            Foreground::free_(chars);
+    if (!buf.append('}'))
         return false;
-    }
-
-    if (!chars) {
-        JS_ReportOutOfMemory(cx);
+    if (outermost && !buf.append(')'))
         return false;
-    }
 
-    str = js_NewString(cx, chars, nchars);
-    if (!str) {
-        cx->free_(chars);
+    JSString *str = buf.finishString();
+    if (!str)
         return false;
-    }
     vp->setString(str);
     return true;
-
-  overflow:
-    cx->free_(vsharp);
-    cx->free_(chars);
-    chars = NULL;
-    goto error;
 }
 #endif /* JS_HAS_TOSOURCE */
 

js/src/jsobj.h

@@ -1555,13 +1555,9 @@ class ValueArray {
 
 /* For manipulating JSContext::sharpObjectMap. */
 #define SHARP_BIT       ((jsatomid) 1)
-#define BUSY_BIT        ((jsatomid) 2)
 #define SHARP_ID_SHIFT  2
 #define IS_SHARP(he)    (uintptr_t((he)->value) & SHARP_BIT)
 #define MAKE_SHARP(he)  ((he)->value = (void *) (uintptr_t((he)->value)|SHARP_BIT))
-#define IS_BUSY(he)     (uintptr_t((he)->value) & BUSY_BIT)
-#define MAKE_BUSY(he)   ((he)->value = (void *) (uintptr_t((he)->value)|BUSY_BIT))
-#define CLEAR_BUSY(he)  ((he)->value = (void *) (uintptr_t((he)->value)&~BUSY_BIT))
 
 extern JSHashEntry *
 js_EnterSharpObject(JSContext *cx, JSObject *obj, JSIdArray **idap, bool *alreadySeen);

js/src/jsstrinlines.h

@@ -209,11 +209,9 @@ StringBuffer::appendInflated(const char *cstr, size_t cstrlen)
     size_t lengthBefore = length();
     if (!cb.growByUninitialized(cstrlen))
         return false;
-#if DEBUG
-    size_t oldcstrlen = cstrlen;
-    bool ok = 
-#endif
-    InflateStringToBuffer(context(), cstr, cstrlen, begin() + lengthBefore, &cstrlen);
+    DebugOnly<size_t> oldcstrlen = cstrlen;
+    DebugOnly<bool> ok = InflateStringToBuffer(context(), cstr, cstrlen,
+                                               begin() + lengthBefore, &cstrlen);
     JS_ASSERT(ok && oldcstrlen == cstrlen);
     return true;
 }