Closed Bug 418069 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta4

People

(Reporter: bc, Assigned: mrbkap)

References

()

Details

(Keywords: regression, testcase)

Attachments

(1 file, 2 obsolete files)

browser test failure starting with 2008021604 builds.

BUGNUMBER: 379245
STATUS: inline calls
FAILED! expected: [reported from test()] uncaught exception: bad this
Flags: in-testsuite+
Flags: in-litmus-
Flags: blocking1.9?
Attached patch More outerObject badness (obsolete) — Splinter Review
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).
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #304141 - Flags: superreview?(jst)
Attachment #304141 - Flags: review?(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
Attachment #304141 - Flags: review?(brendan) → review+
Attachment #304141 - Flags: superreview?(jst) → superreview+
Comment on attachment 304141 [details] [diff] [review]
More outerObject badness

This will fix a bunch of testsuite failures.
Attachment #304141 - Flags: approval1.9?
Target yer bugs!

/be
Flags: blocking1.9? → blocking1.9+
Target Milestone: --- → mozilla1.9beta4
Attachment #304141 - Flags: approval1.9? → approval1.9+
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.
Keywords: checkin-needed
Priority: -- → P2
Attached patch Updated to comments (obsolete) — Splinter Review
Attachment #304141 - Attachment is obsolete: true
Blake:  Did you mean to convey your flags from the previous patch, or does this need more eyeballs?
Oh, I didn't bother moving them forward. This is ready to go in.
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
I backed this out to fix the Camino orange (smorgan confirmed this was the cause).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
sayrer also points out that this caused a sunspider regression.
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.
Is this going to miss beta4?
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.
Blocks: 419221
Moving bugs that aren't beta 4 blockers to target final release.
Target Milestone: mozilla1.9beta4 → mozilla1.9
Er, oops, this *should* block beta 4 though, IMO.
Priority: P2 → P1
Target Milestone: mozilla1.9 → mozilla1.9beta4
Flags: tracking1.9+ → blocking1.9+
This fixes the same problem for non-chrome windows as for chrome windows. I filed bug 420372 on removing the hacks added here.
Attachment #304521 - Attachment is obsolete: true
Attachment #306605 - Flags: superreview?(jst)
Attachment #306605 - Flags: review?(jst)
Attachment #306605 - Flags: superreview?(jst)
Attachment #306605 - Flags: superreview+
Attachment #306605 - Flags: review?(jst)
Attachment #306605 - Flags: review+
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.
Attachment #306605 - Flags: approval1.9b4?
Comment on attachment 306605 [details] [diff] [review]
Fixing the camino orange

a1.9b4=beltzner
Attachment #306605 - Flags: approval1.9b4? → approval1.9b4+
Fix checked back in.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Depends on: 420426
See bug 420426 for the sunspider perf hit bug.
Depends on: 420480
Duplicate of this bug: 418067
v
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.