418069 - js1_5/Regress/regress-379245.js FAIL - browser - bad this

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

browser test failure starting with 2008021604 builds.

BUGNUMBER: 379245
STATUS: inline calls
FAILED! expected: [reported from test()] uncaught exception: bad this

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: More outerObject badness

I don't like this any more than the next guy, but we do need to outerize |this| at some point to avoid cases like these, where we end up with one inner |this| and one outer (XOW wrapped) |this|. I suspect that this patch will fix bug 418067 as well.

Note the fancy footwork in nsWindowSH::OuterObject, when I first tried this patch (solely in jsinterp.c), I was hitting a case where we'd end up trying to get the global JS object on an outer chrome window before mJSObject had been set (so from within nsJSContext::InitContext).

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 304141 [details] [diff] [review]
More outerObject badness

>+++ b/js/src/jsobj.h
>@@ -109,6 +109,15 @@ struct JSObjectMap {
>                 obj = xclasp_->innerObject(cx, obj);                          \
>         }                                                                     \
>     JS_END_MACRO

Nit: blank line here, just one please ;-).

>+#define OBJ_TO_OUTER_OBJECT(cx,obj)                                           \
>+    JS_BEGIN_MACRO                                                            \
>+        JSClass *clasp_ = OBJ_GET_CLASS(cx, obj);                             \
>+        if (clasp_->flags & JSCLASS_IS_EXTENDED) {                            \
>+            JSExtendedClass *xclasp_ = (JSExtendedClass*)clasp_;              \
>+            if (xclasp_->outerObject)                                         \
>+                obj = xclasp_->outerObject(cx, obj);                          \
>+        }                                                                     \
>+    JS_END_MACRO

Guess we have to have this. It won't hurt perf that much. Wonder why we can't get away from it, though -- is the underlying bug our extended eval? Or are closures enough?

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 304141 [details] [diff] [review]
More outerObject badness

This will fix a bunch of testsuite failures.

Comment 4

[Brendan Eich [:brendan]]

Target yer bugs!

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

I'll attach a patch with brendan's nit addressed. It's going to be a couple of days before I'm going to have a chance to check this in.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

Comment 7

[Brian Crowder]

Blake:  Did you mean to convey your flags from the previous patch, or does this need more eyeballs?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Oh, I didn't bother moving them forward. This is ready to go in.

Comment 9

[Brian Crowder]

Checking in dom/src/base/nsDOMClassInfo.cpp;
/cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v  <--  nsDOMClassInfo.cpp
new revision: 1.522; previous revision: 1.521
done
Checking in js/src/jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.437; previous revision: 3.436
done
Checking in js/src/jsobj.h;
/cvsroot/mozilla/js/src/jsobj.h,v  <--  jsobj.h
new revision: 3.94; previous revision: 3.93
done

Comment 10

[:Gavin Sharp [email: [e-mail]]]

I backed this out to fix the Camino orange (smorgan confirmed this was the cause).

Comment 11

[:Gavin Sharp [email: [e-mail]]]

sayrer also points out that this caused a sunspider regression.

Comment 12

[jag (Peter Annema)]

Comment on attachment 304521 [details] [diff] [review]
Updated to comments

>diff --git a/js/src/jsinterp.c b/js/src/jsinterp.c
>--- a/js/src/jsinterp.c
>+++ b/js/src/jsinterp.c
>@@ -951,13 +951,7 @@ ComputeGlobalThis(JSContext *cx, JSBool 
>             thisp = parent;
>     }
> 
>-    clasp = OBJ_GET_CLASS(cx, thisp);
>-    if (clasp->flags & JSCLASS_IS_EXTENDED) {
>-        JSExtendedClass *xclasp = (JSExtendedClass *) clasp;
>-        if (xclasp->outerObject && !(thisp = xclasp->outerObject(cx, thisp)))
>-            return NULL;
>-    }
>-
>+    OBJ_TO_OUTER_OBJECT(cx, thisp);
>     argv[-1] = OBJECT_TO_JSVAL(thisp);

Is it ok to set argv[-1] here when !thisp? You previously wouldn't.

Comment 13

[Bob Clary [:bc] (inactive)]

Is this going to miss beta4?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Unless someone else can figure out what's going on with Camino, yes. I'm not going to be able to look at this before tomorrow.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Moving bugs that aren't beta 4 blockers to target final release.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Er, oops, this *should* block beta 4 though, IMO.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fixing the camino orange

This fixes the same problem for non-chrome windows as for chrome windows. I filed bug 420372 on removing the hacks added here.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 306605 [details] [diff] [review]
Fixing the camino orange

This patch won't turn Camino orange and will fix the facebook "friends" list.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306605 [details] [diff] [review]
Fixing the camino orange

a1.9b4=beltzner

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked back in.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

See bug 420426 for the sunspider perf hit bug.

Comment 23

[Bob Clary [:bc] (inactive)]

v