Last Comment Bug 370488 - GC hazard in GetProperty from jsxml.c
: GC hazard in GetProperty from jsxml.c
Status: VERIFIED FIXED
[sg:critical?]
: verified1.8.0.12, verified1.8.1.4
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-15 02:40 PST by Igor Bukanov
Modified: 2007-05-30 15:35 PDT (History)
4 users (show)
dveditz: blocking1.8.1.4+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.12+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix v1 (12.87 KB, patch)
2007-02-15 07:10 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
Fix v2 (12.79 KB, patch)
2007-02-15 07:21 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
Fix v3 (12.17 KB, patch)
2007-02-15 14:47 PST, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
Fix v4 (9.31 KB, patch)
2007-02-15 23:51 PST, Igor Bukanov
brendan: review+
crowderbt: review+
dveditz: approval1.8.1.4+
Details | Diff | Splinter Review
e4x/GC/regress-370488.js (2.20 KB, text/plain)
2007-02-25 10:49 PST, Bob Clary [:bc:]
no flags Details
Fix v4 for 1.8.0 (9.30 KB, patch)
2007-04-04 17:07 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
diff between trunk and 1.8.0 versions of v4 (1.22 KB, text/plain)
2007-04-04 17:10 PDT, Igor Bukanov
no flags Details
Fix v4 for 1.8.0 updated (9.29 KB, patch)
2007-04-09 23:56 PDT, Igor Bukanov
igor: review+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review
Diff between previously approved and the new version for 1.8.0 (526 bytes, text/plain)
2007-04-09 23:59 PDT, Igor Bukanov
no flags Details

Description Igor Bukanov 2007-02-15 02:40:48 PST
GetProperty from jsxml.c when called for XML list converts the propety value into  qname and without rooting it calls self recursively for list's elements passing the original value as id. This can be exploited as the following example demonstrates:

~/m/trunk/mozilla/js/src $ cat ~/s/y.js
var list = <><a><c/></a></>;
var result = list[{toString: f, c: 0}];

function f() {
  this.c++;
  if (this.c == 1) {
    for (var i = 0; i != 10000; ++i) {
      new QName();
    }
    return "c";    
  } else {
    gc();
  }
  return "c";    
}

gc();
~/m/trunk/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/y.js
before 600080, after 64624, break 09b6d000
Assertion failure: offsetInArena < GC_THINGS_SIZE, at jsgc.c:494
trace/breakpoint trap

Here on the first invocation f as toString implementation makes sure that the current free list for qnames is emptied and new arenas are allocated. On the second invication it runs GC. That collects the previously allocated qnames including the one that exists in GetProperty frame and free their GC arenas. Then uppermost GetProperty on return put the qname pointed to freed memory into live xml list object as its target property. That triggers the crash during the second GC.
Comment 1 Igor Bukanov 2007-02-15 02:50:08 PST
I checked jsxml.c and it seems only GetProperty has this type of problems. In other cases the code is either roots the result of ToXMLname or calls that under js_EnterLocalRootScope.

Yet there is another worry. ToXMLname for function qnames may end up constructing a new atom for function name. That unrooted atom is passed to js_GetProperty/js_SetProperty. As far as I can see it is not possible to trigger a full GC during those calls given that a new atom means that the property does not exist. Yet, for example, prototype chain manipulations may alter that no-full-GC property.       
Comment 2 Igor Bukanov 2007-02-15 04:37:15 PST
This is a regression introduced in bug 336921. Prior to it unrooted nameqn would never be stored in targetprop.
Comment 3 Igor Bukanov 2007-02-15 07:10:34 PST
Created attachment 255222 [details] [diff] [review]
Fix v1

The fix refactors GetProperty to chose code branches based on the type of qname, not the type of the xml objects. In addition the case of XML object is implemented through helper function that is also used in the body of the list loop avoiding all that extra overhead of intermediate list objects and multiple ToXMLName calls.

Then the fix uses explicit temporary roots for nameqn and listobj. Although after the refactoring the code is safe even without any rooting due to the presence weak roots, weak roots are too fragile to rely on them.
Comment 4 Igor Bukanov 2007-02-15 07:13:22 PST
Here is for references the refactored implementation of GetProperty as it is not very redable in the patch form:

