317250 - Regression in js1_5/Regress/regress-68498-003.js

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Bob Clary [:bc] (inactive)]

Regressed on trunk and 1.8 branch in browser only.

BUGNUMBER: 68498
STATUS: Testing calling obj.eval(str)
FAILED!: [reported from test()] Testing calling obj.eval(str); currently at expect[3] within test -
FAILED!: [reported from test()] Expected value 'false', Actual value 'true'
FAILED!: [reported from test()] 

regression window: 2005-07-30-06 to 2005-07-31-10
Looks like bug 296639

I should have caught this earlier. Sorry. :-(

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

This is a DOM bug all the way (sort of). I'm not sure that this can truely block bug 309169 since the fix will be in nsDOMClassInfo... patch (and explanation) in a second.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

By using |delete y;| in the testcase, we were calling OBJ_DELETE_PROPERTY directly on the inner window, meaning that the outer window still had the property in scope (though, of course, when actually getting the value, we'd do a getProperty, and not find it on the inner element) confusing the in operator.

This patch deletes the property from both scopes, if necessary.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 210419 [details] [diff] [review]
Fix

r=jst

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 210419 [details] [diff] [review]
Fix

I was alarmed that we could end up with a var declared property on the outer object, and mrbkap and jst dug into this and agree that we should inner-ize the variable object when compiling.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better fix

Actually, we need to innerize the varobj and scopeobj when setting up with statements. I also fixed with With constructor to do the same. There's a weird use of the with class in jsxml.c, but it only uses the current scopeobj as the parent object, so I left it alone.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 210436 [details] [diff] [review]
Better fix

> #if JS_HAS_EVAL_THIS_SCOPE
>         /* If obj.eval(str), emulate 'with (obj) eval(str)' in the caller. */
>         if (indirectCall) {
>             callerScopeChain = caller->scopeChain;
>+            OBJ_TO_INNER_OBJECT(cx, obj);
>+            if (!obj)
>+                return JS_FALSE;
>             if (obj != callerScopeChain) {

Nit: order the set-up for each != operand to match left-to-right != operand order, so callerScopeChain = ... after OBJ_TO_INNER... etc.

>                 if (!js_CheckPrincipalsAccess(cx, obj,
>                                               caller->script->principals,
>                                               cx->runtime->atomState.evalAtom)) {

Nit: { overflows column 80 -- move to next line, exceptional case where it should sit over the closing }.  Bonus for fixing js_CheckScopeChainValidity 80th column violation you left earlier ;-).

>@@ -1803,26 +1806,32 @@ With(JSContext *cx, JSObject *obj, uintN
>         obj = js_NewObject(cx, &js_WithClass, NULL, NULL);
>         if (!obj)
>             return JS_FALSE;
>         *rval = OBJECT_TO_JSVAL(obj);
>     }
> 
>     parent = cx->fp->scopeChain;
>     if (argc > 0) {
>         if (!js_ValueToObject(cx, argv[0], &proto))
>             return JS_FALSE;
>+        OBJ_TO_INNER_OBJECT(cx, proto);
>+        if (!proto)
>+            return JS_FALSE;
>         v = OBJECT_TO_JSVAL(proto);
>         if (!obj_setSlot(cx, obj, INT_TO_JSVAL(JSSLOT_PROTO), &v))
>             return JS_FALSE;
>         if (argc > 1) {
>             if (!js_ValueToObject(cx, argv[1], &parent))
>                 return JS_FALSE;
>+            OBJ_TO_INNER_OBJECT(cx, parent);
>+            if (!parent)
>+                return JS_FALSE;
>         }
>     }
>     v = OBJECT_TO_JSVAL(parent);
>     return obj_setSlot(cx, obj, INT_TO_JSVAL(JSSLOT_PARENT), &v);

These OBJ_TO_INNER_OBJECT should be pushed down below obj_setSlot into js_SetProtoOrParent, where they are needed to handle all cases.

jsscript.c needs patching too, to match obj_eval.  In two places, yet!  Sigh.

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> These OBJ_TO_INNER_OBJECT should be pushed down below obj_setSlot into
> js_SetProtoOrParent, where they are needed to handle all cases.

s/they/it/

Maybe we can common other code to-do with eval and Script a bit harder.  Let me know what you think.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> These OBJ_TO_INNER_OBJECT should be pushed down below obj_setSlot into
> js_SetProtoOrParent, where they are needed to handle all cases.

Except that would be wrong, no? There are several objects that are (correctly) parented to the outer object. In fact, there are a bunch of them (Components, java, and a few others spring to mind).

> jsscript.c needs patching too, to match obj_eval.  In two places, yet!  Sigh.

I don't see why jsscript.c needs any patching. As far as I can tell, it leaves all scopeobjs and varobjs alone (which is the case that really matters here). The real bug here is that with statements needed to innerize their objects (so that var declarations don't confuse the in operator).

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> (In reply to comment #6)
> > These OBJ_TO_INNER_OBJECT should be pushed down below obj_setSlot into
> > js_SetProtoOrParent, where they are needed to handle all cases.
> 
> Except that would be wrong, no? There are several objects that are (correctly)
> parented to the outer object. In fact, there are a bunch of them (Components,
> java, and a few others spring to mind).

Why are these objects parented by an outer window?  If they are cleared on every page transition, why aren't they on the inner window?  The outer window outlives the inner and document loads, its lifetime matches the GUI window.  If anything manages to form a lexical closure in its scope, we have a security hole.  So this sounds like a hazard to me.

> > jsscript.c needs patching too, to match obj_eval.  In two places, yet!  Sigh.
> 
> I don't see why jsscript.c needs any patching.

Sorry, it already does, via js_CheckScopeChainValidity.  Ok.

/be

Comment 10

[Daniel Veditz [:dveditz]]

Brendan: is this really a 1.8.0.2 blocker? You're rejecting the patches, will we get one in the right timeframe?

If we get a tested patch you can ask for approval and renominate the bug

Comment 11

[Brendan Eich [:brendan]]

Fair enough -- I was hoping for a quicker fix, obviously.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated patch

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 211174 [details] [diff] [review]
Updated patch

Still want inner-izing pushed down into js_SetProtoOrParent, so any other callers of it than obj_setSlot get the treatment.

/be

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 211174 [details] [diff] [review]
Updated patch

Sorry, you are right -- r+me with a clear comment about why in obj_setSlot.

/be

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I'm about to check in

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Followup patch

If an evil site sets an object's __proto__ to null, my previous patch will crash. I'm checking this patch in now to avoid trunk crashiness and hoping brendan will stamp it.

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 215961 [details] [diff] [review]
Followup patch

Duh!  Sure.

/be

Comment 19

[Bob Clary [:bc] (inactive)]

attachment 215931 [details] [diff] [review] fails to apply to my patched 1.6 tree (up to 2006-03-15 01:23) or a current 1.8 branch when patching file jsinterp.c Hunk #1 FAILED at 2114. Blake, am I missing something or can you generate a combined patch for this and the followup against 1.8 for me?

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #19)
> Blake, am I missing something or can you generate a combined patch for
> this and the followup against 1.8 for me?

I'll have to generate a 1.8 branch patch. The threaded interpreter stuff is going to make any patches to jsinterp.c fail to apply between the two branches.

Comment 21

[Bob Clary [:bc] (inactive)]

v mac/linux/win with 20060328 trunk builds.

Comment 22

[Bob Clary [:bc] (inactive)]

Please nominate for spidermonkey 1.6 again when you have a patch that applies to the 1.8. branch.

Comment 23

[Darin Fisher]

Any plans to create a 1.8 branch patch?  Time is running out.

Comment 24

[Darin Fisher]

Comment on attachment 215931 [details] [diff] [review]
What I'm about to check in

a=darin on behalf of drivers (please land both patches on the 1.8 branch and add the fixed1.8.1 keyword)

Comment 25

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1 window/macppc/linux 20060707