Opened 13 years ago

Closed 12 years ago

Last modified 10 years ago

#744 closed defect (wontfix)

dj_eval not evaluating in the global window context in IE

Reported by: smorais@… Owned by: James Burke
Priority: high Milestone: 1.0
Component: General Version: 0.3
Keywords: Cc: smorais@…
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 (37)

comment:1 Changed 13 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 13 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 13 years ago by jkuhnert

Owner: changed from alex to jkuhnert

comment:4 Changed 13 years ago by jkuhnert

Cc: smorais@… 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 13 years ago by 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

comment:6 Changed 13 years ago by James Burke

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

comment:7 Changed 13 years ago by James Burke

Resolution: wontfix
Status: closedreopened

Reopening with a test case (see previous comment).

comment:8 Changed 13 years ago by James Burke

Owner: changed from jkuhnert to James Burke
Status: reopenednew

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

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

comment:15 Changed 12 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 12 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 12 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 12 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 12 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 10 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 10 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.

comment:22 Changed 10 years ago by Mike Wilson

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?

comment:23 Changed 10 years ago by James Burke

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.

comment:24 Changed 10 years ago by Mike Wilson

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?

comment:25 Changed 10 years ago by Mike Wilson

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...

comment:26 Changed 10 years ago by Miksago

If you know that the browser is IE, or have a way of detecting whether the document is a html document, you could always use something similar to:

function loadScript(url, callback){

    var script = document.createElement("script");
    script.type = "text/javascript";

    if (script.readyState){  //IE
        script.onreadystatechange = function(){
            if (script.readyState == "loaded" ||
                    script.readyState == "complete"){
                script.onreadystatechange = null;
                callback();
            }
        };
    } else {  //Others
        script.onload = function(){
            callback();
        };
    }

    script.src = url;
    document.body.appendChild(script);
}

Which actually works without ajax and is crossdomain compatible. I've been looking at implmenting this sort of thing in [Jet.js] for a few weeks now, and I did a few benchmarks comparing the <script> tag and XHR loading times, and found that times varied quite a lot, example of my results:

Using XHR: 159ms

Using Script: 9ms

This is a result from running of localhost and a single request. The file it's requesting is 2502bytes in size.

Although using script tags isn't probably the best solution, it does work cross browser and is rather speedy, there's some good discussion on the Prototype Lighthouse bugtracker about a similar idea: https://prototype.lighthouseapp.com/projects/8886/tickets/433-provide-an-eval-that-works-in-global-scope

XHR method: http://github.com/Miksago/Jet-js/blob/7116a3ce5783faa98d133eed85b6e8cd34696d73/src/Jet._base.js#L232

Script method: http://github.com/Miksago/Jet-js/blob/7116a3ce5783faa98d133eed85b6e8cd34696d73/src/Jet._base.js#L253

and the script tag method, please note that I haven't done any cleanup on the script tag mmethod, and that you can actually remove the tag immediately after it has loaded. (script.text and script.appendChild(document.createTextNode()) are both methods that work without a url.)

Just a few ideas to throw around.

comment:27 Changed 10 years ago by Miksago

Other note on using the script tag implementation for browsers is that it doesn't hide error messages, unlike window.execScript() in IE, see: http://ajaxian.com/archives/evaling-with-ies-windowexecscript#comment-268381

comment:28 Changed 10 years ago by James Burke

Miksago: Thanks for sharing your work. Some things to consider:

  • The script tag approach will not work for i18n bundles. Maybe we just always use XHR for those, to cut down on the issues.
  • In IE and Safari, appending dynamic script tags can have their JS content evaluated out of order compared to how they are added to the DOM -- they are executed in network receive order. So this can cause problems if you need code to execute in a specific order, like we do for Dojo, to satisfy dojo.require dependencies.
  • In our xdomain loader, we do use dynamically created script tags, but the modules have to be run through a build process to wrap the module code in a function call to avoid the out of order issues in IE and Safari.

comment:29 in reply to:  28 Changed 10 years ago by Miksago

Replying to jburke:

Miksago: Thanks for sharing your work. Some things to consider:

  • The script tag approach will not work for i18n bundles. Maybe we just always use XHR for those, to cut down on the issues.
  • In IE and Safari, appending dynamic script tags can have their JS content evaluated out of order compared to how they are added to the DOM -- they are executed in network receive order. So this can cause problems if you need code to execute in a specific order, like we do for Dojo, to satisfy dojo.require dependencies.
  • In our xdomain loader, we do use dynamically created script tags, but the modules have to be run through a build process to wrap the module code in a function call to avoid the out of order issues in IE and Safari.

You could use XHR to load the contents of the file, then use script tags to inject it into the document and evaluate it, similar to http://github.com/Miksago/Jet-js/blob/868d4452269105c6dabe9b933ce1719ec2674ab0/src/Jet._base.js#L233

comment:30 Changed 10 years ago by Eugene Lazutkin

@jburke: Not directly related to this ticket but there is a problem with loading malformed js --- our error message is useless, which was noted by other projects, including Bespin. One way to improve it is to load it as a script tag, or evaluate as a script tag, if it works for errors.

