341877 - GC hazard with for-in loop

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

The implementation of for-in loop during the initialization creates an array holding object properties. However no efforts is taken to protect the array against GC. Thus if a property from the array is deleted during the loop execution and GC is triggered, the GC-ed property would be accessed during the following iterations. The following example demonstrates the problem and triggers a segfault in jsshell and browser:

var obj = { };

var prop = "xsomePropety".substr(1);

obj.first = "first"

obj[prop] = 1;

for (var elem in obj) {
	var tmp = elem.toString(); 
	delete obj[prop];
        // ensure that prop is cut from all roots
	prop = "xsomePropety".substr(2);
	obj[prop] = 2;
	delete obj[prop];
	prop = null;
	if (typeof gc == 'function')
	        gc();
	for (var i = 0; i != 50000; ++i) {
	      var tmp = 1 / 3;
	      tmp /= 10;
	}
}

Comment 1

[Brendan Eich [:brendan]]

That testcase is not crashing a debug jsshell on my macbook.

Do you mean the JSIdArray, by "an array holding object properties"?  If so, the elements of that array are property ids, which may be atom pointers.  Last-ditch GC will keep atoms, so the only hazard here is an explicit GC call, which is not supported except in the shell.  Is there a browser hazard here -- a way to trigger GC while the loop is iterating?

Pointers to get the test crashing mac js shell welcome.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: possible fix

Comment 3

[Igor Bukanov]

Attachment: Test case for the browser (it crashes the browse at least on Linux)

