Bug ID: JDK-8098578 Global scope is not accessible with indirect load call

Type: Bug
Component: core-libs
Sub-Component: jdk.nashorn

Priority: P3
Status: Resolved
Resolution: Fixed
OS: generic
CPU: generic

Submitted: 2015-06-15
Updated: 2015-09-29
Resolved: 2015-06-16

JDK 8	JDK 9
8u60Fixed	9 b70Fixed

If "load" builtin is called on global object or as a direct call, it uses global object as "this" for the script evaluated as well as global object as "scope" for evaluation.

But, if load is indirectly invoked with a different "this" then that object is used as "this" for evaluated code as well as "scope" for the evaluated code.

Example:

var obj = { foo: 44 };
load.call(obj, "foo = 40; var bar = 'hello'"); // obj.foo assigned!

Also, the evaluated code can not access global variables. Trying to access "Object" or "print" from evaluated script results in ReferenceError/TypeError (calling "undefined") etc. Also, all definitions in the evaluated code go as properties. In the above example, new "bar" property is created in obj.

 This is very different anything else in the language/API.

For eg. indirect "eval" call always uses global object as "this" as well as "scope" object for evaluated code. Direct "eval" uses surrounding scope and surrounding "this" -- but only in non-strict mode. Even in that case, function local scope objects are not accessible to user script. In strict mode, direct eval uses an intermediate "scope" object which is not accessible to user code directly. Even in the case of "with" statement, global is always accessible within the with block.

The "load-on-arbitrary-object" exposes a new kind of scope object to user code (apart from global object alone) - where global's properties are not available to user script but definitions from evaluated code go into a user accessible object.

We need to revisit this behaviour and also add tests.

