Last Comment Bug 634590 - Unqualified function invocation doesn't use the global object the property was gotten from as |this|
: Unqualified function invocation doesn't use the global object the property wa...
Status: RESOLVED FIXED
[hardblocker][has patch], fixed-in-tr...
: regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- major with 16 votes (vote)
: ---
Assigned To: Andreas Gal :gal
:
Mentors:
: 635582 (view as bug list)
Depends on: 635582 636364 636795 636820 653959
Blocks: 671947
  Show dependency treegraph
 
Reported: 2011-02-16 07:16 PST by tgvrs_santhosh
Modified: 2013-12-27 14:25 PST (History)
29 users (show)
choller: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+


Attachments
ffTest (409 bytes, text/html)
2011-02-16 07:18 PST, tgvrs_santhosh
no flags Details
ffTest2 (1.04 KB, text/html)
2011-02-16 07:18 PST, tgvrs_santhosh
no flags Details
Simple testcase (188 bytes, text/html)
2011-02-16 10:22 PST, Boris Zbarsky [:bz]
no flags Details
shell testcase based on evalcx (159 bytes, text/plain)
2011-02-16 12:27 PST, Brendan Eich [:brendan]
no flags Details
Another, more interesting testcase (306 bytes, text/html)
2011-02-16 12:32 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details
patch (592 bytes, patch)
2011-02-16 12:40 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (5.19 KB, patch)
2011-02-16 18:17 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (17.51 KB, patch)
2011-02-17 17:03 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (17.51 KB, patch)
2011-02-17 17:05 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (17.53 KB, patch)
2011-02-17 17:16 PST, Andreas Gal :gal
brendan: review+
Details | Diff | Review
patch (17.22 KB, patch)
2011-02-18 15:06 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (18.21 KB, patch)
2011-02-18 17:47 PST, Andreas Gal :gal
brendan: review+
Details | Diff | Review
followup fix and tests that deliver the goods (8.66 KB, patch)
2011-02-19 23:51 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
patch (975 bytes, patch)
2011-02-21 16:17 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (4.90 KB, patch)
2011-02-21 17:05 PST, Andreas Gal :gal
no flags Details | Diff | Review
patch (21.17 KB, patch)
2011-02-22 22:19 PST, Andreas Gal :gal
gal: review+
Details | Diff | Review

Description tgvrs_santhosh 2011-02-16 07:16:29 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.13 (KHTML, like Gecko) Chrome/9.0.597.98 Safari/534.13
Build Identifier: firefox 4 b11

i have an iframe where prototypes have been defined. Created another iframe in that where i have attached these prototype methods to the window of inner iframe. now, if i call the prototype method using window object, then i'm getting outer window context.

Reproducible: Always

Steps to Reproduce:
first create an htm page with the following content with name ffTest.htm

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<script type="text/javascript">
function sampleFunc()
{
	var iframeElem = document.createElement("iframe");
	iframeElem.id = "firstIframe";
	iframeElem.src = "ffTest2.htm";
	document.body.appendChild(iframeElem);
}
</script>
</head>
<body onload="sampleFunc()">
</body>
</html>

then create another htm page with name ffTest2.htm with the following content.

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<script type="text/javascript">
function sampleFunc2()
{
	var iframeElem = document.createElement("iframe");
	iframeElem.id = "secondIframe";
	document.body.appendChild(iframeElem);
	var iframeElemDoc = iframeElem.contentDocument;
	iframeElemDoc.open();
	iframeElemDoc.writeln("<html><head></head><body></body></html>");
	iframeElemDoc.close();
	var src = "var n = {callFunc:function(){alert(frameElement.id);f1();}}";
	var scriptElem = iframeElemDoc.createElement("script");
	iframeElemDoc.getElementsByTagName("head")[0].appendChild(scriptElem);
	scriptElem.setAttribute("type","text/javascript");
	scriptElem.text = src;
	var iframeElemWindow = iframeElem.contentWindow;
	for (var i in x.prototype)	iframeElemWindow[i] = x.prototype[i];
	iframeElemWindow['n'].callFunc();
}
function x()
{
}
x.prototype.f1 = function()
{
	alert(this.frameElement.id);
}
</script>
</head>
<body onload="sampleFunc2()">
</body>
</html>

Now, access ffTest.htm.
Actual Results:  
first secondIframe will be alerted.
then firstIframe will be alerted.

Expected Results:  
secondIframe should be alerted.
again secondIframe should be alerted.

It's working fine in firefox4 Beta 4, But failed in firefox4 Beta 11.

here is the build config of ffb11.

about:buildconfig

Source

Built from http://hg.mozilla.org/mozilla-central/rev/bb9089ae2322

Build platform

target
i686-pc-mingw32
Build tools

Compiler	Version	Compiler flags
d;D:\mozilla-build\msys\mozilla-build\python25\python2.5.exe -O e;D:\mozilla-build\msys\builds\moz2_slave\rel-cen-w32-bld\build\build\cl.py cl	14.00.50727.762	 -TC -nologo -W3 -Gy -Fdgenerated.pdb -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1
d;D:\mozilla-build\msys\mozilla-build\python25\python2.5.exe -O e;D:\mozilla-build\msys\builds\moz2_slave\rel-cen-w32-bld\build\build\cl.py cl	14.00.50727.762	 -GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fdgenerated.pdb -wd4800 -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1
Configure arguments

--enable-application=browser --enable-update-channel=beta --enable-update-packaging --enable-jemalloc --enable-tests --enable-official-branding
Comment 1 tgvrs_santhosh 2011-02-16 07:18:17 PST
Created attachment 512795 [details]
ffTest
Comment 2 tgvrs_santhosh 2011-02-16 07:18:48 PST
Created attachment 512796 [details]
ffTest2
Comment 3 Mats Palmgren (:mats) 2011-02-16 08:02:27 PST
It would help if you can find the first build where it broke.
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/
Comment 4 Mats Palmgren (:mats) 2011-02-16 08:19:42 PST
Nevermind, I found the regression range:  2010-09-11-03 -- 2010-09-12-03
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=73ab2c3c5ad9&tochange=cd3c926a7413
Comment 5 Boris Zbarsky [:bz] 2011-02-16 10:07:29 PST
Hmm.  That's the JM landing.
Comment 6 Boris Zbarsky [:bz] 2011-02-16 10:22:04 PST
This is a JS-level change.  I'll attach a minimal-ish testcase in which we alert "outer" while Chrome, Safari, Opera, and Fx 3.6 alert "inner".

Of course the ES5 spec is wonderfully silent on this issue.  Yay.