/* ECMA-357 9.1.1.1 XML [[Get]] and 9.2.1.1 XMLList [[Get]]. */
static JSBool
GetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
    JSXML *xml, *list, *kid;
    uint32 index;
    JSObject *kidobj;
    JSXMLQName *nameqn;
    jsid funid;
    enum {
        NAME = 0,
        LIST = 1
    };
    JSObject *roots[2];
    JSTempValueRooter tvr;
    JSBool attributes;
    JSXMLArrayCursor cursor;

    xml = (JSXML *) JS_GetInstancePrivate(cx, obj, &js_XMLClass, NULL);
    if (!xml)
        return JS_TRUE;

    if (js_IdIsIndex(id, &index)) {
        if (xml->xml_class != JSXML_CLASS_LIST) {
            obj = ToXMLList(cx, OBJECT_TO_JSVAL(obj));
            if (!obj)
                return JS_FALSE;
            xml = (JSXML *) JS_GetPrivate(cx, obj);
        }

        /*
         * ECMA-357 9.2.1.1 starts here.
         *
         * Erratum: 9.2 is not completely clear that indexed properties
         * correspond to kids, but that's what it seems to say, and it's
         * what any sane user would want.
         */
        if (index < xml->xml_kids.length) {
            kid = XMLARRAY_MEMBER(&xml->xml_kids, index, JSXML);
            if (!kid) {
                *vp = JSVAL_VOID;
                return JS_TRUE;
            }
            kidobj = js_GetXMLObject(cx, kid);
            if (!kidobj)
                return JS_FALSE;

            *vp = OBJECT_TO_JSVAL(kidobj);
        } else {
            *vp = JSVAL_VOID;
        }
        return JS_TRUE;
    }

    /* ECMA-357 9.2.1.1/9.1.1.1 qname case. */

    nameqn = ToXMLName(cx, id, &funid);
    if (!nameqn)
        return JS_FALSE;
    roots[NAME] = nameqn->object;

    if (funid)
        return GetFunction(cx, obj, xml, funid, vp);

    roots[NAME] = nameqn->object;
    roots[LIST] = NULL;
    JS_PUSH_TEMP_ROOT_OBJECTS(cx, JS_ARRAY_LENGTH(roots), roots, &tvr);

    attributes = (OBJ_GET_CLASS(cx, nameqn->object) == &js_AttributeNameClass);

    roots[LIST] = js_NewXMLObject(cx, JSXML_CLASS_LIST);
    if (!roots[LIST]) {
        list = NULL;
        goto out;
    }
    list = (JSXML *) JS_GetPrivate(cx, roots[LIST]);

    if (xml->xml_class == JSXML_CLASS_LIST) {
        XMLArrayCursorInit(&cursor, &xml->xml_kids);
        while ((kid = (JSXML *) XMLArrayCursorNext(&cursor)) != NULL) {
            if (kid->xml_class == JSXML_CLASS_ELEMENT &&
                !GetPropertyHelper(cx, list, kid, nameqn, attributes)) {
                list = NULL;
                break;
            }
        }
        XMLArrayCursorFinish(&cursor);
    } else {
        if (!GetPropertyHelper(cx, list, xml, nameqn, attributes))
            list = NULL;
    }

  out:
    JS_POP_TEMP_ROOT(cx, &tvr);
    if (!list)
        return JS_FALSE;

    /*
     * Erratum: ECMA-357 9.1.1.1 misses that [[Append]] sets the given list's
     * [[TargetProperty]] to the property that is being appended. This means
     * that any use of the internal [[Get]] property returns a list which,
     * when used by e.g. [[Insert]] duplicates the last element matched by id.
     * See bug 336921.
     */
    list->xml_target = xml;
    list->xml_targetprop = nameqn;
    *vp = OBJECT_TO_JSVAL(roots[LIST]);
    return JS_TRUE;
}
Comment 5 Igor Bukanov 2007-02-15 07:21:32 PST
Created attachment 255223 [details] [diff] [review]
Fix v2

The previous code contained double-initialization of roots. here is a fixed version. The code for GetProperty is here:

