GC hazard with for-in loop

VERIFIED FIXED

Status

()

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

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.5, verified1.8.1})

Trunk
verified1.8.0.5, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.0.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(6 attachments, 12 obsolete attachments)

548 bytes, text/html
Details
11.88 KB, patch
Details | Diff | Splinter Review
7.33 KB, patch
mconnor
: approval1.8.1+
Details | Diff | Splinter Review
2.44 KB, text/plain
Details
2.67 KB, text/plain
Details
7.02 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
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;
	}
}
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
Created attachment 226006 [details] [diff] [review]
possible fix
Attachment #226006 - Flags: review?(igor.bukanov)
(Assignee)

Comment 3

11 years ago
Created attachment 226008 [details]
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.
(Assignee)

Updated

11 years ago
Severity: normal → critical
Created attachment 226010 [details] [diff] [review]
better fix
Attachment #226006 - Attachment is obsolete: true
Attachment #226010 - Flags: review?(igor.bukanov)
Attachment #226006 - Flags: review?(igor.bukanov)
(Assignee)

Comment 5

11 years ago
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.
Attachment #226006 - Attachment is obsolete: false
(Assignee)

Updated

11 years ago
Attachment #226010 - Flags: review?(igor.bukanov) → review+
(Assignee)

Comment 6

