299738 - jsarray.c fills holes in arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Recently, we've been playing whack-a-mole with bugs in jsarray.c where the
various functions plug "holes" in the arrays they are working on (e.g.,
array_sort() and array_concat() both plug(ged) holes).

I'm going to go through and fix these plugging problems.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 188370 [details] [diff] [review]
patch v1

Bug: don't return after a successful OBJ_LOOKUP_PROPERTY that returned non-null
prop without OBJ_DROP_PROPERTY.

In particular, in array_reverse with JS_THREADSAFE defined, OBJ_LOOKUP_PROPERTY
will lock the object in which it finds the given id, so looking up the far id
will double-lock (which is handled, and optimized almost away in the usual
case, but still...).  It should be cleaner to encapsulate a HasElement
subroutine helper, although I haven't tried it.  Point is that you don't need
to hold the object lock, or to hold onto prop (if obj were non-native and it
did something like refcount its JSProperty subtype).

Nit: "fill" is better than "plug" for hole erasing, IMHO.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

Yeah, the helper cleans that function up a bunch, so I just used it everywhere.
It's a bit uglier than I like because OBJ_LOOKUP_PROP can fail, but there isn't
much I can do about that.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 188381 [details] [diff] [review]
patch v2

>+static JSBool
>+PropertyExists(JSContext *cx, JSObject *obj, jsid id, JSBool *ret)
>+{
>+    JSObject *obj2;
>+    JSProperty *prop;
>+
>+    *ret = JS_FALSE;

The usual name for such an out param is *foundp, and you could avoid storing
twice in the likely (exists) case by setting *foundp = prop != NULL; below,
then testing if (*foundp) instead of if (prop) to decide to OBJ_DROP_PROPERTY.

>+
>+    if (!OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop))
>+        return JS_FALSE;
>+
>+    if (prop) {
>+        *ret = JS_TRUE;
>+        OBJ_DROP_PROPERTY(cx, obj2, prop);
>+    }
>+
>+    return JS_TRUE;
>+}
>-#if JS_HAS_SPARSE_ARRAYS
>-        /* This part isn't done yet. */
>-
>-        if (!OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop))
>+        /* Check for holes to make sure they don't get filled. */
>+        if (!PropertyExists(cx, obj, id, &idexists) ||
>+            !PropertyExists(cx, obj, id2, &id2exists))
>             return JS_FALSE;

House style says to over-brace the one-line then clause here, because the if
condition is multiline.  In general, over-brace if anything looks multiline
(even if only due to a comment) in the condition, the then, or the else clause
-- and brace the other clause even if single-line.

>+        if (idexists) {
>+            if (!OBJ_GET_PROPERTY(cx, obj, id, &v) ||
>+                !OBJ_SET_PROPERTY(cx, obj, id2, &v))
>+                return JS_FALSE;

Ditto, and again a bit further.

>+
>+        ca.status = PropertyExists(cx, obj, id, &propExists);

Just a thought -- call this variable idexists or just exists, to match above
(exists/exists2, there, if you want to be completely consistent ;-).

Looks good for 1.8b4, r=me with nits picked.  Thanks,

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 188381 [details] [diff] [review]
patch v2

This is a well-understood patch that increases ECMA compliance.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 7

[Brendan Eich [:brendan]]

This was fixed on the trunk well before 1.8 branched.

/be

Comment 8

[Brendan Eich [:brendan]]

Asa, sorry, was I wrong to add fixed1.8?  I wasn't sure why you changed the
blocking1.8b5+ into blocking1.8b4+ given that this was fixed before 1.8 branched.

/be

Comment 9

[Asa Dotzler [:asa]]

yeah, mrbkap was trying to get his buglist clean and this was one of the bugs
resolved before we branched and flag-mangled by a flag change I made a while
back (fixed1.8, which signals that it landed on the 1.8 branch, isn't quite
correct).  The flag being previously set as 1.8b5+ is the result of a flag
shuffle we did whern we added another beta to the release. It's a bit of a mess,
most of which I'm going to leave alone, but for thse four bugs I'm just trying
to clean up some because mrbkap asked me to help him.

Comment 10

[Bob Clary [:bc] (inactive)]

we have other hole tests. if you have any additional tests, please attach here and set in-testsuite?