miscoded

We released a new browser.js file yesterday. It's a pretty interesting release actually..

The Amazon patch is a pretty radical step - we have never launched any patch that embeds a third-party JS library on every page of a global web presence like Amazon. We know it solves some of our problems, and we know it doesn't solve all problems - we'll keep working with Amazon to have more of their server-side browser sniffing fixed. It would be nice if this patch were short-lived


But the most interesting patch - or at least the hardest one to come up with - is a workaround for a bug in the Carakan ECMAScript engine that a new Google Maps feature hit. The feature is being rolled out, so some users have seen this problem and some haven't yet. As you see from the screenshot, they are very excited about the new feature and announce it with a small popup. In Opera, that popup refuses to go away again. Using the close button just brings up the small "loading" indication at the top of the screenshot. Using Google Maps with this stubborn box staying on top of everything else is more than a little annoying..

My colleague Kåre did the hard work of reading obfuscated JS to figure out where the problem was. I don't want to know how many hours it took him to boil it down to this compact demo:

var passed = 0;
(function (){
	var foo = 1;
	this.runtest = function (){
		eval('window.ev=function(a){eval(a)};');
		try{
			window.ev('(function(){passed=foo})()')
			}
		catch(e){}
		document.getElementsByTagName('p')[0].firstChild.data = passed?'PASS':'FAIL';
		};
	})();
runtest();

To understand this issue, including why finding a patch at first seemed impossible, we need to know about an esoteric quirk of the eval() function.

Normally, if you're naughty enough to call eval() (which it is better not to use, by the way..) you invoke the window.eval() built-in function. It will evaluate the code you pass to it in the local scope. So, for example doing this:

function(){
  var ok=true;
  eval('alert(ok?"sees local variable":"does not see local variable")');
}

the eval'ed code sees the local variable 'ok'.

However, eval by any other name doesn't smell as sweet - if the name you use to refer to the eval function is not 'eval' the method will change behaviour entirely, and run code in the global scope instead! Doing this:

var ok=false;
function(){
  var ok=true, ev=window.eval;
  ev('alert(ok?"sees local variable":"does not see local variable")');
}

we see that if the reference to the eval method is different, it no longer sees local variables!

This odd behaviour is a known eval quirk which is now standardised in ECMAScript 5.

Regarding the problem, Google Maps seem to have found a way to do indirect eval in a local scope. Sort of. I have no idea why they are doing it this way..

The window.ev('(function(){passed=foo})()') just references another method that will do the actual eval'ing. Doing an indirect eval here would run code in global scope. Hence it should not actually have accesss to the local 'foo' variable.

However, the earlier eval call is a direct eval - so it will run in the local scope:
eval('window.ev=function(a){eval(a)};');
meaning that the function expression here should have closure data that would include the local variable foo.

Hence, when a string of code that refers to 'foo' is passed to the direct eval() inside that ev function, this eval call will run in the local scope of that function expression, and the variable foo should be found in its closure data. And this is where Carakan stumbles: it fails somehow to get the closure data right for a function expression created in an eval() inside another function.

Now, why was this hard to patch? Normally when some method Opera defines has a bug, we can overwrite that method with a fixed one from browser.js. For example, if we find a problem in the DOM appendChild() method we'd just overwrite Node.prototype.appendChild with a working method. However, we can not overwrite window.eval() like that - because we would need to cache a reference to it, and any calls to that reference would be an "indirect" eval and run in the global scope.. This would violate most of Google Maps assumptions and make the site fail entirely.

Here's the patch I eventually came up with:

addPreprocessHandler(  /function\(a\)\{eval\(a\)\}/g, '(function(){return function(a){eval(a)}})());' );

This does a search/replace on Google Maps' source code while we load it, replacing any instances of

function(a){eval(a)}

with

(function(){return function(a){eval(a)}})());

which adds an extra function expression - one more chance for Carakan to record closure data. This double-take does, by a lucky stroke, save the required context.

So, here's hoping most users get the new browser.js before the Google Maps update. I like the speeeed of patching problems before users even get them