11 years ago
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.
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?
(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
Created attachment 226032 [details] [diff] [review]
patch I'm checking in
Attachment #226006 - Attachment is obsolete: true
Attachment #226010 - Attachment is obsolete: true
Attachment #226032 - Flags: review+

Updated

11 years ago
Assignee: general → brendan
Fixed.

/be
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

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

Comment 11

11 years ago
Created attachment 226046 [details] [diff] [review]
Patch for 1.8.1 and 1.8.0 branches
Attachment #226046 - Flags: review?(brendan)
Attachment #226046 - Flags: approval-branch-1.8.1?(brendan)
(Assignee)

Comment 12

11 years ago
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.
Attachment #226046 - Attachment description: Patch for 1.8.1 branch → Patch for 1.8.1 and 1.8.0 branches
Attachment #226046 - Flags: approval1.8.0.5?
(Assignee)

Comment 13

11 years ago
The same bug exists on 1.7.* branches.
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
(Assignee)

Comment 14

11 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

11 years ago
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.
Attachment #226046 - Flags: review?(brendan)
Attachment #226046 - Flags: approval1.8.0.5?
Attachment #226046 - Flags: approval-branch-1.8.1?(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
(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
(Assignee)

Updated

11 years ago
Assignee: brendan → igor.bukanov
Status: REOPENED → NEW
(Assignee)

Comment 18

11 years ago
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?
Whiteboard: [sg:critical?]
(Assignee)

Comment 19

11 years ago
Created attachment 226074 [details] [diff] [review]
Fix without changing public API

This is an implementation of the above idea.
Attachment #226074 - Flags: review?(brendan)
(Assignee)

Comment 20

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

Comment 24

11 years ago
(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 :)
(Assignee)

Comment 25

11 years ago
Created attachment 226141 [details] [diff] [review]
Fix without changing public API v2
Attachment #226046 - Attachment is obsolete: true
Attachment #226074 - Attachment is obsolete: true
Attachment #226141 - Flags: review?(brendan)
Attachment #226074 - Flags: review?(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
Attachment #226141 - Flags: review?(brendan) → review+
(Assignee)

Comment 27

11 years ago
Created attachment 226174 [details] [diff] [review]
Fix without changing public API v2 with nits addressed.
Attachment #226141 - Attachment is obsolete: true
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
Attachment #226174 - Flags: review+
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?
Attachment #226174 - Flags: approval-branch-1.8.1?(brendan)
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.
Flags: blocking1.8.0.5? → blocking1.8.0.5+
Keywords: qawanted
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
Attachment #226174 - Flags: approval1.8.0.5?
Attachment #226174 - Flags: approval-branch-1.8.1?(brendan)
Attachment #226174 - Flags: approval-branch-1.8.1+
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

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

Comment 34

11 years ago
Created attachment 226204 [details] [diff] [review]
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.
(Assignee)

Comment 35

11 years ago
I committed the patch from comment 34 to the trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 36

11 years ago
Created attachment 226235 [details] [diff] [review]
Fix for 1.8.1 and 1.8.0 branches
Attachment #226235 - Flags: approval-branch-1.8.1?(brendan)
(Assignee)

Updated

11 years ago
Attachment #226174 - Attachment is obsolete: true
Attachment #226174 - Flags: approval1.8.0.5?
(Assignee)

Updated

11 years ago
Attachment #226032 - Attachment is obsolete: true
(Assignee)

Comment 37

11 years ago
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
Attachment #226235 - Attachment description: Fix for 1.8.1 branch → Fix for 1.8.1 and 1.8.0 branches
Attachment #226235 - Flags: approval1.8.0.5?

Updated

11 years ago
Attachment #226235 - Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+
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+"$@"}
Backing out this patch fixes it. Clearly it's not affecting tinderboxes, though, so I'll file a new bug for the regression.
(Assignee)

Comment 40

11 years ago
(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!

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 41

11 years ago
Created attachment 226245 [details] [diff] [review]
Removal of bogus assert

The assert was bogus - see comments in the patch.
Attachment #226245 - Flags: review?(brendan)
Depends on: 342088
(Assignee)

Comment 42

11 years ago
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. 
No longer depends on: 342088
(Assignee)

Updated

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

Updated

11 years ago
Attachment #226235 - Attachment is obsolete: true
Attachment #226235 - Flags: approval1.8.0.5?
(Assignee)

Updated

11 years ago
Attachment #226204 - Attachment is obsolete: true
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
(Assignee)

Comment 44

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

Comment 46

11 years ago
I reverted the commit with patch from comment 34 for now.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 47

11 years ago
(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.

Updated

11 years ago
Blocks: 342088
(Assignee)

Comment 48

11 years ago
Created attachment 226290 [details] [diff] [review]
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.
Attachment #226290 - Flags: review?(brendan)
(Assignee)

Comment 49

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

Comment 50

11 years ago
Created attachment 226298 [details] [diff] [review]
Fix without changing public API v4

This is essentially the patch from comment 34.
Attachment #226298 - Flags: review?(brendan)
(Assignee)

Comment 51

11 years ago
(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 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
Attachment #226298 - Flags: review?(brendan) → review+
(Assignee)

Comment 53

11 years ago
Created attachment 226482 [details] [diff] [review]
Fix without changing public API v4b (better comments)

This is the patch I will commit.
Attachment #226298 - Attachment is obsolete: true
(Assignee)

Comment 54

11 years ago
I committed to the trunk the patch from comment 53.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 55

11 years ago
Created attachment 226484 [details] [diff] [review]
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
Attachment #226484 - Flags: approval1.8.1?
Attachment #226484 - Flags: approval1.8.0.5?
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
Attachment #226484 - Flags: approval1.8.0.5? → approval1.8.0.5+
(Assignee)

Comment 57

11 years ago
I committed the patch from comment 55 to MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1
(Assignee)

Comment 58

11 years ago
I committed the patch from comment 55 to MOZILLA_1_8_0_BRANCH.
Keywords: fixed1.8.0.5

Updated

11 years ago
Attachment #226484 - Flags: approval1.8.1? → approval1.8.1+

Comment 59

11 years ago
Created attachment 226624 [details]
js1_5/GC/regress-341877-01.js

Comment 60

11 years ago
Created attachment 226625 [details]
js1_5/GC/regress-341877-02.js

Updated

11 years ago
Flags: in-testsuite+

Comment 61

11 years ago
verified fixed 1.8.0.5, 1.8.1, 1.9a1 20060622 builds on all platforms
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.5, fixed1.8.1, qawanted → verified1.8.0.5, verified1.8.1

Comment 62

11 years ago
Created attachment 232726 [details] [diff] [review]
1.0.x patch
Group: security

Comment 63

11 years ago
/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

Updated

11 years ago
Flags: blocking1.9a1?
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
(Assignee)

Updated

10 years ago
Flags: blocking1.8.1?
You need to log in before you can comment on or make changes to this bug.