Opened 14 years ago

Closed 13 years ago

Last modified 11 years ago

#744 closed defect (wontfix)

dj_eval not evaluating in the global window context in IE — at Version 21

Reported by: [email protected] Owned by: James Burke
Priority: high Milestone: 1.0
Component: General Version: 0.3
Keywords: Cc: [email protected]
Blocked By: Blocking:

Description (last modified by James Burke)

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);

}

}

Change History (21)

comment:1 Changed 14 years ago by dylan

Milestone: 0.4
Owner: changed from anonymous to alex

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

comment:2 Changed 14 years ago by 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.

comment:3 Changed 14 years ago by jkuhnert

Owner: changed from alex to jkuhnert

comment:4 Changed 14 years ago by jkuhnert

Cc: [email protected] added
Resolution: wontfix
Status: newclosed

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 ?

comment:5 Changed 14 years ago by [email protected]

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

comment:6 Changed 14 years ago by James Burke

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

comment:7 Changed 14 years ago by James Burke

Resolution: wontfix
Status: closedreopened

Reopening with a test case (see previous comment).

comment:8 Changed 14 years ago by James Burke

Owner: changed from jkuhnert to James Burke
Status: reopenednew

comment:9 Changed 14 years ago by James Burke

Milestone: 0.40.5

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.

comment:10 Changed 14 years ago by James Burke

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.

comment:11 Changed 14 years ago by James Burke

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.

comment:12 Changed 13 years ago by 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.

comment:13 Changed 13 years ago by James Burke

Milestone: 0.90.9beta
Status: newassigned

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

comment:14 Changed 13 years ago by James Burke

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

comment:15 Changed 13 years ago by James Burke

Milestone: 0.9beta0.9

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.

comment:16 Changed 13 years ago by James Burke

Milestone: 0.91.0

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

comment:17 Changed 13 years ago by 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?

comment:18 Changed 13 years ago by 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.

comment:19 Changed 13 years ago by James Burke

Resolution: wontfix
Status: assignedclosed

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.

comment:20 Changed 11 years ago by Mike Wilson

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?

comment:21 Changed 11 years ago by James Burke

Description: modified (diff)

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.

Note: See TracTickets for help on using tickets.