Closed Bug 634590 Opened 14 years ago Closed 14 years ago

Unqualified function invocation doesn't use the global object the property was gotten from as |this|

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: tgvrs_santhosh, Assigned: gal)

References

Details

(Keywords: regression, testcase, Whiteboard: [hardblocker][has patch], fixed-in-tracemonkey)

Attachments

(4 files, 12 obsolete files)

Simple testcase
188 bytes, text/html
Details
shell testcase based on evalcx
159 bytes, text/plain
Details
Another, more interesting testcase
306 bytes, text/html
Details
patch
21.17 KB, patch
gal
: review+
Details | Diff | Splinter Review
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
Attached file ffTest (obsolete) —
Attached file ffTest2 (obsolete) —
It would help if you can find the first build where it broke. http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/
Keywords: regression, regressionwindow-wanted, testcase
OS: Windows 7 → All
Hardware: x86 → All
Hmm. That's the JM landing.
Component: General → XPConnect
QA Contact: general → xpconnect
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.
Assignee: nobody → general
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Component: XPConnect → JavaScript Engine
Ever confirmed: true
QA Contact: xpconnect → general
Summary: wrong window context in iframe. → Unqualified function invocation doesn't use the global object the property was gotten from as |this|
Attached file Simple testcase —
Attachment #512795 - Attachment is obsolete: true
Attachment #512796 - Attachment is obsolete: true
This is a dup.
Actually it may be a dup. Waldo?
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.
See comment 6. You can restrict your other-browser testing to IE. ;)
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
IE9 does what everyone did before September 11 last year. I'll take this.
Status: NEW → ASSIGNED
I don't think this should block until we have evidence that it breaks sites.
Status: ASSIGNED → NEW
blocking2.0: ? → -
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Its a regression, so we should discuss taking a reviewed and tested patch if we get one in time.
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
blocking2.0: - → ?
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
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
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.
Attached patch patch (obsolete) — — Splinter Review
(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
(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.
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
(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 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
(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
(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 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.
blocking2.0: ? → final+
Whiteboard: hardblocker
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?
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).
(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
Attached patch patch (obsolete) — — Splinter Review
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?
Attachment #512890 - Attachment is obsolete: true
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.
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.
tgvrs: could you elaborate on how your product is broken? ideally, with a URL or code sample that can be tested?
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.
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 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
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
(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.
...or earlier, of course, with this hack.
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.
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
I still disagree with our needing to fix this, or with it being a bug, so I shouldn't be the assignee.
Assignee: jwalden+bmo → general
Status: ASSIGNED → NEW
(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
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.
No unassigned blockers. /be
Assignee: general → gal
This is biting some content so it should be in the beta. I will try to finish the patch today.
blocking2.0: final+ → betaN+
(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.
(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
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.
Attached patch patch (obsolete) — — Splinter Review
Attachment #513008 - Attachment is obsolete: true
Attached patch patch (obsolete) — — Splinter Review
Attachment #513322 - Attachment is obsolete: true
Attachment #513323 - Flags: review?(brendan)
Attached patch patch (obsolete) — — Splinter Review
Attachment #513323 - Attachment is obsolete: true
Attachment #513323 - Flags: review?(brendan)
Attachment #513324 - Flags: review?(brendan)
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
Attachment #513324 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/b0aa9c20ffe4
Whiteboard: hardblocker → hardblocker, fixed-in-tracemonkey
(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
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?
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.
Whiteboard: hardblocker, fixed-in-tracemonkey → hardblocker
There is an assert that might be mine. Looking at it.
(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.
Whiteboard: hardblocker → [hardblocker][has patch][test failures?]
Whiteboard: [hardblocker][has patch][test failures?] → [hardblocker][needs new patch]
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.
Andreas and I talked and he's off the ledge. Should have a patch today. /be
Attached patch patch (obsolete) — — Splinter Review
Attachment #513324 - Attachment is obsolete: true
Attachment #513610 - Flags: review?(brendan)
Whiteboard: [hardblocker][needs new patch] → [hardblocker][has patch]
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
Attached patch patch (obsolete) — — Splinter Review
Attachment #513610 - Attachment is obsolete: true
Attachment #513610 - Flags: review?(brendan)
Attachment #513647 - Flags: review?(brendan)
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
Attachment #513647 - Flags: review?(brendan) → review+
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
http://hg.mozilla.org/tracemonkey/rev/4e085ba15d4c Pushed with a test, but working on a better browser-based test, too.
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch], fixed-in-tracemonkey
Can we get this merged over to mozilla-central? It's the last betaN+ hard blocker (woot!)
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
(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)
(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.
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
Assignee: gal → brendan
Status: NEW → ASSIGNED
Attachment #513836 - Flags: review?(gal)
Comment on attachment 513836 [details] [diff] [review] followup fix and tests that deliver the goods Andreas asked for a separate blocking bug. /be
Attachment #513836 - Attachment is obsolete: true
Attachment #513836 - Flags: review?(gal)
Depends on: 635582
Filed as blocking bug 635582. /be
Backed out of TM, turned OS X opt orange: http://hg.mozilla.org/tracemonkey/rev/91cd576da2af I think we should ship without this.
Whiteboard: [hardblocker][has patch], fixed-in-tracemonkey → [hardblocker][has patch]
We should probably merge this with bug 635582 since we backed both out.
Whiteboard: [hardblocker][has patch] → [hardblocker]
Attachment #513647 - Attachment is obsolete: true
Attached patch patch (obsolete) — — Splinter Review
Whiteboard: [hardblocker] → [hardblocker][has patch]
Attached patch patch (obsolete) — — Splinter Review
Attachment #514091 - Attachment is obsolete: true
blocking2.0: betaN+ → final+
I think we can skip the beta for this. Restoring the original behavior will not cause breakage, minus unintended consequences.
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 :-)
Please keep this bug on Final+. Beta 12 Hard Blockers are finally at "Zarro" and needs to land.
--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==
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.
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
Attached patch patch — — Splinter Review
Assignee: brendan → gal
Attachment #514101 - Attachment is obsolete: true
http://hg.mozilla.org/tracemonkey/rev/b19abe19a212
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch], fixed-in-tracemonkey
Comment on attachment 514422 [details] [diff] [review] patch Combined patch by gal/brendan, r=brendan/gal.
Attachment #514422 - Flags: review+
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.
Thanks for double checking luke. I guess we need a follow-up fix.
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.
Depends on: 636364
(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
http://hg.mozilla.org/mozilla-central/rev/b19abe19a212
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 636820
Depends on: 636795
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.
Waldo, counting Mac compiler workarounds is low, really low. You're not tall enough to call this one. Give it a rest. /be
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 !
Depends on: 653959
Blocks: 671947
Filter on qa-project-auto-change: Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: