Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
* I promised if I ever got through bug 506491 alive, I would add a few asserts. Here they are.

* The patch for bug 503080 regressed js_DumpObject; here's the fix.

* Since we know Block objects have slots for all the let-variables by the time PutBlockObject has been called, there's really no need for block_{get,set}Property to call JS_{Get,Set}ReservedSlot. They can just read the slot directly.  Happily this change does not conflict with Igor's patch in bug 511425.

* In fact, I don't think block_{get,set}Property can be called except with tinyids, so tighten the runtime check for JSVAL_IS_INT(idval) to an assertion.

* Give js_BlockClass the stub JSClass.{get,set}Property hooks. Instead, when defining a property of a Block, put the special block_{get,set}Property getter and setter on it. No change in behavior, but I think it's better to avoid using the JSClass-wide hooks when possible.
(Assignee)

Comment 1

8 years ago
Created attachment 395647 [details] [diff] [review]
v1

Feel free to hand off review to someone else. I don't know who knows this code well.
Assignee: general → jorendorff
Attachment #395647 - Flags: review?(brendan)
Attachment #395647 - Flags: review?(igor)

Updated

8 years ago
Attachment #395647 - Flags: review?(jim)
Attachment #395647 - Flags: review?(igor)
Attachment #395647 - Flags: review?(brendan)

Updated

8 years ago
Attachment #395647 - Flags: review?(jim)

Comment 2

8 years ago
Comment on attachment 395647 [details] [diff] [review]
v1

>@@ -2616,54 +2617,74 @@ js_PutBlockObject(JSContext *cx, JSBool 
> block_getProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> {
>-    JS_ASSERT(JS_InstanceOf(cx, obj, &js_BlockClass, NULL));
>+    JS_ASSERT(obj->getClass() == &js_BlockClass);

Nit: comment here that Block objects do not leak to scripts hence we can assume that the class is Block and the id is less than OBJ_BLOCK_COUNT(cx, obj). 

>+    JS_UNLOCK_OBJ(cx, obj);
>+    return JS_TRUE;

Nit: use "return true" here and change JS_FALSE/JS_TRUE to true/false in this and the following function.

>+
>+bool
>+js_DefineBlockVariable(JSContext *cx, JSObject *obj, jsid id, int16 index)

Use JSBool here for the the result. Otherwise MSVC would rightfully warn about JSBool->bool cast of the result of js_DefineNativeProperty. Also, if the function is only used in jsobj.cpp, then make it static.

r+ with this fixed.

Comment 3

8 years ago
Comment on attachment 395647 [details] [diff] [review]
v1

> static inline bool
> OBJ_IS_CLONED_BLOCK(JSObject *obj)
> {
>-    return obj->fslots[JSSLOT_PROTO] != JSVAL_NULL;
>+    return !JSVAL_IS_NULL(obj->fslots[JSSLOT_PROTO]);
> }

One more thing: with the bug 511425 fixed use return !obj->getProto().
(Assignee)

Updated

8 years ago
Attachment #395647 - Flags: review?(igor)
(Assignee)

Comment 4

8 years ago
Comment on attachment 395647 [details] [diff] [review]
v1

I don't know how the r? bit got cleared.
(Assignee)

Comment 5

8 years ago
Comment on attachment 395647 [details] [diff] [review]
v1

Oh, it actually got reviewed. Sorry for the noise!
Attachment #395647 - Flags: review?(igor)

Updated

8 years ago
Attachment #395647 - Flags: review+
Depends on: 515885
(Assignee)

Comment 6

8 years ago
http://hg.mozilla.org/tracemonkey/rev/297db27579ca

But Gary already found an assert in there, bug 515885.
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/297db27579ca
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 8

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c4207c7c48f6
status1.9.2: --- → beta1-fixed
Flags: blocking1.9.2+
You need to log in before you can comment on or make changes to this bug.