I'm copying here a transcript of the review chat we had about this issue. Both Hannes and I ended up agreeing with Sundar's implementation, but we did consider implications and alternative ideas, so it's a valuable record of the process behind the thinking: Hannes: To be honest, I'm still not convinced that load is broken, and that this is very useful. (I'm also not convinced of the contrary) Sundararajan: @Hannes Idea is to avoid exposing anything like a real "scope" object to user script. Hannes: One technical note: I don't think the equivalent of load in the comment is correct. Sundararajan: + have global accessible always to any evaluated code Sundararajan: why? Hannes: function load(source) { eval(scriptfromSource) } would eval in the local function scope. Sundararajan: well, right. But there is nothing defined inside "load" function. So, effectively only global vars are accessible Hannes: I think it would be correct if load was called indirectly Hannes: But declarations will go to function scope. Attila: so you're saying that FunctionScope could take the role of the WithObject there Hannes: Yes, that's how this is normally done in JS, and that's part of the reason I'm not convinced about the necessity. Sundararajan: Actually, if you see the way require is implemented.. Sundararajan: it is by generating code like Sundararajan: (function(export) { <<module code here>> }) (exports) Sundararajan: so, global is accessible, all declarations into function scope... Sundararajan: because of the added code, URL association is lost (which is what Avatar folks complained) Sundararajan: now, indirectload can be used to implement in straightforward manner.. Hannes: Why? Declarations now go to the global object again, don't they? Sundararajan: and URL association with code is left intact. Sundararajan: no Sundararajan: with indirect load, declarations actually go into the intermediate scope object .. Sundararajan: which is inaccessible to user code.. exactly like Sundararajan: (function(scope, source) { with(scope) { eval(<load_code>) } })(scope, source) Sundararajan: (and you're right. I should remove equivalent code for direct load call. I'll do) Attila: So, couldn't we achieve the same result from code using FunctionScope instead of WithObject? I don't like WithObject; it's linkage is less efficient Sundararajan: if we use FunctionScope, all declarations will leak into global again.. Sundararajan: or you mean create additional FunctionScope instead of WithObject in load impl? Hannes: Can't we just use the passed scope object directly as scope? We're already setting scope flag. Hannes: Maybe set global as __proto__ Attila: I don't think it's a good idea to change an existing externally passed object for load Sundararajan: +1 Attila: yeah, I meant a FunctionScope instead of WithObject in the implementation Sundararajan: don't want to modify given object behind the scene.. Hannes: Yes, that's another issue. Thought so too. Sundararajan: @Attila can try that .. Attila: I'm - just like Hannes - also not entirely convinced we need this. Or that we don't need it. I guess I'm not seeing the whole picture, really Attila: Let's take this from the functional direction; Sundar yesterday posted the code snippet with the functional principles in the comment: // global is accessible. All declarations go into // intermediate inaccessible scope. "this" is user passed object // User's passed object's properties are accessible as variables Sundararajan: Two things I don't like with the current indirect load: Attila: So what'd be different if we did nothing? Global would not be accessible and declarations would go into the user-passed object, right? Sundararajan: 1) Global is inaccessible Sundararajan: 2) declarations go into user-passed object - only non-global object that behaves like scope and is accessible to user code. Sundararajan: Effectively this makes indirect load useless.. Sundararajan: Unless you set it's proto to be global and okay with scope-like-user-accessible object Attila: well, 2 doesn't, only 1 does I'd think�� Hannes: 1 could be fixed by setting scope.__proto__ = global, which isn't a terrible hack. Attila: or even var scope = Object.create(this); // "this" would be global Attila: (in non-strict mode, at least) Hannes: The only thing I'm a bit worried is that code may expect scopes to implement Scope interface, which contains methods needed by splitter. This may not be a good design decision after all Attila: Sundar is right that so far we had no user-accessible scopes beside Global Attila: I think we even rely on this in subtle ways in linker and codegen here and there Sundararajan: That is the past I'm uncomfortable Sundararajan: yes - possible correctness issues Sundararajan: THere is another simpler option.. Sundararajan: i.e, we say load always uses global Sundararajan: i.e., whatever you pass for "self" is ignored. Like loadWithNewGlobal does Sundararajan: and also like indirect "eval" does.. Attila: hm�� although those scopes were always lexical scopes and their members can be inferred from source code. In this case, this scope is effectively dynamic - it's passed as an argument to load Sundararajan: even WithObject is like that, right? Sundararajan: in that it is "dynamic".. Sundararajan: (eval inside "with" block..) Attila: yep, although "with" is less problematic as its effects at least are delineated lexically :-) Attila: eval, OTOH. With user-accessible scope object, we have the situation where scope properties can change arbitrarily while the code executes in the scope (at the end of the day, the scope object can contain a reference to itself as a property, and the code within load()ed script can exploit that) Attila: so much like eval Hannes: The logical candidate for a scope would really be a function scope. What if we provided a loadWithNewScope, which is like loadWithNewGlobal except it uses a new function scope with the current proto as parent? Sundararajan: not getting the last part Sundararajan: you mean uses current global as proto? Hannes: Basically the same behaviour as invoking a function that does an eval (codee is loaded into the function scope). Sundararajan: That is precisely my indirect load does now.. Sundararajan: i.e., you're saying let's make a new API with that. Sundararajan: (what to do with current load with indirect call? perhaps always use global?) Sundararajan: actually your loadWithNewScope is simpler in that there is no user provided object, right? Sundararajan: i.e., just a fresh object used inside impl.. Attila: how would we use that to implement e.g. require()? (If that's a goal?) Sundararajan: +1 Hannes: I just saw I misread your code, I thought you were actually modifying the user passed object, not the scope object. So yes, this is pretty similar to what you do. Hannes: Well you would have to return the scope, which is strange/inconsistent with other loads, so probably not a good idea after all Sundararajan: This message has been removed. Sundararajan: (got what you said. bit late :) ) Attila: hm. Maybe indeed should discourage the load.call(obj, ...) idiom. It ain't so useful now that I think of it. A load() that allows access to the enclosing lexical context would be better, though, sort-of how eval() works anyway, just with the ability to pass an URL instead of the source text Attila: so e.g. you could write Hannes: could we do a loadInCurrentScope() that behaves like eval()? Attila: function require(url) { var exports = {}; load(url); return exports; } and have the "load" provide current function scope to the loaded script Attila: of course, we'd need to treat such loadInCurrentScope the same as we treat eval Sundararajan: in other words, directEval like equivalent: directLoad Sundararajan: will receive proper scope. Sundararajan: and indirect load is like indirect eval - except for the source script? Attila: I think so�� Sundararajan: hmm.. Sundararajan: (and strict mode and all..) Attila: so we'd have to deal with all HAS_EVAL and HAS_DEEP_EVAL flags. Oh wait, would that mean loadInCurrentScope would not be allowed in strict mode? Attila: yep, exact same thinking :-) Sundararajan: :-) Attila: okay, another idea�� my main concern if we allow user-passed object in the scope chain is that its property membership can change. How about we create a new scope object to be a shallow copy of the passed-in user object and use that in the scope chain? Attila: we'd again only have user-inaccessible objects in the scope chain, but the user could pass in whatever they want initially into it, and because it'd be a shallow copy, they could still access the values after the load() completes Attila: (well, as long as the values are themselves objects, not primitives) Attila: but it'd work for require()-like implementation (sorry for being fixated on require(), it just seems to me like a good concrete example) Sundararajan: We don't actually allow user specified object in scope chain with my change.. Sundararajan: we only use as a "with" expression (and WithObject itself is the actual scope object in the chain) Attila: hm. You're right Sundararajan: (actually I too have require in mind all the time :) Good example) Attila: okay, so WithObject is then actually a good way to expose the properties of the passed-in object in a read/write fashion Attila: I'm getting slowly convinced Sundar's implementation is the way to go :-) Hannes: Well the more I think about it the better your solution looks, Sundar. hat functions declared within a with will be slow - they never can use fast scope. Sundararajan: that's true Attila: that's actually a bit of a weirdness�� we can figure out fast scope with "with" statement occurring lexically in the source code Hannes: Oh wait... that's not true, right? Attila: yeah - CodeGenerator operates on what's in the source code. This WithObject ain't in the source code :-) Sundararajan: Once "with" is seen, all via indirect access, right? Attila: nope Attila: I fixed that way back Attila: well, everything within "with" is via indirect access Attila: but Sundararajan: k (that's the part I meant) Attila: right :-) Attila: var x; // this is fast var y; // this is not with (z) { f(y); } Attila: but in this case, to the script the variables will appear as something defined externally anyway. I can just hope that we're emitting code that either links on the direct scope (and ends up being porto-linked) or walks the proto chain in code up to its own known top-level and does linking on that Attila: e.g. anything but direct linking to Global Attila: but if it did that, I'd think we would've seen it already in load() and eval() not working as expected�� Attila: yup, I just tried a small example and it does it on the scope Attila: so it should be fine Attila: alright, everything said and done I think I'm fine with the changes Sundararajan: :) Sundararajan: thx Hannes: Yes, me too. Attila: (well, with Hannes' comments about not having WithObject for direct load)

16-06-2015

Except for global scope object, no ScriptObject with IS_SCOPE flag set to true is accessible to user code. And only IS_SCOPE flagged objects receive definitions (var, function...) from any evaluated code as properties. It is better to restore that invariant for correctness (and possible optimization assumptions). Also, it's better if global scope is accessible to indirect-load evaluated code too - just like global scope is always accessible to any other evaluated code.

16-06-2015

Suggestions on http://cr.openjdk.java.net/~sundar/8098578/webrev.00/: * Comment typo in Context.java: "it's" -> "its" in "// make global to be it's __proto__" * the general description of the method (before "@param self" description) describes direct load behavior and the description of "@param self" discusses the indirect load. Maybe move both next to each other? E.g. both in method description or both in param description? I'd prefer them both to be in the method description. * global.newObject() is used as scope. This won't work when loaded script is "split" and that requires Scope implementing ScriptObject instance.

16-06-2015