(In reply to comment #1)
> That testcase is not crashing a debug jsshell on my macbook.

Try to use for each instaed of just for loop.

> 
> Do you mean the JSIdArray, by "an array holding object properties"?  If so, the
> elements of that array are property ids, which may be atom pointers. 
> Last-ditch GC will keep atoms, so the only hazard here is an explicit GC call,
> which is not supported except in the shell.  Is there a browser hazard here --
> a way to trigger GC while the loop is iterating?

Yes: during 	

for (var i = 0; i != 50000; ++i) {
    var tmp = 1 / 3;
    tmp /= 10;
}

the browser callback calls JS_MaybeGC which eventually calls js_GC without GC_KEEP_ATOMS set.

Comment 4

[Brendan Eich [:brendan]]

Attachment: better fix

Comment 5

[Igor Bukanov]

Comment on attachment 226006 [details] [diff] [review]
possible fix

I think xml_enumerate should also include JSENUMERATE_MARK. Plus as a bonus the patch can also remove cursor array allocation in xml_enumerateValues as it is used only to hold cursor->index.

Comment 6

[Igor Bukanov]

I hope it is not too late to get this into 1.5.0.5 even if the patch inevitably changes public API in SpiderMonkey.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> (From update of attachment 226006 [details] [diff] [review] [edit])
> I think xml_enumerate should also include JSENUMERATE_MARK. Plus as a bonus the
> patch can also remove cursor array allocation in xml_enumerateValues as it is
> used only to hold cursor->index.

Not so -- note the cursor->array test, and how the array tracks its cursors and updates their index when deletion below the index requires it.

(In reply to comment #6)
> I hope it is not too late to get this into 1.5.0.5 even if the patch inevitably
> changes public API in SpiderMonkey.

JSENUMERATE_MARK is a pure extension, a no-op by default in properly written JSNewEnumerateOp implementations, so this should be ok.  I won't have time to do a 1.8.0 branch patch for a bit, since I'm still on vacation (in spite of patching this bug :-/), so if you or mrbkap would do it, that would be much appreciated.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: patch I'm checking in

Comment 9

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 10

[Igor Bukanov]

(In reply to comment #7)
> (In reply to comment #5)
...
> > patch can also remove cursor array allocation in xml_enumerateValues as it is
> > used only to hold cursor->index.
> 
> Not so -- note the cursor->array test, and how the array tracks its cursors and
> updates their index when deletion below the index requires it.

Here is code in question:

      case JSENUMERATE_NEXT:
        cursor = JSVAL_TO_PRIVATE(*statep);
        if (cursor && cursor->array && (index = cursor->index) < length) {
            kid = XMLARRAY_MEMBER(&xml->xml_kids, index, JSXML);
            kidobj = js_GetXMLObject(cx, kid);
            if (!kidobj)
                return JS_FALSE;
            JS_ASSERT(INT_FITS_IN_JSVAL(index));
            *idp = INT_TO_JSID(index);
            *vp = OBJECT_TO_JSVAL(kidobj);
            cursor->index = index + 1;
            break;
        }
        /* FALL THROUGH */

      case JSENUMERATE_DESTROY:

Here only cursor->index is updated and test for cursor->array can be replaced by not allocating cursor if cursor->array is null during initialization.  

> 
> (In reply to comment #6)
> > I hope it is not too late to get this into 1.5.0.5 even if the patch inevitably
> > changes public API in SpiderMonkey.
> 
> JSENUMERATE_MARK is a pure extension, a no-op by default in properly written
> JSNewEnumerateOp implementations, so this should be ok.

This would not be binary compatible since the function takes enum itself. Thus given
 
    switch (enum_op) {
      case JSENUMERATE_INIT:
         ...
      case JSENUMERATE_NEXT:
         ...
      case JSENUMERATE_DESTROY:
         ...
    }

the compiler is allowed to optimize that when it compiles against the old definition as 
if (enum_op == JSENUMERATE_INIT) {
...
} else if (enum_op == JSENUMERATE_NEXT) {
...
} else {
....
}

since the enum argument promises that the values would be exactly from the enum. For this reason I do not think that it is a good idea to use enum as argumnet in API and not just int if one want to extend the interface in future. 

Moreover, I do not see why it would be improper to code that switch as:
    switch (enum_op) {
      case JSENUMERATE_INIT:
         ...
      case JSENUMERATE_NEXT:
         ...
      default:
         JS_ASSERT(JSENUMERATE_DESTROY == enum_op);
         ...
    }

Since in this case if anybody is using API, then they want to implement JSENUMERATE_MARK to fix potential security bug. 

>  I won't have time to
> do a 1.8.0 branch patch for a bit, since I'm still on vacation (in spite of
> patching this bug :-/), so if you or mrbkap would do it, that would be much
> appreciated.

I will back port it.

Comment 11

[Igor Bukanov]

Attachment: Patch for 1.8.1 and 1.8.0 branches

Comment 12

[Igor Bukanov]

Comment on attachment 226046 [details] [diff] [review]
Patch for 1.8.1 and 1.8.0 branches

The patch applies to MOZILLA_1_8_0_BRANCH as is.

Comment 13

[Igor Bukanov]

The same bug exists on 1.7.* branches.

Comment 14

[Igor Bukanov]

The committed patch is not enough since it does not fix xpcconnect/src/xpccomponents.c.
In fact the code there explicitly demonstrates why extending the enum break
things:

switch(enum_op)
{
...
        case JSENUMERATE_DESTROY:
        default:

That would trigger  JSENUMERATE_DESTROY on any GC inside for-in loop and crash
on NULL pointer  in on NEXT.

Comment 15

[Igor Bukanov]

Comment on attachment 226046 [details] [diff] [review]
Patch for 1.8.1 and 1.8.0 branches

The patch should be extended to fix all users of the enumeration API.

Comment 16

[Brendan Eich [:brendan]]

> For this reason I do not think that it is a good idea to use enum as
> argumnet in API and not just int if one want to extend the interface in future. 

When fur long ago created the JSNewEnumerateOp API, which included this enum, I bet he did not expect anyone to extend it.  It's pretty "just so", e.g. the way the NEXT case can fall through into DESTROY to auto-destruct on last item.

Igor, could you please take this bug off my list?  Thanks,

/be

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > patch can also remove cursor array allocation in xml_enumerateValues as it is
> > > used only to hold cursor->index.
> > 
> > Not so -- note the cursor->array test, and how the array tracks its cursors and
> > updates their index when deletion below the index requires it.
> 
> Here is code in question:

(Note that this JSENUMERATE_NEXT case runs interleaved with many other virtual machine operations, some of which can affect the object being iterated.)
 
>       case JSENUMERATE_NEXT:
>         cursor = JSVAL_TO_PRIVATE(*statep);
>         if (cursor && cursor->array && (index = cursor->index) < length) {
>             kid = XMLARRAY_MEMBER(&xml->xml_kids, index, JSXML);
>             kidobj = js_GetXMLObject(cx, kid);
>             if (!kidobj)
>                 return JS_FALSE;
>             JS_ASSERT(INT_FITS_IN_JSVAL(index));
>             *idp = INT_TO_JSID(index);
>             *vp = OBJECT_TO_JSVAL(kidobj);
>             cursor->index = index + 1;
>             break;
>         }
>         /* FALL THROUGH */
> 
>       case JSENUMERATE_DESTROY:
> 
> Here only cursor->index is updated and test for cursor->array can be replaced
> by not allocating cursor if cursor->array is null during initialization.  

No, because the for-in or for-each-in loop's body could delete children of the XML list, and ECMA specifies that properties deleted after the loop begins must not be enumerated, and that properties above such deleted elements must not be enumerated twice.

This requires decrementing cursor->index when an element is deleted from cursor->array at any index less than the current value of cursor->index.

Furthermore, the "primitiveAssign" logic in PutProperty (ECMA-357 9.1.1.2) may call XMLArrayFinish on kid->xml_kids, which will call XMLArrayCursorFinish on all live cursors for that array.

While kid in such a case should never be an XMLList, the xml_enumerate* hooks were future-proofed against other ways that an XMLArray might be finished before cursors iterating it are finished.  Thus the cursor->array test in the NEXT case.

/be

Comment 18

[Igor Bukanov]

I would like to suggest an alternative solution for this that does not extend the enum. Since the only case in Mozilla's CVS for JSENUMERATE_MARK comes from js_Enumerate in jsobj.c, a simple solution is to form a list from all currently active JSNativeIteratorState and teach js_GC to mark the list starting from the head stored in JSContext. If anybody else would need to root the state, they can always use AddRoot/LockGCThing and friends.

Note also that such linked list is not thread-safe with CVS trunk as the new iterator protocol allows to exposes the state to other threads. But since access to state is not thread-safe in any case, this would be a good enough solution even for CVS trunk pending resolving thread-safty issues for iterator state in general.   

Is it OK?

Comment 19

[Igor Bukanov]

Attachment: Fix without changing public API

This is an implementation of the above idea.

Comment 20

[Igor Bukanov]

Here is a better test case that tries to make sure that memory for atoms freed during GC would be overwritten.

var obj = { };

var prop = "xsomePropety".substr(1);

obj.first = "first"

obj[prop] = 1;

for (var elem in obj) {
        var tmp = elem.toString(); 
        delete obj[prop];
        // ensure that prop is cut from all roots
        prop = "xsomePropety".substr(2);
        obj[prop] = 2;
        delete obj[prop];
        prop = null;
        if (typeof gc == 'function')
                gc();
        for (var i = 0; i != 50000; ++i) {
                var tmp = 1 / 3;
                tmp /= 10;
        }
        for (var i = 0; i != 1000; ++i) {
                // Make string with 11 characters that would take
                // (11 + 1) * 2 bytes or sizeof(JSAtom) so eventually
                // malloc will ovewrite just freed atoms. 
                var tmp2 = Array(12).join(' ');
	}
}

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #18)
> Is it OK?

Sure, I was generalizing, your particular solution is sufficient.  It's hard to generalize and keep API compatibility.

/be

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 226074 [details] [diff] [review]
Fix without changing public API

>+
>+    /* Head for the list of native itearor states to do the marking. */
>+    JSNativeIteratorState       *nativeIteratorStateList;

Names for the list of states include ...StateList (here) and ...States (the function) -- shorten this to match the function and use States consistently.

> /* Private type used to iterate over all properties of a native JS object */
>-typedef struct JSNativeIteratorState {
>+struct JSNativeIteratorState {
>     jsint next_index;   /* index into jsid array */
>     JSIdArray *ida;     /* all property ids in enumeration */
>-} JSNativeIteratorState;
>+    JSNativeIteratorState *prev, *next; /* double-linked list support */
>+
>+};

Prevailing style starts member declarators in same column, usually puts prev and next or equivalent each on its own line.  See below for a slightly leaner single-head doubly-linked list idea.

>+        state->next = cx->nativeIteratorStateList;
>+        state->prev = NULL;
>+        cx->nativeIteratorStateList = state;

Bug: what sets state->next->prev = state?

Alternative using double-indirection to avoid if/else in DESTROY case: use JSNativeIteratorState *next; JSNativeIteratorState **prevp; fields and do

        state->next = cx->nativeIteratorStates;
        if (state->next)
            state->next->prevp = &state->next;
        state->prevp = &cx->nativeIteratorStates;
        *state->prevp = state;

>       case JSENUMERATE_DESTROY:
>         state = (JSNativeIteratorState *) JSVAL_TO_PRIVATE(*statep);
>+        JS_ASSERT(cx->nativeIteratorStateList);
>+        if (!state->prev) {
>+            JS_ASSERT(state == cx->nativeIteratorStateList);
>+            cx->nativeIteratorStateList = state->next;
>+        } else {
>+            JS_ASSERT(state != cx->nativeIteratorStateList);
>+            state->prev->next = state->next;
>+        }
>+        if (state->next)
>+            state->next->prev = state->prev;

This then becomes

        *state->prevp = state->next;

>+void
>+js_MarkNativeIteratorStates(JSContext *cx, JSNativeIteratorState *listHead)

Call listHead state, it reflects its use in the function better.  There's not much gain in documentation in naming it to tell the single caller what to pass, IMHO.

>+/*
>+ * js_Enumerate uses this to hold its state.
>+ */
>+typedef struct JSNativeIteratorState JSNativeIteratorState;

Traditionally this would go in jsprvtd.h, but if you don't need to use th typename in any .c that doesn't already include jsobj.h, then cool.

/be

Comment 23

[Brendan Eich [:brendan]]

(In reply to comment #22)
> This then becomes
> 
>         *state->prevp = state->next;

But of course I left off the

        if (state->next)
            state->next->prevp = state->prevp;

> >+/*
> >+ * js_Enumerate uses this to hold its state.
> >+ */
> >+typedef struct JSNativeIteratorState JSNativeIteratorState;
> 
> Traditionally this would go in jsprvtd.h, but if you don't need to use th
> typename in any .c that doesn't already include jsobj.h, then cool.

But of course the struct is in jsobj.c, so putting its opaque typedef in jsobj.h is the right thing!  Nm,

/be

Comment 24

[Igor Bukanov]

(In reply to comment #22)
> >+        state->next = cx->nativeIteratorStateList;
> >+        state->prev = NULL;
> >+        cx->nativeIteratorStateList = state;
> 
> Bug: what sets state->next->prev = state?

This is what happens when hacking at night.

> 
> Alternative using double-indirection to avoid if/else in DESTROY case: use
> JSNativeIteratorState *next; JSNativeIteratorState **prevp;

And this what happens when one programs in Java too much and forgets the power of C pointers :)

Comment 25

[Igor Bukanov]

Attachment: Fix without changing public API v2

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 226141 [details] [diff] [review]
Fix without changing public API v2

>+    /* Head for the list of native itearor states to do the marking. */

Typo, and how about rewording to say "Native iterator state list, needed for marking id arrays."?  Or something like that.

>+        state = state->next;
>+    } while (state);

Prevailing style prefers (this is the one place you see explicit NULL tests) to nest the loop variable update in the do-while loop's condition.

>+/*
>+ * js_Enumerate uses this to hold its state.
>+ */
>+typedef struct JSNativeIteratorState JSNativeIteratorState;

"this opaque structure", just to avoid pronoun-itis.

r=me with these minor nits picked.  Thanks,

/be

Comment 27

[Igor Bukanov]

Attachment: Fix without changing public API v2 with nits addressed.

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 226174 [details] [diff] [review]
Fix without changing public API v2 with nits addressed.

Great -- thanks for doing this and keeping the API compatible.

/be

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 226174 [details] [diff] [review]
Fix without changing public API v2 with nits addressed.

Brendan, is this patch OK for the 1.8 branch, and baked enough to take in 1.8.0.5?

Comment 30

[Daniel Veditz [:dveditz]]

tentatively marking blocking 1.8.0.5, but would like some beating on this when it's checked into the 1.8 branch before we'll take it.

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 226174 [details] [diff] [review]
Fix without changing public API v2 with nits addressed.

The trunk SpiderMonkey is being stabilized as JS1.7 and will be included wholesale (without any API changes -- one fix remains in that department) for 1.8.1/fx2.  So this doesn't need a branch nomination, but I'm plussing anyway.

The patch is straightforward and can be taken on the 1.8.0 branch after a day or two's tinderboxing on the trunk.  It could be taken right away, but unless you are about to cut off 1.8.0.5 changes I don't see a need to wave the usual baking rule.

/be

Comment 32

[Brendan Eich [:brendan]]

Comment on attachment 226174 [details] [diff] [review]
Fix without changing public API v2 with nits addressed.

>--- .pc/fix341877.diff/js/src/jscntxt.h	2006-06-19 16:09:33.000000000 +0200
>+++ js/src/jscntxt.h	2006-06-19 19:32:18.000000000 +0200
>@@ -644,16 +644,19 @@ struct JSContext {
> 
>     /* Iterator cache to speed up native default for-in loop case. */
>     JSObject            *cachedIterObj;
> 
> #ifdef GC_MARK_DEBUG
>     /* Top of the GC mark stack. */
>     void                *gcCurrentMarkNode;
> #endif
>+
>+    /* List of native iterator states, needed for marking id arrays. */
>+    JSNativeIteratorState       *nativeIteratorStates;
> };
> 
> #define JS_THREAD_ID(cx)            ((cx)->thread ? (cx)->thread->id : 0)

Late-breaking ultra-nit: instead of tabbing *nativeIteratorStates over a full stop (or whatever), just use one space between type and declarator.  Otherwise any future members may end up over-"indented" -- also, nothing really lines up with anything (the body of the JS_THREAD_ID macro starts even further over).

/be

Comment 33

[Bob Clary [:bc] (inactive)]

(In reply to comment #30)
> tentatively marking blocking 1.8.0.5, but would like some beating on this when
> it's checked into the 1.8 branch before we'll take it.
> 

FWIW, I am unsure of the state of 1.8.0 or trunk. A recent change to event handling on the trunk prevented testing over the weekend. I also saw very strange results on Linux on 1.8.0.5 when running the JS tests. My gut feeling is we are not in good shape.

Comment 34

[Igor Bukanov]

Attachment: Fix without changing public API v2 with even more nitlessing

This is a patch to commit where I picked up that suggestion to avoid over indenting is jscntxt.c while waiting for the tidenderbox to be green again.

Comment 35

[Igor Bukanov]

I committed the patch from comment 34 to the trunk.

Comment 36

[Igor Bukanov]

Attachment: Fix for 1.8.1 and 1.8.0 branches

Comment 37

[Igor Bukanov]

Comment on attachment 226235 [details] [diff] [review]
Fix for 1.8.1 and 1.8.0 branches

The patch applies as is to both MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

With fresh trunk build, I am crashing on startup (loading homepage www.speigel.de) as follows:

Assertion failure: cx->nativeIteratorStates, at ../../../mozilla/js/src/jsobj.c:3802
../obj-ff-debug-cairo/dist/bin/run-mozilla.sh: line 131: 29321 Aborted         "$prog" ${1+"$@"}

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backing out this patch fixes it. Clearly it's not affecting tinderboxes, though, so I'll file a new bug for the regression.

Comment 40

[Igor Bukanov]

(In reply to comment #38)
> With fresh trunk build, I am crashing on startup (loading homepage
> www.speigel.de) as follows:
> 
> Assertion failure: cx->nativeIteratorStates, at
> ../../../mozilla/js/src/jsobj.c:3802
> ../obj-ff-debug-cairo/dist/bin/run-mozilla.sh: line 131: 29321 Aborted        
> "$prog" ${1+"$@"}
> 

I really should read a book about double-list implementation. Patch in a minute. Please do not file another bug!

Comment 41

[Igor Bukanov]

Attachment: Removal of bogus assert

The assert was bogus - see comments in the patch.

Comment 42

[Igor Bukanov]

That (In reply to comment #41)
> The assert was bogus - see comments in the patch.

Actually the assert was OK since it reflected the patch logic that assumed that context would live longer then the iterator. But this is not the case when  JSENUMERATE_DESTROY would be executed during GC on another context and even thread. So the patch is broken and I will take it out for now. 

Comment 43

[Brendan Eich [:brendan]]

It's ok for JSENUMERATE_DESTROY to run on another thread, provided the iterator is true garbage.  So is there more to do than adjust or remove the bogus assertion?

Sorry I didn't catch this -- still on vacation (except when online).

/be

Comment 44

[Igor Bukanov]

(In reply to comment #43)
> It's ok for JSENUMERATE_DESTROY to run on another thread, provided the iterator
> is true garbage.  So is there more to do than adjust or remove the bogus
> assertion?

Yes: the context serving as list head can be deleted at that point.

Comment 45

[Brendan Eich [:brendan]]

(In reply to comment #44)
> (In reply to comment #43)
> > It's ok for JSENUMERATE_DESTROY to run on another thread, provided the iterator
> > is true garbage.  So is there more to do than adjust or remove the bogus
> > assertion?
> 
> Yes: the context serving as list head can be deleted at that point.

Duh, I kept mentally switching the list to live in the runtime.  Why not put it there, at the cost of JS_LOCK_RUNTIME?

/be

Comment 46

[Igor Bukanov]

I reverted the commit with patch from comment 34 for now.

Comment 47

[Igor Bukanov]

(In reply to comment #45)
> Duh, I kept mentally switching the list to live in the runtime.  Why not put it
> there, at the cost of JS_LOCK_RUNTIME?

I will do something like that tomorrow.

Comment 48

[Igor Bukanov]

Attachment: Fix without changing public API v3

The new patch takes a different aproach to address the problem. It just detects in iter_mark when the state is an instance of JSNativeIteratorState and mark its properties.

Comment 49

[Igor Bukanov]

Comment on attachment 226290 [details] [diff] [review]
Fix without changing public API v3

That detection that the state object is an instance of JSNativeIteratorState does not work in general as an embedding can access js_Enumerate and use for own purposes throw the map operations. So a global list and locking seems to be the only solution. Price to pay for compatibility...

Comment 50

[Igor Bukanov]

Attachment: Fix without changing public API v4

This is essentially the patch from comment 34.

Comment 51

[Igor Bukanov]

(In reply to comment #50)
> This is essentially the patch from comment 34.

I meant that this was essentially the patch from comment 34 with per-context list replaced by per-runtime one protected with the runtime lock.

Comment 52

[Brendan Eich [:brendan]]

Comment on attachment 226298 [details] [diff] [review]
Fix without changing public API v4

>+    /*
>+     * A helper list to GC-mark native iterator states. See
>+     * js_MarkNativeIteratorStates for details.
>+     */
>+    JSNativeIteratorState *nativeIteratorStates;

Suggestion: "A helper list for the GC, so it can mark native iterator states."

r=me, thanks.

/be

Comment 53

[Igor Bukanov]

Attachment: Fix without changing public API v4b (better comments)

This is the patch I will commit.

Comment 54

[Igor Bukanov]

I committed to the trunk the patch from comment 53.

Comment 55

[Igor Bukanov]

Attachment: 1.8.0 and 1.8.1 version of fix without changing public API v4b

This is a version of the fix for MOZILLA_1_8_BRANCH and  MOZILLA_1_8_0_BRANCH

Comment 56

[Daniel Veditz [:dveditz]]

Comment on attachment 226484 [details] [diff] [review]
1.8.0 and 1.8.1 version of fix without changing public API v4b

approved for 1.8.0 branch, a=dveditz for drivers

Comment 57

[Igor Bukanov]

I committed the patch from comment 55 to MOZILLA_1_8_BRANCH.

Comment 58

[Igor Bukanov]

I committed the patch from comment 55 to MOZILLA_1_8_0_BRANCH.

Comment 59

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/GC/regress-341877-01.js

Comment 60

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/GC/regress-341877-02.js

Comment 61

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0.5, 1.8.1, 1.9a1 20060622 builds on all platforms

Comment 62

[Alexander Sack]

Attachment: 1.0.x patch

Comment 63

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/GC/regress-341877-01.js,v  <--  regress-341877-01.js

/cvsroot/mozilla/js/tests/js1_5/GC/regress-341877-02.js,v  <--  regress-341877-02.js