Closed Bug 283532 Opened 17 years ago Closed 17 years ago

OOM crash [@ jsdValue::GetProperties]

Categories

(Other Applications Graveyard :: Venkman JS Debugger, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: timeless, Assigned: bastiaan)

References

()

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 3 obsolete files)

handle alloc failure
Attached patch fix (obsolete) — Splinter Review
Assignee: timeless → b.jacques
Status: NEW → ASSIGNED
Comment on attachment 187657 [details] [diff] [review]
fix

Asking for review to get the crash out of the way :-)
Attachment #187657 - Flags: review?(rginda)
Comment on attachment 187657 [details] [diff] [review]
fix

r=rginda
Attachment #187657 - Flags: review?(rginda) → review+
(In reply to comment #2)
> (From update of attachment 187657 [details] [diff] [review] [edit])
> Asking for review to get the crash out of the way :-)
Did you actually experience a crash with this codepath? This bug is about a
crash that _could_ happen, not one that we can reproduce.

Please don't set review flags on my patches without asking me first. I will
request review when I think my patch is ready.


Attached patch adds nulling out of out arg (obsolete) — Splinter Review
Attachment #187657 - Attachment is obsolete: true
Attachment #187733 - Flags: superreview?(brendan)
Attachment #187733 - Flags: review?(rginda)
Comment on attachment 187733 [details] [diff] [review]
adds nulling out of out arg

r=rginda
Attachment #187733 - Flags: review?(rginda) → review+
Comment on attachment 187733 [details] [diff] [review]
adds nulling out of out arg

>Index: js/jsd/jsd_xpc.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/js/jsd/jsd_xpc.cpp,v
>retrieving revision 1.70
>diff -p -8 -u -r1.70 jsd_xpc.cpp
>--- js/jsd/jsd_xpc.cpp	10 Nov 2004 21:58:00 -0000	1.70
>+++ js/jsd/jsd_xpc.cpp	30 Jun 2005 01:04:32 -0000
>@@ -2164,30 +2164,33 @@ jsdValue::GetPropertyCount (PRInt32 *_rv
>         *_rval = -1;
>     return NS_OK;
> }
> 
> NS_IMETHODIMP
> jsdValue::GetProperties (jsdIProperty ***propArray, PRUint32 *length)
> {
>     ASSERT_VALID_EPHEMERAL;
>+    *propArray = nsnull;
>+

Why add this here...

>     PRUint32 prop_count = JSD_IsValueObject(mCx, mValue)
>         ? JSD_GetCountOfProperties (mCx, mValue)
>         : 0;
>     if (!prop_count) {
>         if (length)
>             *length = 0;
>         *propArray = 0;

... when the same (but with 0, not nsnull -- style nit) is done here, just
before the only early return in the method?

>         return NS_OK;
>     }
> 
>     jsdIProperty **pa_temp =
>         NS_STATIC_CAST(jsdIProperty **,
>                        nsMemory::Alloc(sizeof (jsdIProperty *) * 
>                                        prop_count));
>+    NS_ENSURE_TRUE(pa_temp, NS_ERROR_OUT_OF_MEMORY);
> 
>     PRUint32     i    = 0;
>     JSDProperty *iter = NULL;
>     JSDProperty *prop;
>     while ((prop = JSD_IterateProperties (mCx, mValue, &iter))) {
>         pa_temp[i] = jsdProperty::FromPtr (mCx, prop);
>         ++i;
>     }

The rest of the method after this context sets *propArray = pa_temp; followed
by straight-line code to the final return.

The NS_ENSURE_TRUE change is good, but if you want to set *propArray = nsnull
first thing, then take out the *propArray = 0; assignment in the early return
case.

/be
Attachment #187733 - Flags: superreview?(brendan) → superreview-
Attached patch address comment (obsolete) — Splinter Review
Attachment #187733 - Attachment is obsolete: true
Attached patch make niceSplinter Review
Attachment #187839 - Attachment is obsolete: true
Attachment #187841 - Flags: superreview?(brendan)
Comment on attachment 187841 [details] [diff] [review]
make nice

sr+a=me, noting rginda's r+ stamp.

/be
Attachment #187841 - Flags: superreview?(brendan)
Attachment #187841 - Flags: superreview+
Attachment #187841 - Flags: review+
Attachment #187841 - Flags: approval1.8b3+
Checked in by timeless (2005-06-30 22:12).
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Crash Signature: [@ jsdValue::GetProperties]
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.