Closed Bug 329749 Opened 14 years ago Closed 14 years ago

Did JS_SealObject mean to call JS_UNLOCK_OBJ instead of JS_UNLOCK_SCOPE ?

Categories

(Core :: JavaScript Engine, defect, critical)

PowerPC
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta1

People

(Reporter: timeless, Assigned: timeless)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, crash, fixed1.8.1)

Attachments

(2 obsolete files)

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? :)
Attachment #214410 - Flags: review?(mrbkap)
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++) {
Attachment #214410 - Flags: review?(mrbkap) → review-
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.
Attachment #214410 - Attachment is obsolete: true
Attachment #214439 - Flags: review?(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
Attachment #214439 - Flags: review?(brendan) → review+
Blocks: js1.6rc1
Status: UNCONFIRMED → NEW
Ever confirmed: true
please check this in soonest.
Comment on attachment 214439 [details] [diff] [review]
unlock the object that was locked

timeless%mozdev.org 	mozilla/js/src/jsapi.c 	3.249
Attachment #214439 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #214439 - Flags: approval-branch-1.8.1?(brendan)
Attachment #214439 - Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+
Flags: in-testsuite-
nominating for 1.8.1 for synchronicity sake.
Flags: blocking1.8.1?
Flags: blocking1.8.1? → blocking1.8.1+
Whiteboard: [checkin needed]
Is this part of the JS 1.7 landing? If not, can we get it checked into the 1.8.1 branch please?
Target Milestone: --- → mozilla1.8.1beta1
Fixed with JS1.7 landing.

/be
Whiteboard: [checkin needed]
Keywords: fixed1.8.1
not on 1.8.0, not making js16
No longer blocks: js1.6rc1
You need to log in before you can comment on or make changes to this bug.