/* ECMA-357 9.1.1.1 XML [[Get]] and 9.2.1.1 XMLList [[Get]]. */
static JSBool
GetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
    JSXML *xml, *list, *kid;
    uint32 index;
    JSObject *kidobj;
    JSXMLQName *nameqn;
    jsid funid;
    enum {
        NAME = 0,
        LIST = 1
    };
    JSObject *roots[2];
    JSTempValueRooter tvr;
    JSBool attributes;
    JSXMLArrayCursor cursor;

    xml = (JSXML *) JS_GetInstancePrivate(cx, obj, &js_XMLClass, NULL);
    if (!xml)
        return JS_TRUE;

    if (js_IdIsIndex(id, &index)) {
        if (xml->xml_class != JSXML_CLASS_LIST) {
            obj = ToXMLList(cx, OBJECT_TO_JSVAL(obj));
            if (!obj)
                return JS_FALSE;
            xml = (JSXML *) JS_GetPrivate(cx, obj);
        }

        /*
         * ECMA-357 9.2.1.1 starts here.
         *
         * Erratum: 9.2 is not completely clear that indexed properties
         * correspond to kids, but that's what it seems to say, and it's
         * what any sane user would want.
         */
        if (index < xml->xml_kids.length) {
            kid = XMLARRAY_MEMBER(&xml->xml_kids, index, JSXML);
            if (!kid) {
                *vp = JSVAL_VOID;
                return JS_TRUE;
            }
            kidobj = js_GetXMLObject(cx, kid);
            if (!kidobj)
                return JS_FALSE;

            *vp = OBJECT_TO_JSVAL(kidobj);
        } else {
            *vp = JSVAL_VOID;
        }
        return JS_TRUE;
    }

    /* ECMA-357 9.2.1.1/9.1.1.1 qname case. */

    nameqn = ToXMLName(cx, id, &funid);
    if (!nameqn)
        return JS_FALSE;
    roots[NAME] = nameqn->object;

    if (funid)
        return GetFunction(cx, obj, xml, funid, vp);

    roots[NAME] = nameqn->object;
    roots[LIST] = NULL;
    JS_PUSH_TEMP_ROOT_OBJECTS(cx, JS_ARRAY_LENGTH(roots), roots, &tvr);

    attributes = (OBJ_GET_CLASS(cx, nameqn->object) == &js_AttributeNameClass);

    roots[LIST] = js_NewXMLObject(cx, JSXML_CLASS_LIST);
    if (!roots[LIST]) {
        list = NULL;
        goto out;
    }
    list = (JSXML *) JS_GetPrivate(cx, roots[LIST]);

    if (xml->xml_class == JSXML_CLASS_LIST) {
        XMLArrayCursorInit(&cursor, &xml->xml_kids);
        while ((kid = (JSXML *) XMLArrayCursorNext(&cursor)) != NULL) {
            if (kid->xml_class == JSXML_CLASS_ELEMENT &&
                !GetPropertyHelper(cx, list, kid, nameqn, attributes)) {
                list = NULL;
                break;
            }
        }
        XMLArrayCursorFinish(&cursor);
    } else {
        if (!GetPropertyHelper(cx, list, xml, nameqn, attributes))
            list = NULL;
    }

  out:
    JS_POP_TEMP_ROOT(cx, &tvr);
    if (!list)
        return JS_FALSE;

    /*
     * Erratum: ECMA-357 9.1.1.1 misses that [[Append]] sets the given list's
     * [[TargetProperty]] to the property that is being appended. This means
     * that any use of the internal [[Get]] property returns a list which,
     * when used by e.g. [[Insert]] duplicates the last element matched by id.
     * See bug 336921.
     */
    list->xml_target = xml;
    list->xml_targetprop = nameqn;
    *vp = OBJECT_TO_JSVAL(roots[LIST]);
    return JS_TRUE;
}
Comment 6 Igor Bukanov 2007-02-15 14:47:17 PST
Created attachment 255259 [details] [diff] [review]
Fix v3

The new version avoids creation of intermediate list when GetProperty is called with index for the non-list case. Here is GetProperty again:

