390948 - js1_5/Regress/regress-156354.js FAIL browser - NS_ERROR_ILLEGAL_VALUE

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

The following tests regressed in the browser between 8/2-8/3

js1_5/Regress/regress-156354.js
js1_5/extensions/regress-381205.js
js1_5/extensions/regress-383965.js
js1_5/extensions/scope-001.js

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This fixes a typo in GetWrapper (duh!) that fixes most of the testcase. The rest of the fix is in XPCWrapper::NewResolve, where we now preserve getter, setter and attributes (such as JSPROP_ENUMERATE).

Johnny, does this do the right thing for safe JSObject wrappers, where we used to only set ENUMERATE?

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 276224 [details] [diff] [review]
Proposed fix

 GetWrapper(JSContext *cx, JSObject *obj)
 {
   while (JS_GET_CLASS(cx, obj) != &sXPC_XOW_JSClass.base) {
-    obj = JS_GetParent(cx, obj);
+    obj = JS_GetPrototype(cx, obj);
     if (!obj) {
       break;
     }

Um, yeah :)

- In XPCWrapper::NewResolve():

+  uintN attrs = sprop->attrs;
+  JSPropertyOp getter = (attrs & JSPROP_GETTER) ? sprop->getter : nsnull;
+  JSPropertyOp setter = (attrs & JSPROP_SETTER) ? sprop->setter : nsnull;
 
   OBJ_DROP_PROPERTY(cx, innerObjp, prop);
 
@@ -310,8 +314,8 @@ XPCWrapper::NewResolve(JSContext *cx, JS
     return JS_FALSE;
   }
 
-  JSBool ok = OBJ_DEFINE_PROPERTY(cx, wrapperObj, interned_id, v, nsnull,
-                                  nsnull, JSPROP_ENUMERATE, nsnull);
+  JSBool ok = OBJ_DEFINE_PROPERTY(cx, wrapperObj, interned_id, v, getter,
+                                  setter, attrs, nsnull);

Do we know we don't need to wrap getter/setter in XOWs or anything here?
 
r+sr=jst with that.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #2)
> Do we know we don't need to wrap getter/setter in XOWs or anything here?

In the interests of paranoia, I'm going to punt on this for now, and just pass (attrs & JSPROP_ENUMERATE) with no getter or setter.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 6

[Bob Clary [:bc] (inactive)]

(In reply to comment #0)
> The following tests regressed in the browser between 8/2-8/3
> 
> js1_5/Regress/regress-156354.js

This passes in the trunk now.

> js1_5/extensions/regress-381205.js
> js1_5/extensions/regress-383965.js
> js1_5/extensions/scope-001.js
> 

These still fail in the trunk however with different errors.

BUGNUMBER: 381205
STATUS: uneval with special getter functions
FAILED! expected: invalid getter usage 

BUGNUMBER: 383965
STATUS: getter function with toSource
FAILED! expected: gett has no properties 

BUGNUMBER: 53268
STATUS: Testing scope after changing obj.__proto__
PASSED! Step 1: setting obj.__proto__ = global object
FAILED! expected: [reported from test()] Expected value '5', Actual value '1'
FAILED! expected: [reported from test()] Type mismatch, expected type undefined, actual type number Expected value 'undefined', Actual value '1'
PASSED! Step 2: setting obj.__proto__ = null
PASSED! Step 3: setting obj.__proto__ to global object again
PASSED! Step 3: setting obj.__proto__ to global object again
PASSED! Step 4: setting obj.__proto__ to null again
PASSED! Step 4: setting obj.__proto__ to null again 

It difficult to diagnose the cause since there might have been another causes in the meantime hidden by the this bug. Should I reopen this bug or file new ones?

Comment 7

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows

Comment 8

[Bob Clary [:bc] (inactive)]

fyi, the other failures are bug 393267, bug 393268, bug 393269.