329749 - Did JS_SealObject mean to call JS_UNLOCK_OBJ instead of JS_UNLOCK_SCOPE ?

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[timeless]

Coverity is complaining about dereferencing scope before and after null checks of it. I suspect it's because the wrong creature is being unlocked. But what do I know? :)

Comment 1

[timeless]

Attachment: unlock the object that was locked?

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 214410 [details] [diff] [review]
unlock the object that was locked?

>Index: jsapi.c
>===================================================================
>RCS file: /cvsroot//cvsroot/mozilla/js/src/jsapi.c,v
>retrieving revision 3.248
>diff -pu10 -r3.248 jsapi.c
>--- jsapi.c
>+++ jsapi.c
>@@ -2281,40 +2281,40 @@ JS_SealObject(JSContext *cx, JSObject *o
>     }
> 
>     scope = OBJ_SCOPE(obj);
> 
> #if defined JS_THREADSAFE && defined DEBUG
>     /* Insist on scope being used exclusively by cx's thread. */
>     if (scope->ownercx != cx) {
>         JS_LOCK_OBJ(cx, obj);
>         JS_ASSERT(OBJ_SCOPE(obj) == scope);
>         JS_ASSERT(scope->ownercx == cx);
>-        JS_UNLOCK_SCOPE(cx, scope);
>+        JS_UNLOCK_OBJ(cx, obj);

Why did you make this change?  It's unnecessary, wrong.

>     }
> #endif
> 
>     /* Nothing to do if obj's scope is already sealed. */
>     if (SCOPE_IS_SEALED(scope))
>         return JS_TRUE;
> 
>     /* XXX Enumerate lazy properties now, as they can't be added later. */
>     ida = JS_Enumerate(cx, obj);
>     if (!ida)
>         return JS_FALSE;
>     JS_DestroyIdArray(cx, ida);
> 
>     /* Ensure that obj has its own, mutable scope, and seal that scope. */
>     JS_LOCK_OBJ(cx, obj);
>     scope = js_GetMutableScope(cx, obj);
>     if (scope)
>         SCOPE_SET_SEALED(scope);
>-    JS_UNLOCK_SCOPE(cx, scope);
>+    JS_UNLOCK_OBJ(cx, obj);

This is good, though -- thanks.  Please attach a minimal patch.

/be

>     if (!scope)
>         return JS_FALSE;
> 
>     /* If we are not sealing an entire object graph, we're done. */
>     if (!deep)
>         return JS_TRUE;
> 
>     /* Walk obj->slots and if any value is a non-null object, seal it. */
>     nslots = JS_MIN(scope->map.freeslot, scope->map.nslots);
>     for (vp = obj->slots, end = vp + nslots; vp < end; vp++) {

Comment 3

[timeless]

Attachment: unlock the object that was locked

it seemed like a pattern. A comment explaining why one object was being locked and a different one unlocked would have prevented me from making that change.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 214439 [details] [diff] [review]
unlock the object that was locked

Those assertions should have told you the same.  Commenting every use-case is bad.  Commenting a definition or declaration is better, but in this case assertions are even better.

I'd like this in js1.6rc1 if possible.

/be

Comment 5

[Bob Clary [:bc] (inactive)]

please check this in soonest.

Comment 6

[timeless]

Comment on attachment 214439 [details] [diff] [review]
unlock the object that was locked

timeless%mozdev.org 	mozilla/js/src/jsapi.c 	3.249

Comment 7

[Bob Clary [:bc] (inactive)]

nominating for 1.8.1 for synchronicity sake.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this part of the JS 1.7 landing? If not, can we get it checked into the 1.8.1 branch please?

Comment 9

[Brendan Eich [:brendan]]

Fixed with JS1.7 landing.

/be

Comment 10

[Bob Clary [:bc] (inactive)]

not on 1.8.0, not making js16