static JSBool
GetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
    JSXML *xml, *list, *kid;
    uint32 index;
    JSObject *kidobj;
    JSXMLQName *nameqn;
    jsid funid;
    enum {
        NAME = 0,
        LIST = 1
    };
    JSObject *roots[2];
    JSTempValueRooter tvr;
    JSBool attributes;
    JSXMLArrayCursor cursor;

    xml = (JSXML *) JS_GetInstancePrivate(cx, obj, &js_XMLClass, NULL);
    if (!xml)
        return JS_TRUE;

    if (js_IdIsIndex(id, &index)) {
        if (xml->xml_class != JSXML_CLASS_LIST) {
            *vp = (index == 0) ? OBJECT_TO_JSVAL(obj) : JSVAL_VOID; 
        } else {
            /*
             * ECMA-357 9.2.1.1 starts here.
             *
             * Erratum: 9.2 is not completely clear that indexed properties
             * correspond to kids, but that's what it seems to say, and it's
             * what any sane user would want.
             */
            if (index < xml->xml_kids.length) {
                kid = XMLARRAY_MEMBER(&xml->xml_kids, index, JSXML);
                if (!kid) {
                    *vp = JSVAL_VOID;
                    return JS_TRUE;
                }
                kidobj = js_GetXMLObject(cx, kid);
                if (!kidobj)
                    return JS_FALSE;
                
                *vp = OBJECT_TO_JSVAL(kidobj);
            } else {
                *vp = JSVAL_VOID;
            }
        }
        return JS_TRUE;
    }

    /* ECMA-357 9.2.1.1/9.1.1.1 qname case. */

    nameqn = ToXMLName(cx, id, &funid);
    if (!nameqn)
        return JS_FALSE;
    if (funid)
        return GetFunction(cx, obj, xml, funid, vp);

    roots[NAME] = nameqn->object;
    roots[LIST] = NULL;
    JS_PUSH_TEMP_ROOT_OBJECTS(cx, JS_ARRAY_LENGTH(roots), roots, &tvr);

    attributes = (OBJ_GET_CLASS(cx, nameqn->object) == &js_AttributeNameClass);

    roots[LIST] = js_NewXMLObject(cx, JSXML_CLASS_LIST);
    if (!roots[LIST]) {
        list = NULL;
        goto out;
    }
    list = (JSXML *) JS_GetPrivate(cx, roots[LIST]);

    if (xml->xml_class == JSXML_CLASS_LIST) {
        XMLArrayCursorInit(&cursor, &xml->xml_kids);
        while ((kid = (JSXML *) XMLArrayCursorNext(&cursor)) != NULL) {
            if (kid->xml_class == JSXML_CLASS_ELEMENT &&
                !GetNamedProperty(cx, kid, nameqn, attributes, list)) {
                list = NULL;
                break;
            }
        }
        XMLArrayCursorFinish(&cursor);
    } else {
        if (!GetNamedProperty(cx, xml, nameqn, attributes, list))
            list = NULL;
    }

  out:
    JS_POP_TEMP_ROOT(cx, &tvr);
    if (!list)
        return JS_FALSE;

    /*
     * Erratum: ECMA-357 9.1.1.1 misses that [[Append]] sets the given list's
     * [[TargetProperty]] to the property that is being appended. This means
     * that any use of the internal [[Get]] property returns a list which,
     * when used by e.g. [[Insert]] duplicates the last element matched by id.
     * See bug 336921.
     */
    list->xml_target = xml;
    list->xml_targetprop = nameqn;
    *vp = OBJECT_TO_JSVAL(roots[LIST]);
    return JS_TRUE;
}
Comment 7 Brendan Eich [:brendan] 2007-02-15 17:04:11 PST
Comment on attachment 255259 [details] [diff] [review]
Fix v3

