ivan Reporter
	Description • 14 years ago

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`.

	ondra zara
	Comment 2 • 14 years ago

I believe this is the same problem as in my testcase at http://tmp.zarovi.cz/ff4-2.html ?

	Brendan Eich [:brendan] Assignee
	Comment 3 • 14 years ago

Can I have betaN+ on this one? Thanks,

/be

	ondra zara
	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.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 5 • 14 years ago

renominating based on comment 4

	Brendan Eich [:brendan] 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

	Igor Bukanov
	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?

	Igor Bukanov
	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.

	Igor Bukanov
	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.

	Brendan Eich [:brendan] 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

	Igor Bukanov
	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.

	Igor Bukanov
	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.

	Brendan Eich [:brendan] 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

	Igor Bukanov
	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.

	Brendan Eich [:brendan] 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

	Brendan Eich [:brendan] 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

	Igor Bukanov
	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.

	Igor Bukanov
	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();

	Brendan Eich [:brendan] 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

	Brendan Eich [:brendan] 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

	Brendan Eich [:brendan] 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

	Brendan Eich [:brendan] 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

	Brendan Eich [:brendan] 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

	Igor Bukanov
	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.

	Brendan Eich [:brendan] 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

	Igor Bukanov
	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.

	Igor Bukanov
	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();

	Igor Bukanov
	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.

	Brendan Eich [:brendan] 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

	Brendan Eich [:brendan] 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

	Igor Bukanov
	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.

	Igor Bukanov
	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.

	Igor Bukanov
	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();

	Brendan Eich [:brendan] 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

	Brendan Eich [:brendan] 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

	Brendan Eich [:brendan] Assignee
	Comment 38 • 14 years ago

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

/be

	Igor Bukanov
	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.

	Brendan Eich [:brendan] 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

	Chris Leary [:cdleary] (not checking bugmail)
	Comment 41 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/a6c636740fb9