Not sure whether this should block.  This has potential to break sites... but we haven't found one yet.
Comment 7 Boris Zbarsky [:bz] 2011-02-16 10:22:32 PST
Created attachment 512842 [details]
Simple testcase
Comment 8 Andreas Gal :gal 2011-02-16 11:11:09 PST
This is a dup.
Comment 9 Andreas Gal :gal 2011-02-16 11:11:36 PST
Actually it may be a dup. Waldo?
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-16 11:14:59 PST
I don't remember seeing this reported, but I might have missed it, way behind on overall bugmail.  I'll test other browsers when I get to the office.
Comment 11 Boris Zbarsky [:bz] 2011-02-16 11:17:55 PST
See comment 6.  You can restrict your other-browser testing to IE.  ;)
Comment 12 Brendan Eich [:brendan] 2011-02-16 11:24:31 PST
This is a bug for sure. Why is the call to f, where that reference is based on the inner window, binding |this| to the outer window (which is f's [[Scope]] internal property value, but that's not relevant)?

/be
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-16 11:43:21 PST
IE9 does what everyone did before September 11 last year.  I'll take this.
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-16 11:43:41 PST
I don't think this should block until we have evidence that it breaks sites.
Comment 15 Andreas Gal :gal 2011-02-16 11:51:43 PST
Its a regression, so we should discuss taking a reviewed and tested patch if we get one in time.
Comment 16 Brendan Eich [:brendan] 2011-02-16 12:24:35 PST
This is very likely to break sites from all I know about JS. Just my "estimate", informed by too much experience.

This is a bug in proxy_call not eagerly computing |this|. Shell test in a sec.

/be
Comment 17 Brendan Eich [:brendan] 2011-02-16 12:27:08 PST
Created attachment 512885 [details]
shell testcase based on evalcx

JSCrossCompartmentWrapper::call may be the buggy method. It does

621	    if (!call.destination->wrap(cx, &vp[1]))

but vp[1] is undefined (lazy this) here. After this point, I think we've lost the thread back to the correct global. Andreas?

/be
Comment 18 Brendan Eich [:brendan] 2011-02-16 12:28:15 PST
So proxy_call is just C forwarding veneer but somewhere between it and line 621 shown in comment 17, we definitely need an eager-this computation based on the correct global for the callee reference expression.

/be
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-16 12:32:54 PST
Created attachment 512887 [details]
Another, more interesting testcase

So, looking closer, are we sure these aren't the proper semantics?  For a non-strict function, |this| is the global object for that function, with our current behavior.  With the previous behavior, |this| depended on the dynamic scoping of the call site.  That's almost always a wrong sign.

