dj_eval not evaluating in the global window context in IE

[smorais@…]

I am using dojo.require to include a custom module outside of the dojo source tree. In this module, I declare a global (window scoped) variable like so:

var FOO = "bar";

dojo.require will then do an xmlhttprequest to fetch the context of my module and eval it using dj_eval. The problem is that in IE, window.eval will not evaluate the script in the context of the window, but rather in the context of the calling function, in this case dj_eval. So FOO will never be visible outside of my module. A workaround is to declare global variables in required modules like so:

FOO = "bar";

or like so:

window.FOO = "bar";

but this is a subtelty that will likely be lost on most module developers. A better solution will be to fix dj_eval to work right in IE:

function dj_eval(s){

if (dj_global.execScript){

return dj_global.execScript(s, "javascript");

} else if (dj_global.eval){

return dj_global.eval(s);

} else {

return eval(s);

}

}

[dylan]

is this valid? if so, it's an easy fix.

[guest]

window.execScript is a JScript only function. It evaluates a script and it's placed in the global scope. Frankly I'm surprised it's not being used by this toolkit already.

[jkuhnert]

I'll happily re-open this ticket if I can find an example of where this is an actual bug. I've tried various versions of calling execScript and can't currently get a lot of the standard widget tests to execute properly with this new scope. I'm assuming that this is because some of the code relies on the old scope being used when evaluating certain portions of code.

A lot of this is probably "over my head". Does your var FOO variable from the example correctly resolve to the scope of dj_global? That is to say, after loading your module can you see the value by calling dj_global.FOO ?

[jdespatis@…]

Hi,

I've had the same problem, in I get a javascript array thanks to dojo.io, i want to eval the latter in order to use it, but it doesn't work on IE (it works on FF)

i've simplified the problem in order to not use dojo.io, and i get again the problem:

If i do:

dj_eval("var foo = new Array(-1, 'plop');"); alert(foo[0]);

it work on FF but not on IE

Someone on IRC (liucougar) has modified it to make it work, just delete the 'var'

dj_eval("foo = new Array(-1, 'plop');"); alert(foo[0]);

I dont know if this is a bug..., but i hope it'll help

Cheers

[jburke]

(In [6731]) References #744: test for verifying that there is an issue.

[jburke]

Reopening with a test case (see previous comment).

[jburke]

Found some more info here: http://nerd.newburyportion.com/2006/07/going-global

I don't think using setTimeout() will work for the non-IE case, since we want synchronous loading and execution. And from what I can tell, we only have the issue with MSIE.

[jburke]

I changed dj_eval to this:

	if(dj_global.execScript){
		return dj_global.execScript(scriptFragment);
	}else{
		return dj_global.eval ? dj_global.eval(scriptFragment) : eval(scriptFragment); 	// mixed
	}

That seems to fix MSIE, but Safari 2.0 fails the test. I'll have to investigate more to see if there is a good solution for Safari. FF 2.0 and Opera 9 work fine.

[jburke]

Some notes from Tom: eval on MSIE might take a second arg that is scope. Also might something like using (new Function(script)).apply(dj_global). Or something like that, I closed the conversation before copying the right piece of it.

[rmoore]

execScript evaluates in global scope and returns NULL, it may be good to post a comment in the source so that someone doesn't expect dj_eval() to consistently return a value as eval() does.

[jburke]

Moving up to 0.9 beta, but may move it back to 0.9 if I run out of time.

[jburke]

(In [9260]) Refs #744. Test for the window.eval issue in IE and Safari.

[jburke]

Moving this to 0.9, but I do not see a good fix for this. Problems with window.execScript:

• Does not solve the Safari 2 issue (have not tried Safari 3 yet, but not optimistic)
• As rmoore points out, execScript does not return a value, so we cannot use it for evaling things like a json string.

Some interesting stuff in this test file: http://trac.dojotoolkit.org/browser/dojo/trunk/tests/_base/_loader/744/foo/bar.js?rev=9260

Notice that

barMessage = "It Worked"; 

and

getBar2Message = function(){...}

both work fine with window.eval in MSIE, but this fails:

function getBarMessage() {...}

This points to the difficulty of the problem -- it seems like a lexical scope issue, something that won't be helped with either one of these:

(new Function(scriptText).apply(window));
with(window){....}

(both of which were tried and did not work).

