Javascript eval problematiek

"Eval is evil" is een van de belangrijke uitspraken van Douglas Crockford, en toch kom je het eval-statement nog wel eens tegen. De laatste tijd ben ik het 2 keer tegengekomen, en bij beide keren leerde ik een nieuwe reden om eval evil te noemen. Dus hierbij deel ik die ervaringen.


Scope problematiek



Bij een project werkte de google-analytics functionaliteit niet helemaal. De bedoeling was dat hij bij elke verversing van de resultaten (partial page refersh, dus via ajax) opnieuw iets naar google analytics stuurde, maar dit gebeurde niet. Alleen bij de eerste keer laden werd er iets gestuurd.



In de HTML bij de partial page refresh werd keurig dit opgenomen:


<script type="text/javascript">
var _gaq = _gaq || [];
_gaq.push(['_setAccount', 'UA-XXXXX-X']);
_gaq.push(['_trackPageview' ...]);
...
</script>

Normaal zou dit moeten werken (het staat vrijwel letterlijk in de google-documentatie), dus waarom gaat dit fout?



Het project gebruikt tapestry 5, en bij een ajax-refresh van een deel van een pagina controleert tapestry op script-tags, en voert de inhoud van deze tags uit nadat de elementen vervangen zijn, en wel via een eval. En als je iets zo uitvoert krijgt het een andere scope. Als het echt op de pagina staat krijgt het namelijk document scope, en nu krijgt het eval scope. Dat betekent dat er eerst alle variabelen en functies worden bepaald, en daarna de assignments etc. uitgevoerd worden. Dus komt het stukje ongeveer overeen met:


var _gaq;
_gaq = _gaq || [];
_gaq.push(['_setAccount', 'UA-XXXXX-X']);
_gaq.push(['_trackPageview' ...]);

Nu zie je duidelijk dat we last hebben van hoisting. _gaq is een lokale variabele (binnen de scope), die bij de eerste assignment altijd een lege array zal zijn. Doordat de variabele lokaal is zal Google analytics deze variabele nooit gaan kennen. Dit kan op verschillende manieren opgelost worden, we hebben toen gekozen voor de eerste variant die we tegenkwamen die werkte:


<script type="text/javascript">
var _gaq = window["_gaq"] || [];
_gaq.push(['_setAccount', 'UA-XXXXX-X']);
_gaq.push(['_trackPageview' ...]);
...
</script>

Dit werkt beter, omdat de eerste keer (als de hele pagina geladen wordt) de globale variabele _gaq goed gezet wordt.  Dit gebeurt door of Google analytics of ons stukje script. En bij elke partial refresh zal dus _gaq gelijk zijn aan window._gaq, en dus nooit aan een lege array. Wat ook gewerkt had was puur ‘var’ weghalen. Dan zal het aangepast moeten worden voor ecmascript 5 strict mode, maar voorlopig werkt dit ook in alle browsers. En er is een verschil, maar dat is voor ons niet echt relevant.



We hadden dit via Tapestry op kunnen lossen, maar als we het in de script-tag willen oplossen is het beste wat we kunnen doen is te beslissen om _gaq alleen een shorthand te laten zijn, dus bijvoorbeeld:


var _gaq = window._gaq || (window._gaq = []);

Het is trouwens wel grappig hoe Google Analytics in dit geval werkt. Het maakt de _gaq variabele aan als hij nog niet bestaat, en als hij er staat voert hij dus de gewenste handelingen uit. Daarbij zal het of de elementen moeten verwijderen of bij gaan houden welke er uitgevoerd zijn. En later moet het op bepaalde tijden controleren of er nieuwe elementen zijn in de _gaq (de google analytics queue dus), en ze uitvoeren. Blijkbaar hebben ze zelfs voor alle browsers een manier gevonden om dit nog 1 keer te doen voordat iemand een pagina sluit, gezien de tekst die erbij staat. En dat is natuurlijk precies wat een gebruiker van analytics het liefst heeft.

Indicator van evil



Bij een andere klus zag ik dat javascript gebruikt werd om de HTML voor formulieren te genereren. Dat doet me al twijfelen, maar de regels die in dat script geplaatst moesten worden waren iets als dit:


<script>
createForm("form");
form.addElement("checkbox", "id", 'naam', "waarde");
...voeg meer elementen toe…
form.addToHtml();
</script>

Dit verbaasde me een beetje. Hoe kan dit goed gaan? Ik maak geen variabele form aan, en ik geef eigenlijk geen plek op waar het formulier moet komen (behalve dat de script-tage op die plaats stond).



Maar na de javascript-code die achter createForm zat bekeken te hebben snapte ik het wel.


function createForm(objectName) {
if (objectName) {
eval(objectName + " = this;");
...
}
}

Degene die dit gemaakt heeft, weet niet waar de klepel hangt qua javascript. De bedoeling is om een globale variabele te maken die verwijst naar iets. Als je “form = this;” uitvoert in eval scope (of zelfs in een functie zonder context) zal this verwijzen naar de globale scope.



Wat het eval-ding doet is feitelijk een globale variabele aanmaken (objectName) of zelfs overschrijven, die verwijst naar de globale scope. En aangezien alle functies in dit geval op die globale scope zitten, werkt het. Maar het is een vreemde constructie. window[objectName] = this; zou hier bijvoorbeeld hetzelfde resultaat hebben gehad in plaats van de eval-regel, dus deze wordt zelfs onnodig gebruikt. En eigenlijk is de hele variabele form onnodig, aangezien hij verwijst naar het ding dat je toch al weet, de globale scope.



Wat veel logischer was geweest was om een object terug te geven dat de gewenste functies had. En volgens mij heeft de maker hiervan nooit doorgehad dat het het de globale scope was die je terugkrijgt. Dit is dus een vrij onzinnig gebruik van globale variabelen, en dit kan leiden tot onlogische gevolgen. Bijvoorbeeld doordat de formuliermaker de naam mag opgeven is de kans op conflicten (het overschrijven van andere globale variabelen, waardoor onderdelen niet goed meer werken) vrij groot.



Toen begon ik wat verder in de code te bladeren, en zag de functie voor het genereren van het formulier. Dit gebeurt in de 740-regels lange addToHtml()-functie, die zeer vaak document.writeln(‘element’) aanroept, met een aparte case voor elk type invoerveld dat gegenereerd kan worden. Het werkt, maar het is een vrij procedurele manier van werken en zal aardig wat onderhoud vergen / gevergd hebben.



Conclusie



Ik had hiervoor eigenlijk de mening dat eval vooral evil was omdat je een manier biedt om willekeurige code (zeker als het via ajax-calls is binnengekomen) uit te voeren, waardoor een hacker meer controle kan krijgen dan je eigenlijk wilt, en het is wat minder doorzichtig/moeilijker te debuggen.



Maar via deze twee feiten zijn er nog 2 redenen bijgekomen voor mij: eval kan via de scope zorgen voor op het eerste gezicht onlogische problemen, en het (onzorgvuldig) gebruik van eval is een ‘canary in the coalmine’, een sterke indicatie dat de code minder is van kwaliteit.

No comments:

Post a Comment