Skip to content

Incorrect scope handling in LambdaFunction and LambdaConstructor. #2104

@aardvark179

Description

@aardvark179

Found this problem while cleaning up the property definition code.

The scope argument in Callable#call represents the caller's scope, not the callee's. So the interpreter and class compiler find the function's declaration scope and use that for lookup operations instead. Built in functions however do not do this. The actual Java code of the function does not have any information about its declaration scope, so the wrapping LambdaFunction needs to do the work instead, but does not currently do so.

  1. We pass some cross realm tests we should not. For example:
/*---
esid: sec-symbol.keyfor
description: Global symbol registry is shared by all realms
info: |
    The GlobalSymbolRegistry is a List that is globally available. It is shared
    by all realms. Prior to the evaluation of any ECMAScript code it is
    initialized as a new empty List.
features: [cross-realm, Symbol]
---*/

var OSymbol = $262.createRealm().global.Symbol;
var parent = Symbol.for('parent');
var child = OSymbol.for('child');

assert.notSameValue(Symbol.keyFor, OSymbol.keyFor);
assert.sameValue(OSymbol.keyFor(parent), 'parent');
assert.sameValue(Symbol.keyFor(child), 'child');

is passing not because we have a cross realm symbol registry, but because Symbol.for is using the caller's scope, not the function declaration's scope, and so looking up in the same realm both times. Similarly if we move the creation of property descriptor JS objects to Object.getOwnPropertyDescriptor then we start failing some test 262 tests because we are, again, executing in the caller's realm.

  1. We have some explicit tests that go against the spec, and I'm not sure if these are intentional, or aren't testing quite the right thing:
    2.1. DynamicScopeTest.standardMethodObjectCreate asserts various things about the prototypes of scopes, and the parent scopes of objects. Neither of those makes sense if objects cease to have parent scopes, and scopes don't have prototypes, so may not be a real concern.
    2.2. Bug783797Test.thisProto depends on either String.prototype being the same in all realms, or the callee scope not being set correctly. I think we're okay changing this test.
    2.3. NativeSetTest was passing because it only checked that this printed as [object Object]. It should have failed as this should have been globalThis, and we print that as [object Global], but it passed on other engines because they don't make that distinction. I've changed the test to check for globalThis explicitly, and it now passes on Rhino and other engines.
    2.4. PromiseBindTest failed for the same reason.
  2. I'm nervous about making this change until I can test it internally. I worry we may have something in our security setup that is going to depend on entirely the wrong behaviour here.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions