fix missing scope definition at some places

[rbri]

next try to fix #934 (this replaces #1226)

[p-bakker]

LGTM

[tuchida]

Thanks! Looking good for now.

However, I felt that using parentScope here was not a good idea.

var f = function() {};
f.prototype.__parent__; // null before fix, [object global] after
f.prototype.__parent__ = null;
Object.getOwnPropertyDescriptor(f.prototype, 'constructor').hasOwnProperty('value'); // TypeError

This Pull Request is correct and necessary, but it may be better not to use parentScope apart from that. Since scope is used to get TopLevel.Builtins.Object, it looks to me like cx.topCallScope is sufficient.

[rbri]

Since scope is used to get TopLevel.Builtins.Object, it looks to me like cx.topCallScope is sufficient.

@tuchida Not sure i got your point - do you talk about

obj.setParentScope(getParentScope()); 

inside setupDefaultPrototype()?

[tuchida]

This approach also seems to solve the problem.

diff --git a/src/org/mozilla/javascript/ScriptableObject.java b/src/org/mozilla/javascript/ScriptableObject.java
index 46e5aaeb..f1f45a24 100644
--- a/src/org/mozilla/javascript/ScriptableObject.java
+++ b/src/org/mozilla/javascript/ScriptableObject.java
@@ -2651,8 +2651,7 @@ public abstract class ScriptableObject
     protected ScriptableObject getOwnPropertyDescriptor(Context cx, Object id) {
         Slot slot = querySlot(cx, id);
         if (slot == null) return null;
-        Scriptable scope = getParentScope();
-        return slot.getPropertyDescriptor(cx, (scope == null ? this : scope));
+        return slot.getPropertyDescriptor(cx, cx.topCallScope);
     }
 
     protected Slot querySlot(Context cx, Object id) {

[rbri]

@tuchida ok, is see, will test and add a commit

[p-bakker]

If we're deriving the scope from the context, there's not much point in passing it into slot getPropertyas we're also passing in context, no?

But are we 100% sure that it is always correct to read the top level scope from the context and not derive it from the Scriptable?

[rbri]

If we're deriving the scope from the context, there's not much point in passing it into slot getPropertyas we're also passing in context, no?

Have seen this but i was not sure if we should stay with the signature for backward compatibility.

But are we 100% sure that it is always correct to read the top level scope from the context and not derive it from the Scriptable?

Had the same question in mind but as long we have no test case that proves this i think both solutions are correct - so i did the commit because the code is simpler (from my point of view).

[p-bakker]

Had the same question in mind but as long we have no test case that proves this i think both solutions are correct - so I did the commit because the code is simpler (from my point of view).

The testcases might be in test262, but not enabled/failing as we're missing part of the required testing infrastructure, see #1077

In the browser I can test the cross-realm scripting using iframes:

// Comparing the __proto__ of a property descriptor of an object from another realm with one from the current realm.
// Yields false, as the the frame ('myframe') has it's own instances of all the global JavaScript objects, like `Object`
let realm = window.myframe;
realm.Object.getOwnPropertyDescriptor(realm.Object, 'getOwnPropertyDescriptor').__proto__ === ({}).__proto__;

realm = window;
// Same check as before, but now all within the same realm, yields true
realm.Object.getOwnPropertyDescriptor(realm.Object, 'getOwnPropertyDescriptor').__proto__ === ({}).__proto__;

The above 'testcase' indicates (I think) that in EcmaScript the lookup of the instance of Object to use for creating for example property descriptors should be based not on the Object instance of the current realm, but on scope in which the object is created on which the getOwnPropertyDescriptor method is called.

The following code with Rhino behaves differently when using cx.topCallScope vs. using the scriptable as scope:

      Context ctx = Context.enter();
      ctx.setLanguageVersion(200);
      Scriptable scriptable = ctx.initStandardObjects();
      Scriptable realm = ctx.initStandardObjects();
      scriptable.put("realm", scriptable, realm);

      String expression = """
        realm.Object.getOwnPropertyDescriptor(realm.Object, 'getOwnPropertyDescriptor').__proto__ === ({}).__proto__;
      """;
      Object result = ctx.evaluateString(scriptable, expression, "<cmd>", 0, null);

      System.out.println(result);

[p-bakker]

Created a (draft) PR for adding the missing infr, see #1229

[p-bakker]

The codechange that @tuchida proposed in #1227 (comment) (using return slot.getPropertyDescriptor(cx, cx.topCallScope); wasn't the correct one, but I think the code in that area can be simplified: the getParentScope() call can be removed and instead just call return slot.getPropertyDescriptor(cx, this);

[rbri]

I think we really have to squash the commits for this pr :->

[p-bakker]

LGTM now

[gbrail]

LGTM for me as well. Thanks for tracking this down!