468070 - New variation of primitive prototype resolution bug: foo['someProp']

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Marc]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.4) Gecko/2008111317 Ubuntu/8.04 (hardy) Firefox/3.0.4
Build Identifier: 

Attached is a patch demonstrating an other variation of the bug in primitive prototype resolution when many top scopes are involved.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Marc]

Attachment: Patch demonstrating and fixing the problem

Comment 2

[Hannes Wallnoefer]

I committed your patch, but without deprecating the old ScriptRuntime.getObjectElem() signature as it is still used in Codegen.

Comment 3

[Marc]

Thanks a lot

Comment 4

[Attila Szegedi]

Folks, in the patch to Codegen, shouldn't variableObjectLocal be used instead of thisObjectLocal?

Comment 5

[Marc]

From what I can guess from the variable names, thisObjectLocal makes more sense to me. This being said, the unit test pass as well when variableObjectLocal is used.

Do you have an example where you think that one of both would fail?

Comment 6

[Marc]

@Hannes:

do you plan to apply my patch in Rhino1_7R2_BRANCH too?

Comment 7

[Hannes Wallnoefer]

(In reply to comment #4)
> Folks, in the patch to Codegen, shouldn't variableObjectLocal be used instead
> of thisObjectLocal?

I was unsure about thisObjectLocal too. What convinced me was that the fix to bug #374918 uses the same approach (not really a good reason, I know :-). But yes, variableObjectLocal should make more sense here. I'll give it a short try here. 

(In reply to comment #6)
> @Hannes:
> 
> do you plan to apply my patch in Rhino1_7R2_BRANCH too?

Yes, if it's really important to you (as it seems to be).

Comment 8

[Attila Szegedi]

(In reply to comment #7)
> (In reply to comment #4)
> > Folks, in the patch to Codegen, shouldn't variableObjectLocal be used instead
> > of thisObjectLocal?
> 
> I was unsure about thisObjectLocal too. What convinced me was that the fix to
> bug #374918 uses the same approach (not really a good reason, I know :-). 

Hm... Maybe revisit that too, then?

Comment 9

[Hannes Wallnoefer]

Attachment: use variable object as scope argument

Patch to use variable object instead of this-object when we need to pass a scope argument. This affects the patches for this bug and bug 374918.

Comment 10

[Marc]

@both: can you imagine a unit test?

Comment 11

[Attila Szegedi]

(In reply to comment #10)
> @both: can you imagine a unit test?

I have a wild imagination, Marc.

Comment 12

[Marc]

(In reply to comment #11)
> I have a wild imagination, Marc.

I trust you Attila and therefore I'd like to see this test to improve my Rhino knowledge.

Comment 13

[Hannes Wallnoefer]

Marc: the variable object is by definition the top-most object in the current scope chain, so it makes more sense to use in this context than the this-object. I don't see a need to create an artificial test to make the original patch break. Committing the second patch. 

Checking in src/org/mozilla/javascript/optimizer/Codegen.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/optimizer/Codegen.java,v  <--  Codegen.java
new revision: 1.269; previous revision: 1.268
done

Comment 14

[Attila Szegedi]

(In reply to comment #12)
> (In reply to comment #11)
> > I have a wild imagination, Marc.
> 
> I trust you Attila and therefore I'd like to see this test to improve my Rhino
> knowledge.

Well, this ain't about Rhino, it's about two basic and distinct JS runtime concepts at the current point of execution: the identity of the "this" object, and the identity of the scope used for identifier resolution. In example:

var x = 1;
function f()
{
    var y = 2;
    print(x);
    print(y);
    print(this.x);
    print(this.y);
}

function obj() {
    this.y = 4;
    return this;
}
obj.prototype =  {
    x: 3,
    my_f: f
};

new obj().my_f();

===
prints:
1
2
3
4

Also, in the interpreter patch, you used "frame.scope" for the same parameter (correctly), and not "frame.thisObj" (which would've been incorrect), so by applying the same reasoning, you use localVariableObject (should really be called localScopeObject...) and not localThisObject in the generated compiled code.

Comment 15

[Marc]

Thanks both for the explanations. What about adding some comment on the variable in CodeGen?

@Hannes:
a test is never artificial when it allows to detect that something is incorrect. I understand your explanations (for my part I was just happy to hack successfully CodeGen) and don't contest this at all. But here we have something that was wrong until Attila's eyes detected it. I have no doubt that his view will stay sharp, but I believe that a unit test is better and more reliable than 1000 eyes. I'll try to propose a patch illustrating why thisObjectLocal was wrong.

Comment 16

[Hannes Wallnoefer]

I committed the last two patches to Rhino1_7R2_BRANCH.

Comment 17

[Attila Szegedi]

(In reply to comment #15)
> I'll try to propose a patch illustrating why thisObjectLocal was wrong.

Heh. I tried to make a test that breaks before the patches using this:

        Context cx = Context.enter();
        cx.setOptimizationLevel(0);
        ScriptableObject s1 = cx.initStandardObjects();
        ScriptableObject s2 = cx.initStandardObjects();
        cx.compileString("String.prototype.foo=1; function f1() { return String.prototype.foo; }", "", 1, null).exec(cx, s1);
        ScriptableObject.putProperty(s2, "f1", ScriptableObject.getProperty(s1, "f1"));
        System.out.println(cx.compileString("this.f1();", "", 1, null).exec(cx, s2));

Reasoning being that without the fix, this.f1() invocation would try to resolve String prototype against s2 ("this" in that invocation), and not the activation scope of f1 (which is a child of s1)). 

Turns out though that Codegen didn't even hit the point of the fix where it'd emit "getPropFunctionAndThis", as it emitted a call to OptRuntime.callProp0() instead :-(

So, my inability to come up with a test stems from the fact that I can't get Codegen to actually hit that code path... (and the fact that I can't spend more time on it right now).