Closed
Bug 97555
Opened 23 years ago
Closed 23 years ago
XPC_WN_JSOp_Enumerate needs to set *statep in failure case
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
VERIFIED
FIXED
People
(Reporter: jband_mozilla, Assigned: jband_mozilla)
References
Details
(Keywords: crash)
Attachments
(2 files)
1.40 KB,
patch
|
asa
:
approval+
|
Details | Diff | Splinter Review |
2.16 KB,
patch
|
asa
:
approval+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•23 years ago
|
||
Comment 2•23 years ago
|
||
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.
Assignee | ||
Comment 3•23 years ago
|
||
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.
Assignee | ||
Comment 4•23 years ago
|
||
Comment 5•23 years ago
|
||
r=jst
Comment 6•23 years ago
|
||
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•23 years ago
|
||
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=brendan@mozilla.org.
/be
Assignee | ||
Comment 8•23 years ago
|
||
Nits fixed. Thanks. Sending approval request...
Assignee | ||
Comment 9•23 years ago
|
||
BTW, I *did* run the JS test suite. The same tests pass/fail with and without
these changes.
Comment 10•23 years ago
|
||
a=asa on behalf of drivers.
Comment 11•23 years ago
|
||
Comment on attachment 47602 [details] [diff] [review]
proposed fix
a=asa on behalf of drivers for both patches
Attachment #47602 -
Flags: approval+
Updated•23 years ago
|
Attachment #47617 -
Flags: approval+
Assignee | ||
Comment 12•23 years ago
|
||
Both patches checked in. Thanks.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•