For another point, shouldn't, |f()| and |f.call(undefined)| behave identically?  See the attachment.  With our current (and I now believe correct) behavior, the two return the same value.  With our previous behavior, and with that of other browsers, the two have different behavior.
Comment 20 Andreas Gal :gal 2011-02-16 12:40:43 PST
Created attachment 512890 [details] [diff] [review]
patch
Comment 21 Brendan Eich [:brendan] 2011-02-16 12:44:15 PST
(In reply to comment #19)
> Created attachment 512887 [details]
> Another, more interesting testcase
> 
> So, looking closer, are we sure these aren't the proper semantics?

Yes. I'm sure, because I created all this stuff in 1995 and it has worked that way since. It works that way in Chrome, Safari, Opera, and (modulo fixed bugs in preview releases) IE.

> For a
> non-strict function, |this| is the global object for that function, with our
> current behavior.  With the previous behavior, |this| depended on the dynamic
> scoping of the call site.  That's almost always a wrong sign.

No, |this| does not depend on "dynamic scope". It depends as always on the callee Reference base, which is evaluated per ECMA-262 rules. The only cross-window or cross-global fact at play here, which goes beyond ES-anything, is the [[Scope]] internal property of f being a different global. And that has *nothing* to do with |this| binding!

> For another point, shouldn't, |f()| and |f.call(undefined)| behave identically?

Because of multiple global objects, not in ES-anything.

f() // call this.f not parent.f (in my shell testcase)

f.call(undefined) // invokes Object.getPrototypeOf(f).call

In the first case, |this| binds to the reference base, the sandbox.

In the second, |this| binds per ES5 15.3.4.4. See the NOTE on a change from ES3:

"NOTE The thisArg value is passed without modification as the this value. This is a change from Edition 3, where a undefined or null thisArg is replaced with the global object and ToObject is applied to all other values and that result is
passed as the this value."

Now in ES3, "the global object" was the one-and-only. In reality, the global in browsers would be the window or frame at the end of the scope chain of the script that called fun_call (the native). You could call this "dynamic scope" but it's not. It is a parameter, |this|, implicitly passed based on the callee expression's reference-base.

Anyway, ES5 changed things for Function.prototype.call as NOTEd but nothing changes for unqualified name callee expressions. We have a bug in proxy-land. Andreas is patching.

>  See the attachment.  With our current (and I now believe correct) behavior,
> the two return the same value.  With our previous behavior, and with that of
> other browsers, the two have different behavior.

Come on. We're not "right" and everyone going back to the dawn of JS is "wrong" because of an equivalence that used to hold up till ES3, but does not in ES5, and only in the presence of multiple global objects.

This is backward reasoning from a faulty understanding of the spec, but worse: it's thumbing your nose at reality and browser interop.

/be
Comment 22 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-16 14:11:44 PST
(In reply to comment #21)
> Because of multiple global objects, not in ES-anything.
> 
> f() // call this.f not parent.f (in my shell testcase)
> 
> f.call(undefined) // invokes Object.getPrototypeOf(f).call
> 
> In the first case, |this| binds to the reference base, the sandbox.
> 
> In the second, |this| binds per ES5 15.3.4.4. See the NOTE on a change from
> ES3:

I'm confused.  Isn't how |this| binds, when processed in the function code for the function referred to by |f|, determined by 10.4.3, in concert with the passed-in |thisArg|?  Both instances here pass in |undefined|.  That determines the ThisBinding for the function code when executed, which is directly referred to when evaluating |this| in the function code.

> Now in ES3, "the global object" was the one-and-only. In reality, the global in
> browsers would be the window or frame at the end of the scope chain of the
> script that called fun_call (the native). You could call this "dynamic scope"
> but it's not. It is a parameter, |this|, implicitly passed based on the callee
> expression's reference-base.

Okay; ES3 passed in the caller's global object.  ES5 makes that determination the callee's responsibility.  I get that.  Yet our engine these days is in many ways based around the determined-by-callee model that ES5 specifies.  My intuition tells me there is further tension here between doing it like ES3 and implementing ES5.  I'll do some testing with the patch here.

But at root this is a tension between either implementing ES3 or implementing ES5.

> Come on. We're not "right" and everyone going back to the dawn of JS is "wrong"
> because of an equivalence that used to hold up till ES3, but does not in ES5,
> and only in the presence of multiple global objects.
>
> This is backward reasoning from a faulty understanding of the spec, but worse:
> it's thumbing your nose at reality and browser interop.

I'm feeling distinctly more antagonism here than seems warranted to me in the case that I'm wrong about this.  Please.  We're on the same team.  I don't need or want kid gloves, but I would appreciate a little more moderation of tone.
Comment 23 Brendan Eich [:brendan] 2011-02-16 14:15:24 PST
One thing to reiterate: dynamic scope, we all frown upon. |this| binding for CallExpressions (not Function.prototype.{apply,call} uses) is neither lexical nor dynamic scoping.

Lexically-scoped |this| is proposed (see http://brendaneich.com/2011/01/harmony-of-my-dreams/) for sharp functions:

  function outer() {
    let self = this;
    return #() { assertEq(this, self); };
  }
  let m1 = outer(), m2 = ({method: outer}).method();
  m1(), m2();

Whatever the callee expression, |this| in the sharpfun is bound to |this| in the enclosing lexical scope. If you want to use the reference base, you'll have to declare |this| as the first parameter (syntax TBD).

Dynamically-scoped |this| would look something like:

  var dynaThis;
  function outer() {
    var self = this;
    return function () {
      assertEq(this, dynaThis);
      assertEq(this === self, false);
    };
  }
  function unrelated(f) {
    dynaThis = this;
    f();
  }
  var obj = {method: unrelated};
  obj.method(outer());

And so on for other ways of calling outer, so long as none used a reference base of obj (dynaThis).

Anyway, |this| is neither lexically nor dynamically scoped -- it's not a scoped name in JS at all. It's a keyword standing for an implicit parameter, bound to the reference base of the callee expression or the relevant global if that base is undefined.

Function.prototype.{call,apply} offer explicit actual |this| parameterization. The ES5 change mentioned in the NOTE is a win in this light. It gets rid of an implicit conversion based on "the global object" (ES3) or the global for the function being called or apply'ed (the |this| parameter to those methods; what we did in Firefox 3:

js> f = evalcx("function f(){return this;}; f")
function f() {
    return this;
}
js> f() == this
true
js> f.call(undefined) == this
false
js> f.call(undefined)
[object sandbox]

This is from my cvs.mozilla.org-built js shell).

/be
Comment 24 Brendan Eich [:brendan] 2011-02-16 14:19:43 PST
(In reply to comment #22)
> I'm confused.  Isn't how |this| binds, when processed in the function code for
> the function referred to by |f|, determined by 10.4.3, in concert with the
> passed-in |thisArg|?

The question is "which global object". ES-all do not have more than one, so they are not sufficient. Again, the spec is incomplete.

The spec is great but it is "just another implementation" ignoring its normative status. It has bugs and omissions. Do not put it on a pedestal at the expense of reality.

> Okay; ES3 passed in the caller's global object.  ES5 makes that determination
> the callee's responsibility.  I get that.  Yet our engine these days is in many
> ways based around the determined-by-callee model that ES5 specifies.

ES5 and ES3 have only one global, so they don't help. But you are mixing up evaluating a CallExpression with Function.prototype.{apply,call}.

> I'm feeling distinctly more antagonism here than seems warranted to me in the
> case that I'm wrong about this.  Please.  We're on the same team.  I don't need
> or want kid gloves, but I would appreciate a little more moderation of tone.

I'm making a serious point, which I've made before and you've rejected or just not heard. The spec is not complete, especially when it comes to things like multiple global objects. The idea that ES5, or Firefox + ES5, trumps what all browsers including past Mozilla ones, back to Netscape 2, is deeply wrong. Sorry.

/be
Comment 25 Brendan Eich [:brendan] 2011-02-16 14:23:14 PST
Comment on attachment 512890 [details] [diff] [review]
patch

This needs the shell-only test that I attached, added to the suite with the

skip-if(!xulRuntime.shell) ...


prefix. Also, maybe a one-line comment?

A blank line after the return would be easier on the eyes.

It seems JSProxy::call is used in only this call-site, so you could make the fix there or here. It doesn't really matter, but I wanted to ask if you had a reason to make it here not there.

/be
Comment 26 Brendan Eich [:brendan] 2011-02-16 14:33:53 PST
(In reply to comment #22)
> Okay; ES3 passed in the caller's global object.

To clarify: it passed in "the global object". ES[1235] spec only one global, so it doesn't make sense to talk about "the caller's global object" as if there could be more than one, or that caller and callee could differ on their globals.

What implementations going back to the dawn of JS have done, in embeddings with multiple globals starting with the Netscape 2 browser, is pass the reference base before censoring it (global object as scope, ES5's object environment record). This is for CallExpressions such as the one in the test

  f()

given this.f = f for this bound in global code to the sandbox.

For Function.prototype.call/apply implementations used the global found from f's [[Scope]] internal property. For the Firefox-3-era js shell session cited in comment 23, that's the sandbox. But for the shell testcase at attachment 512885 [details], it would be the "outer" or frame-parent window.

So the f() <=> f.call(undefined) equivalence was not universal in browsers going back to the dawn of JS. Waldo's test at attachment 512887 [details] shows

unqualified: inner, .call(undef): outer

in the iframe.

For the ES spec, f() <=> f.call(undefined) is a nice equivalence, and the ES5 changes from ES3 do suggest that it is universal, but without spec'ing multiple globals, ES5 does not actually require it. And the de-facto standard trumps the suggestion.

/be
Comment 27 Brendan Eich [:brendan] 2011-02-16 14:37:24 PST
(In reply to comment #26)
> So the f() <=> f.call(undefined) equivalence was not universal in browsers
> going back to the dawn of JS. Waldo's test at attachment 512887 [details] shows
> 
> unqualified: inner, .call(undef): outer
> 
> in the iframe.

... in Firefox 1.5 through 3.6 (I need Rosetta to run Firefox 1.0.8).

/be
Comment 28 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-16 14:41:32 PST
Comment on attachment 512890 [details] [diff] [review]
patch

This patch won't cut it if we want to have our past semantics.  On the "more interesting" testcase, it prints outer for both f() and f.call(undefined), whereas past semantics were unqualified: inner, .call(undef): outer.
Comment 29 Andreas Gal :gal 2011-02-16 16:38:43 PST
Brendan, if f is a function from another scope, and you do f.call(5), the object that gets created for 5 (js_PrimitiveToObject) for a non-strict function, what is that object parented to? The caller's global or the function's global?
Comment 30 Andreas Gal :gal 2011-02-16 17:14:43 PST
The answer for comment 29 is that according to ES5 the object is from the callee's scope (this coercion is done by the callee).
Comment 31 Brendan Eich [:brendan] 2011-02-16 17:25:27 PST
(In reply to comment #30)
> The answer for comment 29 is that according to ES5 the object is from the
> callee's scope (this coercion is done by the callee).

Of course ES5 doesn't say anything about multiple globals, but as Waldo has pointed out it does make the callee coerce this. This is to preserve strict-|this| semantics for callee functions that "use strict".

/be
Comment 32 Andreas Gal :gal 2011-02-16 18:17:00 PST
Created attachment 513008 [details] [diff] [review]
patch

Waldo and I whipped up a patch to eagerly push the global object if the current scope and the callee scope differ (since in this case the lazy this resolution would pick up the wrong global). This sits in the property cache miss slow path. We didn't check that we don't cache this case (we should double check that). Perf overhead aside, this mucks with the abstraction of strict mode as a property of the callee. If the callee is strict, we don't want to eagerly override here, so we have to find out. For proxies there is no way to tell. For wrappers we cheat by unwrapping. This is all pretty ugly. Brendan, what do you think?
Comment 33 David Mandelin [:dmandelin] 2011-02-16 19:03:37 PST
Brendan: how sure are you that keeping the Fx3.6 behavior is required for web compat? Beta users haven't reported any problems with this yet, which tends to make me think the current Fx4 behavior is OK, but I don't have a good sense of how complete coverage we get from betas on this sort of thing. I ask because it sounds like the engine is simpler and more consistent with the current Fx4 behavior, so it would be nice to keep it if we can get away with it.
Comment 34 tgvrs_santhosh 2011-02-17 06:30:29 PST
Thanks for quick response. This issue has potential to break sites and our product is already affected by this. Hope soon it will be resolved and will be available for the next beta release.
Comment 35 Mike Shaver (:shaver -- probably not reading bugmail closely) 2011-02-17 09:05:50 PST
tgvrs: could you elaborate on how your product is broken?  ideally, with a URL or code sample that can be tested?
Comment 36 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-17 12:10:00 PST
tgvrs, I'm also curious: how hard would it be for you to explicitly pass in the calling scope's global object as |this|, rather than doing an unqualified call and expecting the caller's global object?  For example, in the testcase you post in comment 0 the change is as trivial as substituting this:

  f1();

for this:

  f1.call(window);

Yours is the first report of this in five months, and as you can see there's some disagreement about what proper behavior should be here.  And in any case, explicitly passing the window you want is better than relying on the caller to pass it for you, particularly since you're relying on under-specified vagaries of the web.
Comment 37 Brendan Eich [:brendan] 2011-02-17 13:29:45 PST
I'm sure we can cajole someone kind enough to report a fundamental bug like this into working around it but this is a bad bug. It should block, in my opinion, and we should stop trying not to fix it.

I'll review the patch.

/be
Comment 38 Brendan Eich [:brendan] 2011-02-17 13:37:11 PST
Comment on attachment 513008 [details] [diff] [review]
patch

Nice to pull a macro's guts out into an inline but I don't think this approach is needed.

>+static inline bool
>+SlowThis(JSContext *cx, JSObject *obj, const Value &funval, Value *vp)
>+{
>+    JS_ASSERT(funval.isObject());
>+    JSObject *funobj = funval.toObject().unwrap();
>+    bool strict = false;
>+    if (funobj->unwrap()->isFunction()) {
>+        // XXX: This misses strict callables.

What is a strict callable?

>+        JSFunction *fun = funobj->getFunctionPrivate();
>+        strict = fun->isInterpreted() && fun->inStrictMode();
>+    }
>+    Class *clasp;
>+    JSObject *thisp = obj;
>+    if ((thisp->isGlobal() &&
>+         (strict || funobj->getParent() == cx->fp()->scopeChain().getGlobal())) ||
>+        (clasp = thisp->getClass()) == &js_CallClass ||
>+        clasp == &js_BlockClass ||
>+        clasp == &js_DeclEnvClass) {

Pre-existing (clasp == ... || ...) condition should just use js_IsCacheableNonGlobalScope.

>+        /* Push the ImplicitThisValue for the Environment Record */
>+        /* associated with obj. See ES5 sections 10.2.1.1.6 and  */
>+        /* 10.2.1.2.6 (ImplicitThisValue) and section 11.2.3     */

Pulling this comment out of the macro means you can use normal major-comment style.

The easier fix seems to me to be to not fill the property cache for calls to unqualified names where the reference base is not the callee's global. If it is important for perf to cache such calls, we can cache them differently and make the slow-this logic kick in only for such hits.

/be
Comment 39 Brendan Eich [:brendan] 2011-02-17 13:51:16 PST
Andreas and I talked -- this is a fine start, it'll want some factoring of the predicate to tell the callee expression's base is not the callee object's global, since that predicate is needed in PropertyCache::fill.

The unwrap() is confined so safe. No XXX comments please. I'll gladly review a complete patch.

/be
Comment 40 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-17 13:52:43 PST
(In reply to comment #38)
> What is a strict callable?

Proxy.createFunction with a call hook that's a strict mode function, a function cross-compartments that's a strict mode function, and so on.  And you can't check many of those because the target function to be eventually called may be in another thread, or somesuch, or not easily examinable.

> The easier fix seems to me to be to not fill the property cache for calls to
> unqualified names where the reference base is not the callee's global. If it is
> important for perf to cache such calls, we can cache them differently and make
> the slow-this logic kick in only for such hits.

The property cache isn't an issue, unless I'm missing something.  It's precisely the vp[0].toObject().getGlobal() bit in ComputeGlobalThis, which is called only when |this| is evaluated by the callee.
Comment 41 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-17 13:56:20 PST
...or earlier, of course, with this hack.
Comment 42 Andreas Gal :gal 2011-02-17 14:01:15 PST
Well we can make this work with the approach in patch, so lets do it. It won't work in some cases (i.e. strict proxy call hook). Let TC39 worry about those cases. They need to tackle multiple global objects. We need to move on this bug. It is a hard blocker.
Comment 43 Brendan Eich [:brendan] 2011-02-17 14:03:19 PST
The wrapper case is handled. Proxy.createFunction, we have a choice. Push undefined if a proxy that we can't unwrap; or do the backward-compatbile |this| computation.

The property cache is an issue. Variant testcase just loops over the call to f:

this.name = "outer";
var sb = evalcx('');
sb.name = "inner";
sb.parent = this;
function f() { return this.name; }
print(evalcx('this.f = parent.f; for(var i=0;i<2;i++) f()', sb));

On the second iteration, we hit the cache in the JSOP_CALLGNAME case and push undefined.

/be
Comment 44 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-17 14:04:49 PST
I still disagree with our needing to fix this, or with it being a bug, so I shouldn't be the assignee.
Comment 45 Brendan Eich [:brendan] 2011-02-17 14:06:00 PST
(In reply to comment #42)
> Well we can make this work with the approach in patch, so lets do it. It won't
> work in some cases (i.e. strict proxy call hook). Let TC39 worry about those
> cases. They need to tackle multiple global objects. We need to move on this
> bug. It is a hard blocker.

Apart from wrappers, which we can unwrap, we have a choice as noted in comment 43 first paragraph. We could treat opaque (general) proxies as new-ES5-strict->Harmony constructs and go with callee-coerces. Why not? Waldo, I hope you will join me on this point :-P.

/be
Comment 46 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-17 14:15:29 PST
We should indeed.  Passing a global is definitely a spec deviation, because when the function containing the |this| reference is strict, there is no ambiguity whatsoever about |this| being exactly the value |undefined|.  So proxies either deviate from the clear words of the spec by passing in the caller's global object (old behavior), or do the right thing and pass |undefined|.  But this means proxies can't emulate strict mode functions in this respect.  This disconnect further illustrates why I think old behavior was wrong, but it's unavoidable if old behavior really is deemed a gold standard.
Comment 47 Brendan Eich [:brendan] 2011-02-17 14:27:33 PST
No unassigned blockers.

/be
Comment 48 Andreas Gal :gal 2011-02-17 14:42:53 PST
This is biting some content so it should be in the beta. I will try to finish the patch today.
Comment 49 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-17 14:53:27 PST
(In reply to comment #46)
> But this means proxies can't emulate strict mode functions in
> this respect.

Slight amplification of an unclearly-stated point: can't emulate strict mode functions if a global is pushed, and can't emulate non-strict mode functions if |undefined| is pushed.  It's between a rock and a hard place, if you think the caller's global is right in some instances.
Comment 50 Brendan Eich [:brendan] 2011-02-17 15:02:24 PST
(In reply to comment #49)
> (In reply to comment #46)
> > But this means proxies can't emulate strict mode functions in
> > this respect.
> 
> Slight amplification of an unclearly-stated point: can't emulate strict mode
> functions if a global is pushed, and can't emulate non-strict mode functions if
> |undefined| is pushed.  It's between a rock and a hard place,

No. Proxies are new, not yet standard, and proposed for Harmony. Harmony is built on ES5 strict. So there is no reason to be future-hostile (we agree), and no backward compatibility issue.

That we allow Proxies to be programmed by non-strict-mode code should not alter the future-friendly vs. backward-compatibility judgment.

Non-strict proxy-handler trap functions should not imply a backward-compatible eager-|this| binding, since there aren't proxies in the backward-compatible content on the web.

> if you think the
> caller's global is right in some instances.

What to do with general (non-wrapper) proxies is not an issue.

/be
Comment 51 Allen Wirfs-Brock 2011-02-17 16:44:21 PST
I agree that compatibility with the existing web should be the primary concern for now.  That said this caller provides the global object behavior, in the presence of cross frame calls, sure sounds wrong. Particular when you consider that within the callee references to globals will resolve via the [[Scope]] chain and that in ES5 that are many places where references to built-in are statically resolved without any scope references.  So for example, if this is bound to a different frame's global object then we have situations like:
   this.Array === Array  //this will be false
and
   Object.getProtypeOf([]) === this.Array.prototype //also false

People probably seldom don't do stuff like this so it probably hasn't been noticed.  

Finally, there is an approach that I don't think has been discussed for matching the old behavior.  It involves the caller explicitly tagging (or wrappering) an implicitly passed global object such that the callee's prologue can recognize it as such.  Then a strict callee can choose to ignore it and set its this binding to undefined.  In general, it seems better for the caller to only do unconditional (based upon the callee) work and to live such callee conditional decisions to the callee.
Comment 52 Andreas Gal :gal 2011-02-17 17:03:22 PST
Created attachment 513322 [details] [diff] [review]
patch
Comment 53 Andreas Gal :gal 2011-02-17 17:05:09 PST
Created attachment 513323 [details] [diff] [review]
patch
Comment 54 Andreas Gal :gal 2011-02-17 17:16:33 PST
Created attachment 513324 [details] [diff] [review]
patch
Comment 55 Brendan Eich [:brendan] 2011-02-17 17:19:40 PST
Comment on attachment 513324 [details] [diff] [review]
patch

Nice -- just comment (before the macro, not in its body!) about the propcache fill veto-purge.

>+#define SLOW_PUSH_THISV(cx, obj, funval)                                      \
>+    JS_BEGIN_MACRO                                                            \
>+        Value v;                                                              \
>+        if (!SlowThis(cx, obj, funval, &v))                                   \
>+            goto error;                                                       \
>+        if (!v.isUndefined()) {                                               \
>+            PropertyCacheEntry *entry;                                        \
>+            JSObject *obj2;                                                   \
>+            JSAtom *atom;                                                     \
>+            JS_PROPERTY_CACHE(cx).test(cx, regs.pc, obj, obj2, entry, atom);  \
>+            if (!atom) {                                                      \
>+                ASSERT_VALID_PROPERTY_CACHE_HIT(0, obj, obj2, entry);         \
>+                memset(entry, 0, sizeof(*entry));                             \
>+            }                                                                 \
>+        }                                                                     \
>+        PUSH_COPY(v);                                                         \
>+    JS_END_MACRO                                                              \
> 

And add those tests, js1_8_5/regress is ok on the theory that anywhere beats not adding tests.

/be
Comment 56 Andreas Gal :gal 2011-02-17 17:54:32 PST
http://hg.mozilla.org/tracemonkey/rev/b0aa9c20ffe4
Comment 57 Brendan Eich [:brendan] 2011-02-17 17:59:17 PST
(In reply to comment #51)
> I agree that compatibility with the existing web should be the primary concern
> for now.  That said this caller provides the global object behavior, in the
> presence of cross frame calls, sure sounds wrong.

It is what it is -- morality does not enter into it!

> Particular when you consider
> that within the callee references to globals will resolve via the [[Scope]]
> chain and that in ES5 that are many places where references to built-in are
> statically resolved without any scope references.

The |this| parameter is not always an object from the function's scope chain. There is no necessary relation.

> So for example, if this is
> bound to a different frame's global object then we have situations like:
>    this.Array === Array  //this will be false
> and
>    Object.getProtypeOf([]) === this.Array.prototype //also false
> 
> People probably seldom don't do stuff like this so it probably hasn't been
> noticed.  

It may have been noticed but after almost 16 years it's hard to change.

> Finally, there is an approach that I don't think has been discussed for
> matching the old behavior.  It involves the caller explicitly tagging (or
> wrappering) an implicitly passed global object such that the callee's prologue
> can recognize it as such.  Then a strict callee can choose to ignore it and set
> its this binding to undefined.  In general, it seems better for the caller to
> only do unconditional (based upon the callee) work and to live such callee
> conditional decisions to the callee.

This is not the approach we chose, because what you suggest costs more on both the caller side (wrappering or tagging somehow) and the strict-mode callee side (which we do want to be fast). We're trying to bias for the future. The "wrong" old semantics are slow-path'ed.

/be
Comment 58 christian 2011-02-17 18:38:06 PST
Any chance we can get this landed on m-c tonight so it makes the nightly updates? This looks to be the last JS betaN blocker...do we need to wait for a full TM merge?
Comment 59 David Mandelin [:dmandelin] 2011-02-17 18:55:32 PST
Backed out due to tinderbox orange to avoid any risk of merging over a bad patch:

  http://hg.mozilla.org/tracemonkey/rev/f73f073eca0c

It looks like at least some of the errors may be just test cases that need updating, but I also see a crash on jsreftests.
Comment 60 Andreas Gal :gal 2011-02-17 20:45:44 PST
There is an assert that might be mine. Looking at it.
Comment 61 tgvrs_santhosh 2011-02-17 22:15:11 PST
(In reply to comment #35)
> tgvrs: could you elaborate on how your product is broken?  ideally, with a URL
> or code sample that can be tested?

Hi, I have shared a sample code in the beginning of the defect.

We have an enterprise framework and not able to share it via URL.

Our application runs even in cloud, and serves to fortune 2000 companies. Good
news is that many enterprise are  standardizing on FireFox, hence this becomes
even more critical.

Our usage is as follows:

1. The top (window) has the application.
2. A primary iframe works as a bootstrap, which loads further components on
individual iframes in-side it self.
3. Now when this 3 level (top -> bootstrap -> components) component refer to
|this| 

And changing code ff.call(window) or window.ff, would be too costly and risky
to be changed in many many places, and would not accepted.

The problem is when we say this with context of 3 level, from these recent
FireFox Beta this breaks. 

It works in IE, Chrome, Safari, Opera, FireFox 3.6 and till FireFox  Beta 4.

I could safely guess lots of enterprise or frameworks like this would have
patterns. And with various releases of Chrome/IE 9/Safari/Opera could be that
people have not tested yest with latest FireFox beta. BTW, it works in the
earlier FireFox Beta 4.
Comment 62 Andreas Gal :gal 2011-02-18 11:59:54 PST
There is a significant problem with the patch. The trace recorder fills the property cache for the interpreter to avoid an observable re-lookup in the interpreter. This makes the interpreter fall into the fast path where we do not check for cross scope function calls.

In addition, I am also a little worried about chrome global object leaking when chrome calls a content function unqualified.
Comment 63 Brendan Eich [:brendan] 2011-02-18 13:49:25 PST
Andreas and I talked and he's off the ledge. Should have a patch today.

/be
Comment 64 Andreas Gal :gal 2011-02-18 15:06:34 PST
Created attachment 513610 [details] [diff] [review]
patch
Comment 65 Brendan Eich [:brendan] 2011-02-18 15:26:35 PST
Comment on attachment 513610 [details] [diff] [review]
patch

>+SlowThis(JSContext *cx, JSObject *obj, const Value &funval, Value *vp)
>+{
>+    if (!funval.isObject() ||
>+        (obj->isGlobal() || IsCacheableNonGlobalScope(obj)) &&
>+        IsSafeForLazyThisCoercion(cx, funval)) {

Parenthesize the right operand of the outer (left-most) || to avoid GCC warning.

>+/*
>+ * Eager 'this' coercion slow path. If we end up calculating 'this' eagerly,
>+ * purge the property cache since we don't perform eager 'this' coercion in
>+ * the property cache hit fast path.
>+ */

Most of this comment is stale now.

>+IsSafeForLazyThisCoercion(JSContext *cx, const Value &funval)

Any reason this isn't inline along with IsCacheableNonGlobalScope?

>+{
>+    JS_ASSERT(funval.isObject());

The parameter should be typed to make this asseriton unnecessary: JSObject *funobj. But we don't know it's a function, so JSObject *callee is best.

>+    /*
>+     * Look past any wrappers. If the callee is a strict function it is always
>+     * safe because we won't do 'this' coercion in strict mode. Otherwise
>+     * the callee is only safe to transform into the lazy this token (undefined)
>+     * if it is in the current scope. Without this restriction lazy 'this'
>+     * coercion would pick up the wrong global in the other scope.

Comment wrap-margin is random. Suggested revision:

    /*
     * Look past any wrappers. If the callee is a strict function it is always
     * safe because we won't do 'this' coercion in strict mode. Otherwise the
     * callee is only safe to transform into the lazy 'this' token (undefined)
     * if it is in the current scope. Without this restriction, lazy 'this'
     * coercion would pick up the wrong global in the other scope.
     */

>+     */
>+    JSObject *funobj = funval.toObject().unwrap();
>+    if (funobj->isFunction()) {
>+        JSFunction *fun = funobj->getFunctionPrivate();
>+        if (fun->isInterpreted() && fun->inStrictMode())
>+            return true;
>+    }
>+    return funobj->getGlobal() == cx->fp()->scopeChain().getGlobal();

I've argued we should return true if callee->isFunctionProxy() too. See comment 45 and especially comment 50. That suggests calling funobj->isProxy() around all the costly bits here:

    if (callee->isProxy()) {
        callee = callee->unwrap();
        if (callee->isFunction()) {
            JSFunction *fun = callee->getFunctionPrivate();
            return fun->isInterpreted() && fun->inStrictMode();
        }
        return true; // treat any non-wrapped-function proxy as strict
    }
    return callee->getGlobal() == cx->fp()->scopeChain().getGlobal();

/be
Comment 66 Andreas Gal :gal 2011-02-18 17:47:13 PST
Created attachment 513647 [details] [diff] [review]
patch
Comment 67 Brendan Eich [:brendan] 2011-02-18 17:50:55 PST
Comment on attachment 513647 [details] [diff] [review]
patch

>+IsSafeForLazyThisCoercion(JSContext *cx, JSObject *callee)
>+{
>+    /*
>+     * Look past any wrappers. If the callee is a strict function it is always
>+     * safe because we won't do 'this' coercion in strict mode. Otherwise the
>+     * callee is only safe to transform into the lazy 'this' token (undefined)
>+     * if it is in the current scope. Without this restriction, lazy 'this'
>+     * coercion would pick up the wrong global in the other scope.
>+     */
>+    if (callee->isProxy()) {
>+        callee = callee->unwrap();
>+        if (callee->isFunction()) {
>+            JSFunction *fun = callee->getFunctionPrivate();
>+            if (fun->isInterpreted() && fun->inStrictMode())
>+                return true;

Argh! This should just be:

..            return fun->isInterpreted() && fun->inStrictMode();

Otherwise you fall through to here:

>+        }
>+        return true; /* treat any non-wrapped-function proxy as strict. */

And it's as if you just always returned true for any proxy (including function wrappers).

r=me with this fixed *and* a test to cover it (tested to fail with this patch and pass with the one to commit)!

/be
Comment 68 Brendan Eich [:brendan] 2011-02-18 20:33:53 PST
Or to handle the case of a wrapped, interpreted non-strict function scoped by the same global as the reference base of the binding through which it was called:

    if (callee->isProxy()) {
        callee = callee->unwrap();
        if (!callee->isFunction())
            return true; // treat any non-wrapped-function proxy as strict

        JSFunction *fun = callee->getFunctionPrivate();
        if (fun->isInterpreted() && fun->inStrictMode())
            return true;
    }
    return callee->getGlobal() == cx->fp()->scopeChain().getGlobal();

/be
Comment 69 Andreas Gal :gal 2011-02-18 21:27:36 PST
http://hg.mozilla.org/tracemonkey/rev/4e085ba15d4c

Pushed with a test, but working on a better browser-based test, too.
Comment 70 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-19 07:37:53 PST
Can we get this merged over to mozilla-central? It's the last betaN+ hard blocker (woot!)
Comment 71 Brendan Eich [:brendan] 2011-02-19 13:07:41 PST
This needs more tests as requested in comment 68, which would have exposed something I did a poor job of catching by inspection during review. Followup patch with tests coming.

/be
Comment 72 Bill Gianopoulos [:WG9s] 2011-02-19 13:40:52 PST
(In reply to comment #71)
> This needs more tests as requested in comment 68, which would have exposed
> something I did a poor job of catching by inspection during review. Followup
> patch with tests coming.
> 
> /be

Exactly the right way to proceed, I think Beltzner was just trying to make it clear that this is the only blocker for 4.0b12, so once it passes all tests it needs to land on mozilla-central.  (and I ma sure you already figured that out, and I did not need to say that)
Comment 73 Chris B. 2011-02-19 15:12:11 PST
(In reply to comment #72)
> (In reply to comment #71)
> > This needs more tests as requested in comment 68, which would have exposed
> > something I did a poor job of catching by inspection during review. Followup
> > patch with tests coming.
> > 
> > /be
> 
> Exactly the right way to proceed, I think Beltzner was just trying to make it
> clear that this is the only blocker for 4.0b12, so once it passes all tests it
> needs to land on mozilla-central.  (and I ma sure you already figured that out,
> and I did not need to say that)

This isn't the last blocker any more.  Bug 635008 has been added, so now there are TWO left including this one.
Comment 74 Brendan Eich [:brendan] 2011-02-19 23:51:14 PST
Created attachment 513836 [details] [diff] [review]
followup fix and tests that deliver the goods

The tests suffixed b (for block) and c (for call) fail without the jsinterp.cpp part of the patch. We leak Block and Call objects bound to this (very evil).

The d (declenv, the named function expression's name binding rib) test is odd. It results in |this| binding to the sandbox instead of the lexical scope global. I will debug, but here's the patch.

/be
Comment 75 Brendan Eich [:brendan] 2011-02-20 00:02:21 PST
Comment on attachment 513836 [details] [diff] [review]
followup fix and tests that deliver the goods

Andreas asked for a separate blocking bug.

/be
Comment 76 Brendan Eich [:brendan] 2011-02-20 00:05:35 PST
Filed as blocking bug 635582.

/be
Comment 77 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-20 21:53:08 PST
Backed out of TM, turned OS X opt orange:

http://hg.mozilla.org/tracemonkey/rev/91cd576da2af

I think we should ship without this.
Comment 78 Andreas Gal :gal 2011-02-20 21:59:39 PST
We should probably merge this with bug 635582 since we backed both out.
Comment 79 Andreas Gal :gal 2011-02-21 12:16:11 PST
*** Bug 635582 has been marked as a duplicate of this bug. ***
Comment 80 Andreas Gal :gal 2011-02-21 16:17:44 PST
Created attachment 514091 [details] [diff] [review]
patch
Comment 81 Andreas Gal :gal 2011-02-21 17:05:41 PST
Created attachment 514101 [details] [diff] [review]
patch
Comment 82 Andreas Gal :gal 2011-02-22 11:56:25 PST
I think we can skip the beta for this. Restoring the original behavior will not cause breakage, minus unintended consequences.
Comment 83 dkwakkel 2011-02-22 12:00:32 PST
I would propose not to skip beta for this, because you never know how something is broken. That is one of the reasons you have beta software :-)
Comment 84 Chris B. 2011-02-22 12:26:11 PST
Please keep this bug on Final+.  Beta 12 Hard Blockers are finally at "Zarro" and needs to land.
Comment 85 Andreas Gal :gal 2011-02-22 15:05:42 PST
bent and I downloaded the patch from the tinderbox

http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/agal@mozilla.com-0bb527ae6134/tryserver-macosx64/

if we run the executable in 32-bit mode we can some weird ass crashes:

http://crash-stats.mozilla.com/report/index/bp-6f6ebf35-45b6-4014-ad4f-1b8632110222
http://crash-stats.mozilla.com/report/index/bp-96d18666-9c57-4268-9eda-44a072110222
http://crash-stats.mozilla.com/report/index/bp-41fc0394-d3c8-41ef-890d-975b52110222
http://crash-stats.mozilla.com/report/index/bp-2c075dca-839c-4774-bc55-0bb572110222

The first two stacks look like bug 509434

The crash we caught in gdb triggers __stack_chk_fail which points to a stack overflow.

This is going to be extremely painful to debug.
Comment 86 Andreas Gal :gal 2011-02-22 15:27:20 PST
--25756-- /Users/gal/Desktop/Crashing/Minefield.app/Contents/MacOS/components/libalerts_s.dylib:
--25756-- dSYM directory is missing; consider using --dsymutil=yes
--25756-- /Users/gal/Desktop/Crashing/Minefield.app/Contents/MacOS/components/libbrowsercomps.dylib:
--25756-- dSYM directory is missing; consider using --dsymutil=yes
==25756== Thread 1:
==25756== Invalid read of size 4
==25756==    at 0x110CBC7: js::Interpret(JSContext*, JSStackFrame*, unsigned int, JSInterpMode) (in /Users/gal/Desktop/Crashing/Minefield.app/Contents/MacOS/XUL)
==25756==  Address 0x8df029e8 is not stack'd, malloc'd or (recently) free'd
==25756==
Comment 87 Andreas Gal :gal 2011-02-22 16:13:59 PST
This more and more looks like a compiler bug.

(gdb) disass $pc-135 $pc+32
Dump of assembler code from 0x10f22a2 to 0x10f2349:
0x010f22a2 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77522>:	movl   $0x0,0x8(%esp)
0x010f22aa <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77530>:	mov    0x4fc4f9(%ebx),%eax
0x010f22b0 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77536>:	mov    %eax,0x4(%esp)
0x010f22b4 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77540>:	mov    0xd4(%esp),%ecx
0x010f22bb <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77547>:	mov    %ecx,(%esp)
0x010f22be <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77550>:	call   0x1056160 <JS_ReportErrorNumber>
0x010f22c3 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77555>:	mov    0x900(%esp),%eax
0x010f22ca <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77562>:	mov    %eax,(%esp)
0x010f22cd <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77565>:	call   0x13eec02 <dyld_stub_free>
0x010f22d2 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77570>:	mov    0xd4(%esp),%esi
0x010f22d9 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77577>:	mov    0xcc(%esi),%esi
0x010f22df <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77583>:	mov    %esi,0x7c(%esp)
0x010f22e3 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77587>:	mov    0x75c(%esp),%ebp
0x010f22ea <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77594>:	mov    0x610(%esp),%edi
0x010f22f1 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77601>:	jmp    0x10e0304 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+3892>
0x010f22f6 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77606>:	nopw   %cs:0x0(%eax,%eax,1)
0x010f2300 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77616>:	mov    0xd4(%esp),%ebp
0x010f2307 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77623>:	mov    0xcc(%ebp),%ebp
0x010f230d <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77629>:	mov    %ebp,0x7c(%esp)
0x010f2311 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77633>:	mov    0x75c(%esp),%ebp
0x010f2318 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77640>:	mov    0x610(%esp),%edi
0x010f231f <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77647>:	jmp    0x10e0304 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+3892>
0x010f2324 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77652>:	call   0x13eea5e <dyld_stub___stack_chk_fail>
0x010f2329 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77657>:	mov    0xd4(%esp),%eax
0x010f2330 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77664>:	mov    0xcc(%eax),%eax
0x010f2336 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77670>:	mov    %eax,0x7c(%esp)
0x010f233a <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77674>:	mov    0x75c(%esp),%ebp
0x010f2341 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77681>:	mov    0x610(%esp),%edi
0x010f2348 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+77688>:	jmp    0x10e0304 <_ZN2js9InterpretEP9JSContextP12JSStackFramej12JSInterpMode+3892>
End of assembler dump.

We are crashing in __stack_chk_fail somewhere near code that we really didn't change.

I pushed to try a new version of the patch that takes a hunk of code out of line that was inlined before. Keeping fingers crossed.
Comment 88 Andreas Gal :gal 2011-02-22 19:36:41 PST
Yep, this is a compiler bug. The patch that doesn't inline the path passes try. I will make a new version that tries to inline some part of the hot path and try that.

http://hg.mozilla.org/try/rev/2e7ec13c81ae
Comment 89 Andreas Gal :gal 2011-02-22 22:19:30 PST
Created attachment 514422 [details] [diff] [review]
patch
Comment 90 Andreas Gal :gal 2011-02-22 22:27:32 PST
http://hg.mozilla.org/tracemonkey/rev/b19abe19a212
Comment 91 Andreas Gal :gal 2011-02-22 22:27:59 PST
Comment on attachment 514422 [details] [diff] [review]
patch

Combined patch by gal/brendan, r=brendan/gal.
Comment 92 Luke Wagner [:luke] 2011-02-23 11:04:18 PST
Maybe I'm ignoring an invariant here, but if you have a strict mode function from a different global but *not* from a different compartment (hence not hitting the callee->isProxy() branch) then it seems like IsSafeForLazyThisCoercion will return false and the object will be pushed instead of undefined.
Comment 93 Andreas Gal :gal 2011-02-23 11:08:04 PST
Thanks for double checking luke. I guess we need a follow-up fix.
Comment 94 Luke Wagner [:luke] 2011-02-23 12:53:10 PST
Part of Waldo's mega-global-obj patch was a shell function that created a new global without putting it in a new compartment; that could be yoinked for easy testing in the shell.
Comment 95 Brendan Eich [:brendan] 2011-02-23 19:45:32 PST
(In reply to comment #94)
> Part of Waldo's mega-global-obj patch was a shell function that created a new
> global without putting it in a new compartment; that could be yoinked for easy
> testing in the shell.

Addressed in patch for bug 636364 -- thanks for catching this. I caught another (!) lack in this bug's patch, addressed there: needed methodjit changes.

/be
Comment 97 Jeff Walden [:Waldo] (remove +bmo to email) 2011-02-25 15:22:52 PST
Bug 634590 is at four regressions now: a methodjit regression in bug 636820, a (seemingly) tracing jit regression in bug 636795, existing tests that everyone agreed should have the previous behavior a year ago in bug 636364, and original-patch incompleteness in bug 635582.  Plus you can add a few backouts and relandings and tweaks for Mac compiler bugs stuff done on these issues has triggered (coincidentally, perhaps, but the fact remains).  If we fix all that, are we really sure we've fixed everything, and that we see the light at the end of the tunnel?  This small, simple fix has become anything but, or so it seems to me.
Comment 98 Brendan Eich [:brendan] 2011-02-25 16:58:30 PST
Waldo, counting Mac compiler workarounds is low, really low.

You're not tall enough to call this one. Give it a rest.

/be
Comment 99 anand 2011-03-27 23:10:35 PDT
Kudos and Congratulations FireFox 4 team !, Well done !.

Our framework and application loads minimum twice faster in FireFox 4  when compared to Chrome 10 and 11.

With this fix already there are growing number fans amongst enterprise users  enjoying FireFox 4 while using our product. 

Well done, Congratulations !
Comment 100 Christian Holler (:decoder) 2013-03-11 11:25:35 PDT
Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.

Note You need to log in before you can comment on or make changes to this bug.