Basically as soon as we detected the error after evaluating a script all bets are off, and we know that an application has failed already --- let's help users to find a problem. We can safely load the failed script again as a script hoping that the browser will produce some sane diagnostic message about syntax problems in the script. If we do it automatically when isDebug === true on the loading error it'll help many users. Just think about the pesky "stray comma" error in IE, which drinks my blood by buckets.

comment:31 in reply to:  26 ; Changed 10 years ago by Mike Wilson

Replying to Miksago:

If you know that the browser is IE, or have a way of detecting whether the document is a html document, you could always use something similar to:
...
var script = document.createElement("script");

Hm, I thought there was a synchronous requirement for dojo.eval (this is mentioned by James in comment:9). That rules out script tag loading, doesn't it?

comment:32 in reply to:  31 ; Changed 10 years ago by Miksago

Replying to mikewse:

Replying to Miksago:

If you know that the browser is IE, or have a way of detecting whether the document is a html document, you could always use something similar to:
...
var script = document.createElement("script");

Hm, I thought there was a synchronous requirement for dojo.eval (this is mentioned by James in comment:9). That rules out script tag loading, doesn't it?

I do believe you can get synchronous loading using script tags, as in the above example, we're only using them to actually eval the code, the xhr backend is what handles the load order. As in Comment 29 above.

comment:33 Changed 10 years ago by James Burke

Miksago: I have considered using a script tag with inlined text as child text node to bring the module into being, but I feel it makes debugging too intractable -- IIRC, the debugger will just show the HTML file having a bunch of scripts in it, and you cannot individually select the modules in the debugger dropdown. It also would be bad for syntax errors -- the line reporting would make it hard to go back to the original source file to fix it.

On the whole, I think a better debugging experience is better than having global context evaluation, particularly since the best coding practice avoids globals or explicitly declares them by prepending "window." to the variable. I appreciate that does not work for all code/systems though.

I like elazutkin's mention of appending a script tag when there is a syntax error, that would help the problem debugging. But as he mentions, it does not directly apply to the scope issue for this ticket. BTW, that "append a script tag" has a bug number : #5398, however, when I tried a modified patch from Mike Wilcox, it did not seem to work as desired in IE. I'll reopen that ticket to see if we can get that to work better though.

comment:34 in reply to:  32 Changed 10 years ago by Mike Wilson

Replying to Miksago:

I do believe you can get synchronous loading using script tags, as in the above example, we're only using them to actually eval the code, the xhr backend is what handles the load order. As in Comment 29 above.

Ah right, I got hooked on your url/callback parameter interface which communicated async loading to me. The script tag "with text" sounds interesting if the debugging experience could be solved.

I feel we are talking about many potential slight variations of eval, depending on if we prioritize synchronicity, global scope, return value, etc. Would it make sense to offer several "eval kind" functions in Dojo, with different semantics and possibly mapping to different mechanisms in debug and non-debug mode?

comment:35 in reply to:  33 ; Changed 10 years ago by Mike Wilson

Replying to jburke:

I have considered using a script tag with inlined text as child text node to bring the module into being, but I feel it makes debugging too intractable -- IIRC, the debugger will just show the HTML file having a bunch of scripts in it, and you cannot individually select the modules in the debugger dropdown. It also would be bad for syntax errors -- the line reporting would make it hard to go back to the original source file to fix it.

Loud thinking: if it's sufficient with a good debugging experience in Firefox/Firebug?, then maybe we could approach them and ask for better handling of script tags with inline code?

comment:36 in reply to:  35 Changed 10 years ago by James Burke

Replying to mikewse:

Loud thinking: if it's sufficient with a good debugging experience in Firefox/Firebug?, then maybe we could approach them and ask for better handling of script tags with inline code?

Firebug already supports eval debugging, and I believe there is work afoot to support it in webkit. To support inline script tags mapping back to a file, I can see where there is probably no motivation explicitly within the Firefox/Firebug? community for that. I expect it might only gain traction if someone were to do a patch. But then, only maybe.

But Firefox does not have the "eval not in the global scope" problem this ticket is for. IE is the problem, and I cannot see them picking up support for mapping an inline script to a file. Maybe over time if you did a patch against firefox, that got picked up and eventually showed the usefulness, but for me, I think that is unlikely. I think it is more likely people will just change their coding habits not to use the global space.

As for today, if someone really needs this global space thing, you can run dojo in djConfig.debugAtAllCosts mode. It is slower, but it uses script tags to bring all the modules into being.

comment:37 Changed 10 years ago by Mike Wilson

Oh, I think we are talking about two different things here. My loud thinking was about the theoretical possibility to fix any Dojo issue (not just this one) with a bad-for-debugging-solution if at the same time taking steps to improve the debugger (Firebug) to ease the effects of the Dojo fix. But that depends on if it feels sufficient to be able to debug the particular feature in Firebug only.

As to this particular ticket, I am looking at dojo.eval as a function that should provide a cross-browser experience, as several other toolkits have implemented. The point of a public Dojo eval API should be to take the burden off the developer not having to know about the different browser quirks.

Note: See TracTickets for help on using tickets.