283532 - OOM crash [@ jsdValue::GetProperties]

Status: RESOLVED FIXED

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

Description

[timeless]

handle alloc failure

Comment 1

[Bastiaan Jacques]

Attachment: fix

Comment 2

[:Gijs (he/him)]

Comment on attachment 187657 [details] [diff] [review]
fix

Asking for review to get the crash out of the way :-)

Comment 3

[Robert Ginda]

Comment on attachment 187657 [details] [diff] [review]
fix

r=rginda

Comment 4

[Bastiaan Jacques]

(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.

Comment 5

[Bastiaan Jacques]

Attachment: adds nulling out of out arg

Comment 6

[Robert Ginda]

Comment on attachment 187733 [details] [diff] [review]
adds nulling out of out arg

r=rginda

Comment 7

[Brendan Eich [:brendan]]

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

Comment 8

[Bastiaan Jacques]

Attachment: address comment

Comment 9

[Bastiaan Jacques]

Attachment: make nice

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 187841 [details] [diff] [review]
make nice

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

/be

Comment 11

[Bastiaan Jacques]

Checked in by timeless (2005-06-30 22:12).