>@@ -550,14 +551,18 @@ JS_STATIC_ASSERT(sizeof(JSTempValueUnion
>  * alternatives to JS_PUSH_TEMP_ROOT_GCTHING for JSObject and JSString. They
>  * also provide a simple way to get a single pointer to rooted JSObject or
>  * JSString via JS_PUSH_TEMP_ROOT_(OBJECT|STRTING)(cx, NULL, &tvr). Then
>  * &tvr.u.object or tvr.u.string gives the necessary pointer, which puns
>  * tvr.u.value safely because JSObject * and JSString * are GC-things and, as
>  * such, their tag bits are all zeroes.
>  *
>+ * Similar JS_PUSH_TEMP_ROOT_OBJECTS is a type safe alternative to
>+ * JS_PUSH_TEMP_ROOT too root an array of objects and it works for the same
>+ * reasons.

s/too/to/

Wonder if it wouldn't be better to use the jsval *array member, casting. Nah, never mind.

>+    ok = JS_TRUE;

Blank line here would be nice.

>+  out:
>+    XMLArrayCursorFinish(&cursor);
>+    return ok;
>+}
>+
> /* ECMA-357 9.1.1.1 XML [[Get]] and 9.2.1.1 XMLList [[Get]]. */
> static JSBool
> GetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> {
[snip...]
>+    /* ECMA-357 9.2.1.1/9.1.1.1 qname case. */
> 

Maybe this blank line is overkill for a one-line comment. Could use a major comment?

>+    roots[LIST] = js_NewXMLObject(cx, JSXML_CLASS_LIST);
>+    if (!roots[LIST]) {
>+        list = NULL;
>+        goto out;

With just one goto out, tempting to use else clause and over-indent the guts by one level. Man, I'm fussy today. Whatever you prefer ;-).

Thanks for fixing this. I should have refactored the spec when I wrote this.

/be
Comment 8 Brendan Eich [:brendan] 2007-02-15 17:08:41 PST
(In reply to comment #7)
> >+ * Similar JS_PUSH_TEMP_ROOT_OBJECTS is a type safe alternative to
> >+ * JS_PUSH_TEMP_ROOT too root an array of objects and it works for the same
> >+ * reasons.
> 
> s/too/to/
> 
> Wonder if it wouldn't be better to use the jsval *array member, casting. Nah,
> never mind.

Oh, but this patch does rely on tvr->u.array punning tvr->u.objects, so I still wonder whether objects is necessary. It seems odd to use it to avoid a cast, but imply that the punning is safe. I'd rather see a cast when punning instead of a union arm overlaying another arm, but I admit it's a style (and very minor source line bloat) issue.

/be
Comment 9 Igor Bukanov 2007-02-15 23:51:28 PST
Created attachment 255316 [details] [diff] [review]
Fix v4

The new version uses JS_PUSH_TEMP_ROOT to avoid touching jscntxt.h and replaces goto with if.
Comment 10 Igor Bukanov 2007-02-15 23:56:37 PST
Here again for references GetProperty implementation:

/* ECMA-357 9.1.1.1 XML [[Get]] and 9.2.1.1 XMLList [[Get]]. */
static JSBool
GetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
    JSXML *xml, *list, *kid;
    uint32 index;
    JSObject *kidobj, *listobj;
    JSXMLQName *nameqn;
    jsid funid;
    jsval roots[2];
    JSTempValueRooter tvr;
    JSBool attributes;
    JSXMLArrayCursor cursor;

    xml = (JSXML *) JS_GetInstancePrivate(cx, obj, &js_XMLClass, NULL);
    if (!xml)
        return JS_TRUE;

    if (js_IdIsIndex(id, &index)) {
        if (xml->xml_class != JSXML_CLASS_LIST) {
            *vp = (index == 0) ? OBJECT_TO_JSVAL(obj) : JSVAL_VOID;
        } else {
            /*
             * ECMA-357 9.2.1.1 starts here.
             *
             * Erratum: 9.2 is not completely clear that indexed properties
             * correspond to kids, but that's what it seems to say, and it's
             * what any sane user would want.
             */
            if (index < xml->xml_kids.length) {
                kid = XMLARRAY_MEMBER(&xml->xml_kids, index, JSXML);
                if (!kid) {
                    *vp = JSVAL_VOID;
                    return JS_TRUE;
                }
                kidobj = js_GetXMLObject(cx, kid);
                if (!kidobj)
                    return JS_FALSE;

                *vp = OBJECT_TO_JSVAL(kidobj);
            } else {
                *vp = JSVAL_VOID;
            }
        }
        return JS_TRUE;
    }

    /*
     * ECMA-357 9.2.1.1/9.1.1.1 qname case.
     */
    nameqn = ToXMLName(cx, id, &funid);
    if (!nameqn)
        return JS_FALSE;
    if (funid)
        return GetFunction(cx, obj, xml, funid, vp);

    roots[0] = OBJECT_TO_JSVAL(nameqn->object);
    JS_PUSH_TEMP_ROOT(cx, 1, roots, &tvr);

    listobj = js_NewXMLObject(cx, JSXML_CLASS_LIST);
    if (listobj) {
        roots[1] = OBJECT_TO_JSVAL(listobj);
        tvr.count++;
        
        list = (JSXML *) JS_GetPrivate(cx, listobj);
        attributes = (OBJ_GET_CLASS(cx, nameqn->object) ==
                      &js_AttributeNameClass);

        if (xml->xml_class == JSXML_CLASS_LIST) {
            XMLArrayCursorInit(&cursor, &xml->xml_kids);
            while ((kid = (JSXML *) XMLArrayCursorNext(&cursor)) != NULL) {
                if (kid->xml_class == JSXML_CLASS_ELEMENT &&
                    !GetNamedProperty(cx, kid, nameqn, attributes, list)) {
                    listobj = NULL;
                    break;
                }
            }
            XMLArrayCursorFinish(&cursor);
        } else {
            if (!GetNamedProperty(cx, xml, nameqn, attributes, list))
                listobj = NULL;
        }

        /*
         * Erratum: ECMA-357 9.1.1.1 misses that [[Append]] sets the given
         * list's [[TargetProperty]] to the property that is being appended.
         * This means that any use of the internal [[Get]] property returns
         * a list which, when used by e.g. [[Insert]] duplicates the last
         * element matched by id.
         * See bug 336921.
         */
        list->xml_target = xml;
        list->xml_targetprop = nameqn;
        *vp = OBJECT_TO_JSVAL(listobj);
    }

    JS_POP_TEMP_ROOT(cx, &tvr);
    return listobj != NULL;
}
Comment 11 Brendan Eich [:brendan] 2007-02-16 10:20:27 PST
Comment on attachment 255316 [details] [diff] [review]
Fix v4

Looks good, r=me. Thanks,

/be
Comment 12 Igor Bukanov 2007-02-16 12:06:11 PST
I committed the patch from comment 10 to the trunk:
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.141; previous revision: 3.140
done
Comment 13 Bob Clary [:bc:] 2007-02-25 10:49:38 PST
Created attachment 256371 [details]
e4x/GC/regress-370488.js

I added this locally on 2/17 but forgot to record it here.
Comment 14 Bob Clary [:bc:] 2007-02-28 10:07:24 PST
verified fixed 1.9.0 20070226 windows/mac*/linux.
Comment 15 Daniel Veditz [:dveditz] 2007-03-21 15:32:24 PDT
Comment on attachment 255316 [details] [diff] [review]
Fix v4

Would like a second review for a non-trivial js-engine patch on the branch.
Comment 16 Daniel Veditz [:dveditz] 2007-03-23 00:08:59 PDT
Comment on attachment 255316 [details] [diff] [review]
Fix v4

approved for 1.8.0.12 and 1.8.1.4, a=dveditz
Comment 17 Igor Bukanov 2007-04-04 16:40:57 PDT
I committed the patch from comment 9 to MOZILLA_1_8_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.60; previous revision: 3.50.2.59
done
Comment 18 Igor Bukanov 2007-04-04 17:07:33 PDT
Created attachment 260647 [details] [diff] [review]
Fix v4 for 1.8.0

1.8.0 version of the patch required hand-merging due to trailing whitespace that exists on 1.8.0 and JS_LocalScope versus js_LocalScope usages.
Comment 19 Igor Bukanov 2007-04-04 17:10:27 PDT
Created attachment 260648 [details]
diff between trunk and 1.8.0 versions of v4
Comment 20 Brendan Eich [:brendan] 2007-04-04 17:21:24 PDT
Comment on attachment 260647 [details] [diff] [review]
Fix v4 for 1.8.0

Does lack of js_LeaveLocalRootScopeWithResult on the 1.8.0 branch mean there's a GC hazard propagating listobj out?

/be
Comment 21 Igor Bukanov 2007-04-04 18:09:18 PDT
(In reply to comment #20)
> Does lack of js_LeaveLocalRootScopeWithResult on the 1.8.0 branch mean there's
> a GC hazard propagating listobj out?

I do not thing so. On the branch GetProperty is called either already inside local roots or with rooted-via-native-call rval. So js_LeaveLocalRootScopeWithResult was not necessary for GC-safty. 
Comment 22 Daniel Veditz [:dveditz] 2007-04-06 11:58:22 PDT
Comment on attachment 260647 [details] [diff] [review]
Fix v4 for 1.8.0

approved for 1.8.0.12, a=dveditz for release-drivers
Comment 23 Igor Bukanov 2007-04-09 23:56:14 PDT
Created attachment 261106 [details] [diff] [review]
Fix v4 for 1.8.0 updated

The commit for bug 366975 affected this patch as it changed the text in comments this patch removes. So here is an update.
Comment 24 Igor Bukanov 2007-04-09 23:59:38 PDT
Created attachment 261107 [details]
Diff between previously approved and the new version for 1.8.0

Here is a proof of triviality of the update as bugzilla diff can not show the difference.
Comment 25 Daniel Veditz [:dveditz] 2007-04-13 11:19:10 PDT
Comment on attachment 261106 [details] [diff] [review]
Fix v4 for 1.8.0 updated

approved for 1.8.0.12, a=dveditz for release-drivers
Comment 26 Igor Bukanov 2007-04-13 16:30:49 PDT
I committed the patch from comment 23 to MOZILLA_1_8_0_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.15.2.36; previous revision: 3.50.2.15.2.35
done
Comment 27 Bob Clary [:bc:] 2007-04-17 06:01:25 PDT
verified fixed windows, macppc, linux 1.8.0, 1.8.1 shell 20070417

Note You need to log in before you can comment on or make changes to this bug.