97555 - XPC_WN_JSOp_Enumerate needs to set *statep in failure case

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[John Bandhauer]

We crash if an enumerate hook returns failure during JSENUMERATE_INIT. We are
supposed to set *statep = JSVAL_NULL and we don't. Since we don't do that the JS
engine later crashes when finalizing the PropertyIterator object. That finalizer
will do the right thing if we set *statep = JSVAL_NULL. Let's do that!

See...
http://lxr.mozilla.org/seamonkey/ident?i=prop_iterator_finalize
http://lxr.mozilla.org/seamonkey/ident?i=XPC_WN_JSOp_Enumerate

Patch coming up.

Comment 1

[John Bandhauer]

Attachment: proposed fix

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Doesn't seem to work. After the callback to nsDOMClassInfo sets retval to false,
we set statep to JSVAL_NULL, but for some reason when we hit this function again
for JSENUMERATE_DESTROY (on GC), statep is no longer null; it's a bogus value
which causes a crash just as before. My bug is 59208, I'll post my current patch
there.

Comment 3

[John Bandhauer]

OK. We're uncovered a bug in jsinterp.c

That code has two places where it calls OBJ_ENUMERATE passing JSENUMERATE_INIT - 
one for the initial object and one that is used for each object up the proto 
chain. Way back whenever we fixed one of these places to do...

  propobj->slots[JSSLOT_ITER_STATE] = iter_state

...*before* checking 'ok' and possibly terminating the iteration. This is done 
to ensure that propobj is in a valid state for its eventiual finalization. 
Unfortunately, it looks like we fixed only one of these two places. We need to 
fix the other loaction. I also see that when that one line was moved the 
associated comment was not moved with it.

Patch coming. This fixes the quicky test cases I hacked.

Comment 4

[John Bandhauer]

Attachment: fix for jsinterp.c - both this and previous patch needed

Comment 5

[Johnny Stenback (:jst)]

r=jst

Comment 6

[Mitchell Stoltz (not reading bugmail)]

This fixes my testcase as well, so r=mstoltz too. What is required to make a
change to the JS engine? Can this be checked in on the trunk? Does someone need
to OK it? I'd like to get this in today if possible.

Comment 7

[Brendan Eich [:brendan]]

jband, all I can do is pick nits:

First instance:
                  * Stash private iteration state into property iterator object.

Second instance:
                 ok = OBJ_ENUMERATE(cx, obj, JSENUMERATE_INIT, &iter_state, 0);

+                /*
+                 * Stash private iteration state into iterator JSObject.

Nit #1: wahhh, gratuitous lack of copy-paste in second comment instance.
Nit #2: trailing spaces, at least on the /* open comment line shown above.
Nit #3: prevailing style usually puts an extra newline before a major comment,
unless the token before the comment is {.

Fix those, and sr=[e-mail].

/be

Comment 8

[John Bandhauer]

Nits fixed. Thanks. Sending approval request...

Comment 9

[John Bandhauer]

BTW, I *did* run the JS test suite. The same tests pass/fail with and without 
these changes.

Comment 10

[Asa Dotzler [:asa]]

a=asa on behalf of drivers.

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 47602 [details] [diff] [review]
proposed fix

a=asa on behalf of drivers for both patches

Comment 12

[John Bandhauer]

Both patches checked in. Thanks.

Comment 13

[Phil Schwartau]

Marking Verified -