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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

P3
normal
VERIFIED FIXED
13 years ago
13 years ago

People

(Reporter: bc, Assigned: mrbkap)

Tracking

({regression, verified1.8.1})

Trunk
mozilla1.9alpha1
x86
Windows XP
regression, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.2 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch], URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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. :-(
(Reporter)

Updated

13 years ago
Flags: testcase+
(Assignee)

Comment 1

13 years ago
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.
Component: JavaScript Engine → DOM
Priority: -- → P3
Whiteboard: [patch]
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 2

13 years ago
Created attachment 210419 [details] [diff] [review]
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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #210419 - Flags: superreview?(brendan)
Attachment #210419 - Flags: review?(jst)
Comment on attachment 210419 [details] [diff] [review]
Fix

r=jst
Attachment #210419 - Flags: review?(jst) → review+
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
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
Attachment #210419 - Flags: superreview?(brendan) → superreview-
(Assignee)

Comment 5

13 years ago
Created attachment 210436 [details] [diff] [review]
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.
Attachment #210419 - Attachment is obsolete: true
Attachment #210436 - Flags: review?(brendan)
(Assignee)

Updated

13 years ago
Component: DOM → JavaScript Engine
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
Attachment #210436 - Flags: review?(brendan) → review-
(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
(Assignee)

Comment 8

13 years ago
(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).
(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
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
Flags: blocking1.8.0.2? → blocking1.8.0.2-
Fair enough -- I was hoping for a quicker fix, obviously.

/be
(Assignee)

Comment 12

13 years ago
Created attachment 211174 [details] [diff] [review]
Updated patch
Attachment #210436 - Attachment is obsolete: true
Attachment #211174 - Flags: review?(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
Attachment #211174 - Flags: review?(brendan) → review-
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
Attachment #211174 - Flags: review- → review+
(Assignee)

Comment 15

13 years ago
Created attachment 215931 [details] [diff] [review]
What I'm about to check in
(Assignee)

Comment 16

13 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

13 years ago
Created attachment 215961 [details] [diff] [review]
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.
Attachment #215961 - Flags: review?(brendan)
Comment on attachment 215961 [details] [diff] [review]
Followup patch

Duh!  Sure.

/be
Attachment #215961 - Flags: review?(brendan) → review+
(Reporter)

Comment 19

13 years ago
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?

(Assignee)

Comment 20

13 years ago
(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.
(Reporter)

Comment 21

13 years ago
v mac/linux/win with 20060328 trunk builds.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 22

13 years ago
Please nominate for spidermonkey 1.6 again when you have a patch that applies to the 1.8. branch.
No longer blocks: 309169

Comment 23

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

Comment 24

13 years ago
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)
Attachment #215931 - Flags: approval1.8.1+

Updated

13 years ago
Attachment #215961 - Flags: approval1.8.1+

Updated

13 years ago
Whiteboard: [patch] → [patch] [checkin needed]
Keywords: fixed1.8.1
Whiteboard: [patch] [checkin needed] → [patch]
(Reporter)

Comment 25

13 years ago
verified fixed 1.8.1 window/macppc/linux 20060707
Keywords: fixed1.8.1 → verified1.8.1
You need to log in before you can comment on or make changes to this bug.