Closed
Bug 586482
Opened 14 years ago
Closed 14 years ago
arguments.callee.caller not equal to proto-delegated joined function object method
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla2.0
People
(Reporter: ivan, Assigned: brendan)
References
()
Details
(Keywords: regression, Whiteboard: hardblocker, fixed-in-tracemonkey)
Attachments
(2 files, 5 obsolete files)
267 bytes,
text/plain
|
Details | |
14.05 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre The test page "shows the prototype chain in action, where the method barrier is on the proto of the |this| object." Reproducible: Always Actual Results: In Firefox 4 nightlies, the page shows `false`. Expected Results: It should show `true`.
Keywords: regression
Assignee | ||
Updated•14 years ago
|
Assignee: general → brendan
Blocks: 577648
Status: UNCONFIRMED → ASSIGNED
blocking2.0: --- → ?
Ever confirmed: true
Priority: -- → P1
Summary: arguments.callee.caller not equal to method in FF 4 → arguments.callee.caller not equal to proto-delegated joined function object method
Target Milestone: --- → mozilla2.0b4
Assignee | ||
Comment 1•14 years ago
|
||
Comment 2•14 years ago
|
||
I believe this is the same problem as in my testcase at http://tmp.zarovi.cz/ff4-2.html ?
Assignee | ||
Comment 3•14 years ago
|
||
Can I have betaN+ on this one? Thanks, /be
Target Milestone: mozilla2.0b4 → mozilla2.0
Updated•14 years ago
|
blocking2.0: ? → betaN+
Updated•14 years ago
|
Comment 4•14 years ago
|
||
I am not sure what the recent status change exactly means, but please let me stress that this bug fatally *blocks* most of the JS code present for instance on mapy.cz, the biggest mapping provider here in Czech republic. It is, from my point of view, absolutely necessary to solve this issue prior to releasing FF 4.0.
Updated•14 years ago
|
blocking2.0: - → ?
Updated•14 years ago
|
blocking2.0: ? → betaN+
Assignee | ||
Comment 6•14 years ago
|
||
I'm being conservative here in looping up the proto chain, stopping on first shape found for the given method id. A non-native object that is encountered by js_LookupPropertyWithFlags stops that function's loop, and vectors off into the non-native's lookupProperty op. Likewise, a resolve hook on original thisp or an object in between the original thisp and the object in which the method is bound could dynamically define a shadowing property, but I force the method unjoining clone anyway. It's always safe to deoptimize when in doubt; not safe to optimize if in doubt! /be
Attachment #500312 -
Flags: review?(igor)
Comment 7•14 years ago
|
||
(In reply to comment #6) > Created attachment 500312 [details] [diff] [review] > proposed fix I am not sure about skipping the non-native objects in the prototype loop. I.e what if the caller does come from a get operation on the proxy found somewhere on the prototype chain?
Comment 8•14 years ago
|
||
(In reply to comment #7) > I am not sure about skipping the non-native objects in the prototype loop. I.e > what if the caller does come from a get operation on the proxy found somewhere > on the prototype chain? In particular, since we still allow to mutate __proto__ the proxy can mutate the prototype chain of the initial object, we need to deal with a prototype chain like a->proxy->b. That chain after the get on the proxy that returns an undefined would be be mutated into a->c while we get an uncloned method from b.
Comment 9•14 years ago
|
||
(In reply to comment #8) > In particular, since we still allow to mutate __proto__ the proxy can mutate > the prototype chain of the initial object, we need to deal with a prototype > chain like a->proxy->b. This is wrong as the prototype of the proxy is irrelevant for its get/has operations. In particular, if js_LookupPropertyWithFlagsInline finds a proxy on the prototype chain, then the resulting pobj is either NULL or the proxy itself. Thus js_GetPropertyHelperWithShapeInline would never a native object if LookupPropertyWithFlagsInline finds a proxy on the prototype object. Similarly XML objects on the prototype are also safe as LookupPropertyWithFlagsInline would return xml object only for them. Still to be safe I suppose js_GetPropertyHelperWithShapeInline must apply the read barrier unconditionally if the LookupPropertyWithFlagsInline returns a native object after it has found a non-native on the prototype. With this the loop in the patch can stop as long as it finds non-native on the prototype.
Assignee | ||
Comment 10•14 years ago
|
||
The joined method read barrier does not apply when getting a callee in a call expression (JSOP_CALLPROP -> js_GetMethod -> js_GetPropertyHelper), so no lookup helper need impose the barrier. Only on actual "get" do we need the barrier. The bug here is confined to JSStackFrame::getValidCalleeObject, which does not use any lookup helper. I'd rather not change it to do so and put more constraints on the general lookup path. This is a special case. We know the direct obj case works (that had a test when this code went in). The indirect (inherited from a proto) case needs fixing. But how to loop? Yes, a proxy handles all property has/get tests including up any proto chain it manages, but what if it does look back up into a native Object that has a joined method property? That is why I keep looking past natives in the loop in the patch. Worst case we clone a method unnecessarily (when the proxy is not actually going to search its proto). Does this make sense? /be
Comment 11•14 years ago
|
||
(In reply to comment #10) > The joined method read barrier does not apply when getting a callee in a call > expression (JSOP_CALLPROP -> js_GetMethod -> js_GetPropertyHelper), so no > lookup helper need impose the barrier. This assumes that js_LookupPropertyWithFlagsInline cannot mutate the prototype chain if it returns a native object. In general we do not have such guarantees and AFAICS GetMethod implementation must do the barrier read if it got a native object past a non-native on the prototype chain. > But how to loop? Yes, a proxy handles all property has/get tests including up > any proto chain it manages, but what if it does look back up into a native > Object that has a joined method property? Note that proxy implementation unconditionally returns itself from lookupProperty when it finds a property. But other non-native implementations may do the above lookup. And it may do that lookup on an arbitrary object unrelated to the prototype chain. So it is pointless to continue looping after a non-native is found on the protype chain.
Comment 12•14 years ago
|
||
(In reply to comment #11) > This assumes that js_LookupPropertyWithFlagsInline cannot mutate the prototype > chain if it returns a native object. In general we do not have such guarantees I meant: In general we do not have such guarantees after we found a non-native object on the prototype chain.
Assignee | ||
Comment 13•14 years ago
|
||
Ok, I'll revise to stop on first non-native. It's true an insane non-native could do anything, including make iloops or nonsense proto chains. But those would be bugs. /be
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #500312 -
Attachment is obsolete: true
Attachment #500471 -
Flags: review?(igor)
Attachment #500312 -
Flags: review?(igor)
Comment 15•14 years ago
|
||
Comment on attachment 500471 [details] [diff] [review] proposed fix, v2 >+ /* >+ * No point worrying about anything above a non-native object >+ * on the |this| object's prototype chain, as a non-native is >+ * responsible for handling its entire prototype chain. >+ */ >+ if (!thisp->isNative()) >+ break; To add teeth to the comment the patch should also assert that js_LookupPropertyWithFlagsInline never returns a native property after it finds non-native on the prototype. Otherwise we may end up with js_NativeGetInline called from js_GetMethod and js_GetPropertyHelperWithShapeInline mistakenly not applying the read barrier. r+ with that.
Attachment #500471 -
Flags: review?(igor) → review+
Updated•14 years ago
|
Whiteboard: hardblocker
Assignee | ||
Comment 16•14 years ago
|
||
(In reply to comment #15) > Comment on attachment 500471 [details] [diff] [review] > proposed fix, v2 > > >+ /* > >+ * No point worrying about anything above a non-native object > >+ * on the |this| object's prototype chain, as a non-native is > >+ * responsible for handling its entire prototype chain. > >+ */ > >+ if (!thisp->isNative()) > >+ break; > > To add teeth to the comment the patch should also assert that > js_LookupPropertyWithFlagsInline never returns a native property after it finds > non-native on the prototype. Dense arrays have a native prototype full of native properties. > Otherwise we may end up with js_NativeGetInline > called from js_GetMethod and js_GetPropertyHelperWithShapeInline mistakenly not > applying the read barrier. r+ with that. Dense arrays are saved by this code in array_getProperty circa line 765: if (!js_NativeGet(cx, obj, obj2, shape, JSGET_METHOD_BARRIER, vp)) So I must at least exclude dense arrays from the assertion. Comments? /be
Assignee | ||
Comment 17•14 years ago
|
||
Argh, the typed arrays (all of them) are like dense arrays. They are non-natives with native prototypes. E.g., Uint32Array.prototype is native and full of useful method and other properties: js> Object.getOwnPropertyNames(Uint32Array.prototype) ["constructor", "length", "byteLength", "byteOffset", "buffer", "slice", "set", "BYTES_PER_ELEMENT"] Again there's a saving if (!js_NativeGet(cx, obj, obj2, shape, JSGET_METHOD_BARRIER, vp)) in each typed array's obj_getProperty internal method. /be
Comment 18•14 years ago
|
||
(In reply to comment #17) > Argh, the typed arrays (all of them) are like dense arrays. So the initial patch with the continue would deal with such cases properly. This suggests to assert that when js_LookupPropertyWithFlagsInline gets a native result from the lookup call on a non-native object found on the prototype chain, then that result would belong to the prototype chain of that non-native. (In reply to comment #17) > Again there's a saving > > if (!js_NativeGet(cx, obj, obj2, shape, > JSGET_METHOD_BARRIER, vp)) > > in each typed array's obj_getProperty internal method. getProperty is not called when non-native is on the prototype of the native. We are saved because those non-natives cannot alter the prototype chain during the property lookup.
Comment 19•14 years ago
|
||
The patch is not enough to solve the problem as the script can modify or delete original functional property. So the following example (it delete the property before the caller is accessed) still asserts with the patch: var checkCaller = function(me) { var f = me['doThing']; delete MyObj.prototype['doThing']; var caller = arguments.callee.caller; var callerIsMethod = (f === caller); assertEq(callerIsMethod, true); }; var MyObj = function() { }; MyObj.prototype.doThing = function() { checkCaller(this); }; (new MyObj()).doThing();
Assignee | ||
Comment 20•14 years ago
|
||
(In reply to comment #18) > (In reply to comment #17) > > Argh, the typed arrays (all of them) are like dense arrays. > > So the initial patch with the continue would deal with such cases properly. > This suggests to assert that when js_LookupPropertyWithFlagsInline gets a > native result from the lookup call on a non-native object found on the > prototype chain, then that result would belong to the prototype chain of that > non-native. An #ifdef DEBUG block, yeah. > (In reply to comment #17) > > Again there's a saving > > > > if (!js_NativeGet(cx, obj, obj2, shape, > > JSGET_METHOD_BARRIER, vp)) > > > > in each typed array's obj_getProperty internal method. > > getProperty is not called when non-native is on the prototype of the native. It is called when the native is the prototype of the non-native, which is the case that a naive assert of the kind you initially suggested botches on. > We are saved because those non-natives cannot alter the prototype chain > during the property lookup. That's a saving grace in any scenario, but not relevant to the problem I raised with dense/typed arrays. (In reply to comment #19) > The patch is not enough to solve the problem as the script can modify or delete > original functional property. The existing code, prior to this bug's patch, tried to cope with that in general (but not for joined methods). > So the following example (it delete the property > before the caller is accessed) still asserts with the patch: > > var checkCaller = function(me) { > var f = me['doThing']; > delete MyObj.prototype['doThing']; > var caller = arguments.callee.caller; > var callerIsMethod = (f === caller); > assertEq(callerIsMethod, true); > }; > > var MyObj = function() { > }; > > MyObj.prototype.doThing = function() { > checkCaller(this); > }; > > (new MyObj()).doThing(); Thanks, I will add this in and try again. arguments.callee, blech! /be
Assignee | ||
Comment 21•14 years ago
|
||
> (In reply to comment #19)
> > The patch is not enough to solve the problem as the script can modify or delete
> > original functional property.
>
> The existing code, prior to this bug's patch, tried to cope with that in
> general (but not for joined methods).
The existing code got this right -- the patch clearly regresses it by transposing the if (thisp->hasMethodBarrier()) and if (shape) nested if's and failing to put this final last-ditch case:
- /*
- * If control flow reaches here, we couldn't find an
- * already-existing clone (or force to exist a fresh
- * clone) created via thisp's method read barrier, so
- * we must clone fun and store it in fp's callee to
- * avoid re-cloning upon repeated foo.caller access.
- * There are not any properties left referring to fun.
- */
inside the if (shape) {...} consequent.
/be
Assignee | ||
Comment 22•14 years ago
|
||
(In reply to comment #21) > > (In reply to comment #19) > > > The patch is not enough to solve the problem as the script can modify or delete > > > original functional property. > > > > The existing code, prior to this bug's patch, tried to cope with that in > > general (but not for joined methods). > > The existing code got this right Or "as right as it could". The test in comment 19 fails still, but the joined function object does not leak. This was an intentional state of affairs, not to call it a design. The cost we were trying to avoid (Waldo and I were in on this, at least) was a stack frame walk from the method read barrier, just in case there was an activation whose callee needed updating. I'm going to make this a known-fail testcase in the patch. /be
Assignee | ||
Comment 23•14 years ago
|
||
(In reply to comment #22) > The cost we > were trying to avoid (Waldo and I were in on this, at least) was a stack frame > walk from the method read barrier, just in case there was an activation whose > callee needed updating. Besides the stack frame walk potentially being costly, nesting re-entries of the VM from native code via the JS API could leave stack chains in the tree of stacks unreachable. We definitely don't want to enumerate all frames! This convinced me to break the hard case -- intentional fail for testcase 3. /be
Assignee | ||
Comment 24•14 years ago
|
||
Igor, if you have a better idea for making the regress-586482-3.js testcase not be a "fails" one, I'm all ears, but I'd rather it happen in a followup bug. That testcase always failed since JSStackFrame::getValidCalleeObject existed. /be
Attachment #500471 -
Attachment is obsolete: true
Attachment #501225 -
Flags: review?(igor)
Comment 25•14 years ago
|
||
(In reply to comment #20) > arguments.callee, blech! The callee is innocent! The problem is with the caller. (In reply to comment #24) > if you have a better idea for making the regress-586482-3.js testcase not > be a "fails" one, I'm all ears, but I'd rather it happen in a followup bug. The non-blocking follow up is fine as long as the fix is sufficient for web compatibility.
Assignee | ||
Comment 26•14 years ago
|
||
Function.caller, even worse! :-P No web compat issues known. Could walk the stack from delete of a property for which shape->isMethod() in an object that hasMethodBarrier(). Followup fodder, I will file tomorrow and cite in that FIXME and get rid of the ?. /be
Comment 27•14 years ago
|
||
Comment on attachment 501225 [details] [diff] [review] proposed fix, v3 >diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp >--- a/js/src/jsfun.cpp >+++ b/js/src/jsfun.cpp >@@ -1421,70 +1421,81 @@ JSStackFrame::getValidCalleeObject(JSCon >+ >+ do { ... >+ if (thisp->hasMethodBarrier()) { ... >+ JSObject *newfunobj = CloneFunctionObject(cx, fun, fun->getParent()); >+ if (!newfunobj) >+ return false; >+ newfunobj->setMethodObj(*thisp); >+ calleeValue().setObject(*newfunobj); >+ return true; > } >+ } while ((thisp = thisp->getProto()) != NULL); The cloning should be done after the loop in case we get some unrelated object with a barrier on the proto chain. r+ with that fixed.
Attachment #501225 -
Flags: review?(igor) → review+
Comment 28•14 years ago
|
||
Here is a regression test for the issue from the comment 27: var obj = { f: function() { var g = this.g; assertEq(arguments.callee.caller, g); print("Ok"); }}; var obj2 = { __proto__: obj, g: function() { this.f(); }}; var obj3 = { __proto__: obj2, h: function() { this.g(); }}; var obj4 = { __proto__ : obj3 } obj4.h();
Comment 29•14 years ago
|
||
Yet another issue is that even with the patch the code would leak the compiled function object if fun->methodAtom() is null. I suppose the assumption is that we cannot have the join issue with nameless functions. If so we need an assert enforcing that.
Assignee | ||
Comment 30•14 years ago
|
||
(In reply to comment #29) > Yet another issue is that even with the patch the code would leak the compiled > function object if fun->methodAtom() is null. I suppose the assumption is that > we cannot have the join issue with nameless functions. If so we need an assert > enforcing that. This is not a problem: fun->methodAtom() is not the same as fun->atom -- it's a special member set only for joined-method-optimized function objects. Thanks for the review, I will add the test in comment 28 as well. /be
Assignee | ||
Comment 31•14 years ago
|
||
One more time. This sucks, your testcase made me sample the first object with a method barrier encountered on thisp or its proto chain and use that as the method object (tested by hasMethodObject, see patch and pre-existing code). We're really guessing that the delete was from the closest barriered object on the prototype chain starting at|thisp|. Followup filed and cited. /be
Attachment #501225 -
Attachment is obsolete: true
Attachment #501553 -
Flags: review?(igor)
Assignee | ||
Comment 32•14 years ago
|
||
Attachment #501553 -
Attachment is obsolete: true
Attachment #501592 -
Flags: review?(igor)
Attachment #501553 -
Flags: review?(igor)
Comment 33•14 years ago
|
||
(In reply to comment #31) > We're really guessing that the delete was from the closest barriered object on > the prototype chain starting at|thisp|. Followup filed and cited. The situation is even worth since the script can mutate the prototype chain for the object. In that case the object will be completely unrelated to the original caller.
Comment 34•14 years ago
|
||
Comment on attachment 501592 [details] [diff] [review] proposed fix, v5 >diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp >--- a/js/src/jsfun.cpp >+++ b/js/src/jsfun.cpp >@@ -1434,70 +1434,85 @@ JSStackFrame::getValidCalleeObject(JSCon > * atom by which it was uniquely associated with a property. > */ > const Value &thisv = functionThis(); > if (thisv.isObject()) { > JS_ASSERT(funobj.getFunctionPrivate() == fun); > > if (&fun->compiledFunObj() == &funobj && fun->methodAtom()) { What if fun->methodAtom() is null here? Wouldn't we leak the compile-time object? >+ /* >+ * At this point, we couldn't find an already-existing clone (or >+ * force to exist a fresh clone) created via thisp's method read >+ * barrier, so we must clone fun and store it in fp's callee to >+ * avoid re-cloning upon repeated foo.caller access. >+ */ >+ JSObject *newfunobj = CloneFunctionObject(cx, fun, fun->getParent()); >+ if (!newfunobj) >+ return false; >+ newfunobj->setMethodObj(*first_barriered_thisp); first_barriered_thisp can be null here as the code can change the prototype chain before accessing the caller so there could be no objects with the barrier. I guess we can just throw in this case.
Attachment #501592 -
Flags: review?(igor)
Comment 35•14 years ago
|
||
Here is a test case for that proto-mutating case: function check() { obj2.__proto__ = null; return arguments.callee.caller; } var obj = { f: function() { check(); }}; var obj2 = { __proto__: obj }; obj2.f();
Assignee | ||
Comment 36•14 years ago
|
||
Oops, easily fixed. In such an absurd case (mutable __proto__ outside of object initialisers), we will just have to leak the joined function object. /be
Attachment #501592 -
Attachment is obsolete: true
Attachment #501674 -
Flags: review?(igor)
Updated•14 years ago
|
Attachment #501674 -
Flags: review?(igor) → review+
Assignee | ||
Comment 37•14 years ago
|
||
(In reply to comment #34) > Comment on attachment 501592 [details] [diff] [review] > proposed fix, v5 > > >diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp > >--- a/js/src/jsfun.cpp > >+++ b/js/src/jsfun.cpp > >@@ -1434,70 +1434,85 @@ JSStackFrame::getValidCalleeObject(JSCon > > * atom by which it was uniquely associated with a property. > > */ > > const Value &thisv = functionThis(); > > if (thisv.isObject()) { > > JS_ASSERT(funobj.getFunctionPrivate() == fun); > > > > if (&fun->compiledFunObj() == &funobj && fun->methodAtom()) { > > What if fun->methodAtom() is null here? Wouldn't we leak the compile-time > object? No, compile-and-go means it is fine to share a compile-time top-level function object. The fun->methodAtom() test is exactly and only to cover the joined method case, since only that case (see JSOP_LAMBDA in jsinterp.cpp) sets this novel function member. /be
Assignee | ||
Comment 38•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/a6c636740fb9 /be
Whiteboard: hardblocker → hardblocker, fixed-in-tracemonkey
Comment 39•14 years ago
|
||
Regarding the frame search loop - since we leak compile-time objects in any case perhaps we should just skip the frame searching in js_DeleteProperty? Also that loop does not deal with suspended generators, but I am not sure yet if there is any issues with that.
Assignee | ||
Comment 40•14 years ago
|
||
(In reply to comment #39) > Regarding the frame search loop - since we leak compile-time objects in any > case perhaps we should just skip the frame searching in js_DeleteProperty? Also > that loop does not deal with suspended generators, but I am not sure yet if > there is any issues with that. The loop in js_DeleteProperty pre-exists and I don't want to remove it. It fixes the naive testcase you wrote (thanks! I suspect there's another test from when I wrote that loop in js_DeleteProperty, haven't tracked it down). We're "better" but not "perfect". How we roll. Short of ES5 strict and Harmony (based on it), we are not going to be perfect. /be
Comment 41•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a6c636740fb9
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•