The default bug view has changed. See this FAQ.

GC hazard in GetProperty from jsxml.c

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.12, verified1.8.1.4})

Trunk
verified1.8.0.12, verified1.8.1.4
Points:
---
Bug Flags:
blocking1.8.1.4 +
wanted1.8.1.x +
blocking1.8.0.12 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(5 attachments, 4 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
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.       
Assignee: general → igor
(Assignee)

Comment 2

10 years ago
This is a regression introduced in bug 336921. Prior to it unrooted nameqn would never be stored in targetprop.
(Assignee)

Comment 3

10 years ago
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.
Attachment #255222 - Flags: review?(brendan)
(Assignee)

Comment 4

10 years ago
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;
}
(Assignee)

Comment 5

10 years ago
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;
}
(Assignee)

Updated

10 years ago
Attachment #255222 - Attachment is obsolete: true
Attachment #255222 - Flags: review?(brendan)
(Assignee)

Updated

10 years ago
Attachment #255223 - Flags: review?(brendan)
(Assignee)

Updated

10 years ago
Attachment #255223 - Attachment is patch: true
Attachment #255223 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 6

10 years ago
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;
}
Attachment #255223 - Attachment is obsolete: true
Attachment #255259 - Flags: review?(brendan)
Attachment #255223 - Flags: review?(brendan)
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
Attachment #255259 - Flags: review?(brendan) → review+
(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
(Assignee)

Comment 9

10 years ago
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.
Attachment #255259 - Attachment is obsolete: true
Attachment #255316 - Flags: review?(brendan)
(Assignee)

Comment 10

10 years ago
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;
}
Status: NEW → ASSIGNED
Comment on attachment 255316 [details] [diff] [review]
Fix v4

Looks good, r=me. Thanks,

/be
Attachment #255316 - Flags: review?(brendan)
Attachment #255316 - Flags: review+
Attachment #255316 - Flags: approval1.8.1.3?
Attachment #255316 - Flags: approval1.8.0.11?
(Assignee)

Comment 12

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 13

10 years ago
Created attachment 256371 [details]
e4x/GC/regress-370488.js

I added this locally on 2/17 but forgot to record it here.

Updated

10 years ago
Flags: in-testsuite+
Whiteboard: [sg:critical?]

Comment 14

10 years ago
verified fixed 1.9.0 20070226 windows/mac*/linux.
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Comment on attachment 255316 [details] [diff] [review]
Fix v4

Would like a second review for a non-trivial js-engine patch on the branch.
Attachment #255316 - Flags: review?(crowder)

Updated

10 years ago
Attachment #255316 - Flags: review?(crowder) → review+
Comment on attachment 255316 [details] [diff] [review]
Fix v4

approved for 1.8.0.12 and 1.8.1.4, a=dveditz
Attachment #255316 - Flags: approval1.8.1.4?
Attachment #255316 - Flags: approval1.8.1.4+
Attachment #255316 - Flags: approval1.8.0.12?
Attachment #255316 - Flags: approval1.8.0.12+
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
(Assignee)

Comment 17

10 years ago
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
Keywords: fixed1.8.1.4
(Assignee)

Comment 18

10 years ago
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.
Attachment #260647 - Flags: review?(brendan)
Attachment #260647 - Flags: approval1.8.0.12?
(Assignee)

Comment 19

10 years ago
Created attachment 260648 [details]
diff between trunk and 1.8.0 versions of v4
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
Attachment #260647 - Flags: review?(brendan) → review+
(Assignee)

Comment 21

10 years ago
(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. 
(Assignee)

Updated

10 years ago
Attachment #255316 - Flags: approval1.8.0.12+
Comment on attachment 260647 [details] [diff] [review]
Fix v4 for 1.8.0

approved for 1.8.0.12, a=dveditz for release-drivers
Attachment #260647 - Flags: approval1.8.0.12? → approval1.8.0.12+
(Assignee)

Comment 23

10 years ago
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.
Attachment #260647 - Attachment is obsolete: true
Attachment #261106 - Flags: review+
Attachment #261106 - Flags: approval1.8.0.12?
(Assignee)

Comment 24

10 years ago
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.
(Assignee)

Updated

10 years ago
Attachment #260647 - Flags: approval1.8.0.12+
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
Attachment #261106 - Flags: approval1.8.0.12? → approval1.8.0.12+
(Assignee)

Comment 26

10 years ago
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
Keywords: fixed1.8.0.12

Comment 27

10 years ago
verified fixed windows, macppc, linux 1.8.0, 1.8.1 shell 20070417
Keywords: fixed1.8.0.12, fixed1.8.1.4 → verified1.8.0.12, verified1.8.1.4
Group: security
You need to log in before you can comment on or make changes to this bug.