Drupal.js has some room for improvement and one of them is a critical bug.

The critical bug is in Drupal.checkPlain. Other JS that adds properties to Object.prototype causes these added properties to leak into checkPlain.

With this patch in place Drupal should just run as expected. It changes no APIs. More details to follow.

CommentFileSizeAuthor
drupaljs_cleanup.patch1.22 KBmfer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mfer’s picture

I explained these problems for a blog post. If I can explain them for a blog post I can submit a patch as well. Since explaining them in detail was quite long and I do not want to do it again checkout the blog post for the long explanation.

arlinsandbulte’s picture

So... where is the blog post? A link would be nice.

Also, I disagree with pointing to a blog post to explain an issue. The explanation really belongs in the issue, for posterity's sake.
Perhaps a simple copy/paste? (probably not that simple, especially if you have screenshots or attachments)

Damien Tournoud’s picture

Priority: Critical » Major
 Drupal.checkPlain = function (str) {

Please use jQuery.each() in Drupal.checkPlain().

-  for (var i = 1, args = []; i < arguments.length; i++) {
-    args.push(arguments[i]);
-  }
+  var args = Array.prototype.slice.apply(arguments, [1]);

I don't see any bug here. Also, any reason not to use arguments.slice(1)? Using the prototype function seems wrong and lazy.

mfer’s picture

Priority: Major » Critical

The blog post is at http://engineeredweb.com/blog/10/11/fixing-drupal-javascript-problem-exa.... My goof on not posting it.

Using jQuery.each() in Drupal.checkPlain() is slower than the patch and does not really make sense. We are not iterating over DOM nodes so the nice elements it has to do this are not needed. If you look at what we would pass into jQuery.each() it would work (internally) similar to this patch but with more code and logic executed.

The arguments variable in every function is not an array. It is an object that works kind of like an array. It has a length property but not the rest of the Array objects functions. At least not before ES5 (which was recently passed and is not in most of the browsers most of the users around the world have). arguments.slice() is not a function we have can use at the moment.

Array.prototype.slice is the appropriate call to use here. If you poke around internally at the jQuery source you'll find they use it as well. It is not inappropriate to use.

What makes this critical can be highlighted in a simple example. If add the following line to JS in the page:

Object.prototype.foo = "bar";

Anywhere Drupal.checkPlain is used "foo" will be replaced with "bar" in the strings.

mfer’s picture

Status: Active » Needs review
mfer’s picture

Additionally, I'd like to point out that jQuery does not use jQuery.each() for situations like Drupal.checkPlain() within its own codebase. It uses the approach this patch takes.

Damien Tournoud’s picture

We have jQuery.each(), I see no reason not to use it.

The arguments variable in every function is not an array. It is an object that works kind of like an array. It has a length property but not the rest of the Array objects functions. At least not before ES5 (which was recently passed and is not in most of the browsers most of the users around the world have). arguments.slice() is not a function we have can use at the moment.

That makes zero sense. You should not apply a method of the Array prototype to something that is not an array.

cosmicdreams’s picture

Read your blog post, interesting stuff there. Is anyone able to test the expected performance gain from this patch?

Damien Tournoud’s picture

The performance gain would be marginal at best. Those functions are seldom called in Javascript.

mfer’s picture

@consmicdreams You could use something like http://jsperf.com/

@Damien Tournoud jQuery.each() is not an end all be all for loops. It's helpful in certain circumstances. Take a look at http://api.jquery.com/each

The .each() method is designed to make DOM looping constructs concise and less error-prone.

I'll give you 3 reasons not to use it:

  1. The code becomes more difficult to read. Readability is a plus.
  2. It doesn't make much sense. Passing into jQuery.each() will create some variables (some with values testing to see if values are there), go through some control logic, and end up at a for ( name in object ) { loop. By using a native JS loop we are bypassing this extra stuff we don't need.
  3. jQuery.each() is great at iterating over a set of items and making a change to each of them. That's why it works so well for DOM elements. But, we are iterating over a set of items and changing one item, the string, which is not on the object. To do this it does not make sense.

Internally within jQuery Array.prototype.slice.apply() is used on arguments to turn them into an array. It is considered an acceptable practice even by members of the ECMA committee. I even checked YUI3 and they use it internally on arguments as well. It is considered an acceptable practice.

Damien Tournoud’s picture

@mfer, obviously, I was talking about the generic loop function:

http://api.jquery.com/jQuery.each/

This function is *designed* to be a generic iterator. It allows you to write more readable/natural Javascript code without having to worry about implementation details. jQuery.each() works exactly in the way foreach() works in PHP.

You could either use (your code):

for (character in replace) {
  if (replace.hasOwnProperty(character)) {
    regex = new RegExp(character, 'g');
    str = str.replace(regex, replace[character]);
  }
}

or:

jQuery.each(replace, function(source, replacement) {
  regex = new RegExp(source, 'g');
  str = str.replace(regex, replacement);
});

The latter is way more readable, natural and as a consequence less error prone for non-hardcore Javascript developers. The speed difference should be minimal, especially on non-naive Javascript execution engines.

Internally within jQuery Array.prototype.slice.apply() is used on arguments to turn them into an array. It is considered an acceptable practice even by members of the ECMA committee. I even checked YUI3 and they use it internally on arguments as well. It is considered an acceptable practice.

It's an implementation detail of Javascript that we should try to avoid. I see that jQuery.makeArray() is a documented way of doing that. jQuery has a test for this exact use case:

	equals( (function(){ return jQuery.makeArray(arguments); })(1,2).join(""), "12", "Pass makeArray an arguments array" );
mfer’s picture

@Damien Tournoud jQuery('#foo').each() literally is a wrapper around jQuery.each(). See the code:

each: function( callback, args ) {
    return jQuery.each( this, callback, args );
},

The inner complexities are still there.

But, more importantly, using jQuery.each() in your implementation keeps the bug from #4 where someone can inadvertently add properties to the source/replace object by adding them to Object.prototype. jQuery.each() does not have a filter mechanism. The code you would need reads:

jQuery.each(replace, function(source, replacement) {
  if (replace.hasOwnProperty(source)) {
    regex = new RegExp(source, 'g');
    str = str.replace(regex, replacement);
  }
});

Is that really more readable than:

for (character in replace) {
  if (replace.hasOwnProperty(character)) {
    regex = new RegExp(character, 'g');
    str = str.replace(regex, replace[character]);
  }
}

jQuery.each() is not the end all be all of loops. If you look at jQuery internally and other code written using jQuery each() is often not used and native loops are used.

It's an implementation detail of Javascript that we should try to avoid.

Why? JavaScript is what it is. There are some beautiful parts. Some so-so parts. And, some downright ugly parts. Using Array.prototype.slice.apply() on arguments is not one of the ugly parts. It's acceptable for people who write JavaScript to use, why not us?

jQuery.makeArray() uses Array.prototype.slice.apply() to make something an array. Then it has some logic to add results to it. If you use that you would do it in2 steps.

var args = jQuery.makeArray(arguments);
args = args.slice(1);

Why do it in 2 steps when you can do it in 1 which is faster, uses less logic, and is common enough is JS circles?

Damien Tournoud’s picture

Priority: Critical » Major

Ok, that's reasonable. I incorrectly thought (based on the examples on the documentation) that jQuery.each() already filtered the indirect properties, but it is not actually the case.

I'm still not convinced about the second change. At the minimum we need a comment to explain why this is done. I know that documenting code is not "common enough in JS circles", but that doesn't mean we should do that :)