So I do not see a good generic solution to this issue. The only option seems to use a separate function to handle dojo.require() path, and leave dojo.eval as it is, for use in json operations. However I do not like it since it does not fix Safari, and thinking about it more, the issue will show up in xdomain builds, since the module code is wrapped in a function (so we do not execute the code until we resolve dependencies.

I am tempted to close this issue as wontfix.

[jburke]

Very tempted to close, but will push it to 1.0 for a final discussion.

[guest]

rmoore here again...

Can I suggest that your "only option" sounds like a good answer? There's no reason to return a value for eval within a #require, it's inefficient and un-necessary. For JSON you probably - very specifically - DON'T want to evalute in a global scope, just to mitigate any (possible?) script injection attacks.

http://trac.dojotoolkit.org/ticket/236 should fix #require problems for Safari window.execScript will fix #require for IE. Tomorrow I'll try to paste our version of function_eval() that includes both of these, but I think it uses some deprecated code (dojo.browser.???).

Do either of these make a difference for XD builds? If this is broken everywhere except FF/Op, does it hurt to make these changes so that at least it works everywhere for non-XD builds?

[guest]

This is the function that we use:

function dj_eval(/*String*/ scriptFragment, /*bool*/forceGlobal){ 
  // summary: Perform an evaluation in the global scope.  Use this rather than calling 'eval()' directly.
  // description: Placed in a separate function to minimize size of trapped evaluation context.
  // note:
  //  - JSC eval() takes an optional second argument which can be 'unsafe'.
  //  - Mozilla/SpiderMonkey eval() takes an optional second argument which is the
  //    scope object for new symbols.

  // Fix to evaluate in global scope for IE
  if (forceGlobal && window.execScript) {
    window.execScript(scriptFragment);
    return 1; // or just return?
  } 
  if (forceGlobal && dojo.render.html.safari) {
    // From: http://trac.dojotoolkit.org/ticket/236
    var script = document.createElement('script');
    var content = document.createTextNode(scriptFragment);
    script.appendChild(content);
    script.type = 'text/javascript';
    script.defer = false;
    var head = document.getElementsByTagName('head').item(0);
    head.appendChild(script);
    return 1; // or just return?
  }
  return dj_global.eval ? dj_global.eval(scriptFragment) : eval(scriptFragment); 	// mixed
}

forceGlobal was added so the default behavior for JSON/etc is to do dj_eval(string) as normal, but then in #require we can add in dj_eval(string, true) to force it global. This way it behaves exactly as before in the default case, but a global eval is available if required.

As I said before, this is based on the 0.3.1 source. The implementation has changed "a little" since then, so some of this would have to be rewritten.

[jburke]

dojo.requireLocalization() depends on a return value from dojo._loadPath() (which backs dojo.require(), so I do not think the Safari option above is workable.

Also, in the near future we will want to support multiple versions of dojo running in a page. To do that, we will most likely need to wrap any dojo.require'd code inside an anonymous function, to set the dojo/dojox/dijit/whatever variables to the right value.

I can see this anonymous function closures being used by default, so in that case, we will get consistent behavior across browsers: those top level functions will not longer be in the global space.

I also do not think it is good programming practice to expose globals in module code. It should be done explicitly, binding explicitly to window.

If global code is wanted, without the needs of going through dojo.require(), dojo.io.script can be used to pull it in. There is no dependency resolution, and it is asynchronous though.

Closing this is as wontfix, given the above, but I appreciate this is a complex issue. More input is welcome.

[mikewse]

1) Long time has passed since this was wontfix:ed, and sometimes I keep hitting places where I would have liked Dojo to use the global scope execScript fix. Would it be interesting to revisit this topic and evaluate in today's context, with Safari 2 support in principle uninteresting but many more years of IE7 to come?

2) Also, I'd like to discuss a related topic:

dojo.eval("this.abc=123");
->
abc == undefined
dojo.abc == 123

(This affects dojo.required code containing "this" assignments that normally address the global object.) Is that discussion best held in this ticket, a new ticket, or on dojo-contributor?

[jburke]

If there is a solution that works across the browsers we support (Firefox 3+, IE 6+, at least Safari 3, and ideally the latest Opera), works with dojo.requireLocalization, works with the closures that are used for multiversion/xdomain support, and is not a very big solution, then it can be handled in the ticket as a patch.

I think that is a pretty high bar to reach though. If you need it for a specific project that does not care about Safari/WebKit browsers, then you could monkey-patch in execScript for the specific app.

[mikewse]

I'll have a look. Could you outline the difficult requirements just a little bit? I e, how would the code look that simulates the multiversion support?

And what about the lack of a return value from execScript; is that a show-stopper or did you do some workaround in your previous tests?

Btw, I don't seem to find any unit tests for this area (eval scopes), is that correct?

[jburke]

The multiversion code wraps all dojo.required calls in a function wrapper like so:

(function(dojo, dijit, dojox){
...
})(dojo, dijit, dojox);

Something similar happens in the module.xd.js files for xdomain builds.

dojo.requireLocalization() internally depends on a return value from dojo._loadPath() (which backs dojo.require()) so we need a return value.

There are no tests for eval scopes.

[mikewse]

Thanks, James.

Regarding:

(function(dojo, dijit, dojox){
...
})(dojo, dijit, dojox);

What part of this is put through eval, and how are the actual dojo/dijit/dojox transferred into it?

[mikewse]

Right, I have now looked at the mentioned browsers and Safari 3 is the show-stopper just as you suggested. I didn't realize WebKit? was suffering from the same bug as IE, but without an execScript replacement. FWIW, Chrome 2 seems to fix this bug so I expect Safari 4 to also work the way we want.

Safari 4 was released a couple of weeks ago so when Safari 3 will be considered unsupported (but IE6/7 is) I guess something could be done with execScript for cases that don't need a return value, but apart from that I agree this ticket seems pretty hard to take forward...