Damien Tournoud’s picture

I downgraded this above. Nothing is broken here, except if another script pushes stuff in the main object constructor. This is sadly "common enough in JS circles" but clearly downright ugly.

ksenzee’s picture

Priority: Major » Critical

There's no reason not to borrow a useful method from the Array prototype. JS uses prototypical OO, where methods are first-class objects and can easily and reasonably be reused, not classic OO, where using a method from an unrelated class would be insane. And mfer is exactly right: there are excellent reasons for avoiding jQuery.each. It's especially pernicious if you use it with an inline closure and not a named function. All of this is a Drupal 8 discussion, which I am very much looking forward to having, but a Drupal 7 critical issue is definitely not the place for it. Let's keep jQuery out of this as much as we possibly can, and stick to straight JS.

ksenzee’s picture

Priority: Critical » Major

x-post.

mikl’s picture

Status: Needs review » Reviewed & tested by the community

Wow, nice catch, mfer. I had no idea we had such poor quality JavaScript in core.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

Let's add some comments in both places.

mfer’s picture

While I'm a fan of documentation I'm trying to understand to what level we need to document. The two items I see to document (hasOwnProperty and Array slice usages) are both basics of using the language. They are not fancy tricks or anything out of the ordinary in JavaScript. Are we to document how the language works and what we can do with it when it comes to JavaScript? What do I assume about the developers reading the code here?

My general assumption when it comes to inline documentation is to document what people who know the language might need some guidance on to understand. Holding to that I see no need for documentation here.

Crell’s picture

By and large I agree with mfer here. We don't document PHP usage unless we're dealing with a language WTF or really obscure dark recesses of the language or libraries. We assume that people can and have read the PHP manual, even when we're doing something weird (like, say, hooks). For JS, we should do the same.

So the question is, do either of these changes qualify as "a language WTF or really obscure dark recesses of the language or libraries"? The slice.apply line definitely doesn't. It's basically the same concept as array_map(), at least that's how my PHP brain is interpreting it.

The hasOwnProperty() issue I didn't know about either, although now that Matt has described it I get why it exists. That's only a language WTF to someone who doesn't understand prototypical inheritance, which is most Drupal developers but no serious JS developers. Arguably we could document that as an excuse for a "teachable moment", but since we don't do that for any of our PHP OO (we assume that people reading OO Drupal PHP code understand interfaces, abstract classes, final, etc.) I don't know that it's really needed here either.

mfer’s picture

@Crell, thanks for explaining what I was thinking better than I did. :)

mfer’s picture

Status: Needs work » Reviewed & tested by the community

Since mikl put this to rtbc and it was switched to needs work to discuss comments I'm putting it back now that (I think) that discussion is done.

mikl’s picture

#20: Agreed, I think there’s little here that needs documentation. Googling for “hasownproperty foreach” tells you just about all there is to know :)

Dries’s picture

DamZ, you're good or?

mfer’s picture

Any word on this? I don't want it to fall through the cracks.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Crell and I talked about this patch, he explained to me that there's a potential security issue here, so I decided to commit it since the thing this was being held up for was documentation.

However, I think that documentation ought to be discussed in a separate issue, because this is not the only weird part of the code right now. Also, nothing here sticks out at me as a "someone is going to try and 'fix' this later" type of problem.

Committed to HEAD. Thanks!

mfer’s picture

@webchick - The issues highlighted here should be documented somewhere. I think some sort of guide on how to write good JS would be useful.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.