[1.9.1] Ticket #13360 - String.prototype.namespace breaks jQuery.Event

[andrewplummer]

Sugar (http://sugarjs.com/) adds methods to built-in prototypes.

Now, I realize that sentence alone will likely elicit a strong reaction, especially within the jQuery team. However, Sugar makes an argument that there is a safe way to augment JS natives by being safe, conservative in approach, tons of testing, and a few fundamental differences to Prototype (most of this is detailed here http://sugarjs.com/native). Sugar takes a certain stance, but I do see valid points to both sides of the argument.

However, I believe that the pull request I have submitted, lies outside this argument. The crux of the issue is that jQuery is checking for the property namespace on an object that is assumed to be a jQuery.Event but in fact may also be a string. When making this assumption it should do a type check to ensure that the object is an instance of jQuery.Event before assuming the property namespace on it.

I am anticipating a counterpoint that "it is reasonable for libs to assume "clean" prototypes" and I think there is a valid point to be made there. However I have my own counterpoints to that, and regardless this change can only serve to make jQuery more robust. Also I believe that if you are presuming a namespace property it is reasonable to assume it to be scoped to the object you have defined it on.

I am probably going to remove this method from Sugar anyway, 1. as an emergency fix on the assumption that this pr will take a while to be released even if it is accepted and 2. because it never fit into the lib very well anyway.

Thank you!

andrewplummer/Sugar/issues/265

[andrewplummer]

Sorry, I had accidentally modified the package.json file by trying to much around with a build (to get tests running). Reverted that change now.

[otakustay]

+1 for this

But would jQuery accept trigger call like this: elem.trigger({ type: 'click', namespace: 'widget' });? your patch assumes event argument to be a jQuery.Event instance and could possibly break duck typing programming.

[andrewplummer]

Hm... I actually had a pretty thorough look, and did it this way as it appeared that only jQuery itself would ever add a ´namespace´ property (as an internal flag) but now that I think about it, if it's part of the spec that ´namespace´ be allowed in plain objects as well, then this check should probably be changed to ´typeof event === 'object'´ instead.

[andrewplummer]

Also note that anything that is not a jQuery.Event will be converted to one, but that part comes below.

[andrewplummer]

@otakustay Okay you win... I had started with a simple typeof event === "object" but for full "duck-typing" as you say, I'm checking the typeof on the property itself. Does jQuery intend you to be able to pass one string defined on another? That's what you say would imply and it would be odd... also there is another instanceof jQuery.Event already on line 632 so I don't think strict duck-typing is a requirement here, but this way works too. I also added the same check for the above line on event.type as the implication is the same.

I tightened up the unit tests as well and am now triggering using object literals for full test coverage.

[dmethvin]

I think you've anticipated our line of reasoning correctly. If we start doing this there are a lot of places where we would need to do it.

[andrewplummer]

Well, while I may have anticipated the overall attitude, I don't think I could have anticipated the actual response.

I've gone to lengths to make sure this change is well tested. It is only 20 characters that does not adversely affect the logic... in fact it reinforces the intent of the original developer. So what is the problem?

You don't have to do it everywhere... this is the code that's breaking now, so let's do it here! Even if it did become necessary to do these kinds of checks going forward (and it would be a good idea), then a very simple method could be made to check for the existence of a string property on an unknown object. I'm not suggesting you sweep through your codebase...

[uipoet]

Wow, @dmethvin. Not even a discussion before closing the door? Please consider those of us using both jQuery and Sugar in our production websites.

[ermouth]

jQuery team arguments like "If we start doing this there are a lot of places where we would need to do it" are good for politics, but irrelevant for engineers.

I think jQuery has no exclusive rights to reserve some common-scope non-system properties of prototypes assigning no useful features to these properties. Unfortunately this is exactly what I see.

Arguments like "extending prototypes is a bad practice" are even more irrelevant. In no way jQuery neither shall reduce the set of native javascript features nor count on assumption that nobody will extend these prototypes. For example Douglas Crockford's json2 lib do extend prototypes – cause it's sometimes the only method to make code work with appropriate perfomance. I think Mr. Crockford is enough authoritative for jQuery team )

So, if something need to be done, it must be done.

[gnarf]

Hate to beat a dead horse, but @dmethvin has a pretty solid point. We do our best to avoid conflicts where possible, but there are always edge cases. Most of these cases will be people modifying prototypes. The extra type checking added here isn't supremely dangerous or complex, but it is only needed if someone actually adds a namespace to a prototype. Looking at what this method actually did in sugar also supports my assumption that it probably shouldn't be on String.prototype in the first place.

Considering this method was simply removed from sugar in andrewplummer/Sugar@2f146fa, I don't think we need to take any action here.

@ermouth - It's not about politics, it's about our approach. We specifically forbid adding anything to Object.prototype, as do most people. We also completely avoid decorating base prototypes inside jQuery. We feel that others should also avoid decorating base prototypes, with the obvious exception being polyfills. These arguments are not irrelevant. We can count on these assumptions, because we are vocal about them.

We aren't saying that ALL modifications to String.prototype will break jQuery, because obviously, adding toJSON doesn't break anything. We simply do not want to accept responsibility for adding extra type checking to support libraries that do enhance native prototypes with methods that have dangerous names.

Next thing we'll need to fix is someone who adds String.prototype.nodeType or something equally strange.

All and all, I think the original comments in the PR by @andrewplummer summed up my reaction pretty quickly. I also am not opposed to adding the extra type checking myself, but considering newer versions of sugar will not create this problem, I don't see why we should bother merging it in.

[ermouth]

@gnarf37 Sorry, but still no solid arguments. "We feel that others should also avoid decorating base prototypes" – aint it slightly arrogant? Crockford is allowed to extend prototypes. In some cases. Others are not. Right?

"We can count on these assumptions, because we are vocal about them" – please provide the link were you say "Guys, you must not use names ... , ... , .... extending prototypes ... , ... , ....". I think it doesn't exist, this link.

"We won't extend prototypes to avoid jQuery incomatibility with other libs" – this is approach. "We won't add 20 bytes to fix problems caused by prohibiting others to extend prototypes. They all are not Mr. Crockford" – this is politics.

Thank you for your answers, your highness :(

[andrewplummer]

@gnarf37 Thanks for replying...

there are always edge cases

Agreed of course, but doesn't it make sense to fix these edge cases as they come up? I'm not requesting here that we sweep the code... edge cases should be dealt with ad-hoc in my opinion, but they should be dealt with. I'm also asking a real question as well... Maybe there actually is a legitimate reason not to fix this issue?

but it is only needed if someone actually adds a namespace to a prototype

But that's exactly the issue here. It's also something that I think every library author should expect, regardless of their stance on prototypes. jQuery is probably the most included script in the world. It will inevitably run up against modified prototypes, and it will, in this instance, break. Now of course you can say to people that it's simply wrong to modify prototypes, and as I said before I think that's a valid opinion. But we both know that doesn't matter to the poor soul who simply wanted to use both libs together and has no idea why that doesn't work. At the end of the day it's a breakage that jQuery could have prevented, and without modifying it's stance on prototypes either. I'm not asking jQuery to condone prototype modification, I'm simply asking it to expect it. I believe that is valid advice even if Sugar/Prototype never existed in the first place.

Another example to reinforce this point... Many devs out there have taken a stance against libraries modifying prototypes, but with the understanding that if the code is all under your own control, then it is acceptable. However even that would break here, as the code in question is making assumptions about the state of the prototype object, which can be modified (regardless of whether or not it should be).

Looking at what this method actually did in sugar also supports my assumption that it probably shouldn't be on String.prototype in the first place.

You're right... completely agree, but that's really quite irrelevant to this point in the end. namespace could have been anything.

Considering this method was simply removed from sugar in andrewplummer/Sugar@2f146fa, I don't think we need to take any action here.

Honestly I'm not concerned about taking direct action on this ticket so much as I am in changing the attitude. namespace really didn't fit into Sugar, and I had actually been planning on removing it so this problem was a no-brainer. However that will most definitely not be the case if this happens again.

We specifically forbid adding anything to Object.prototype

I repeat this every time it comes up, but Sugar DOES NOT, HAS NOT, and WILL NOT EVER modify Object.prototype. There is a very major difference between decorating Object and other native objects.

We also completely avoid decorating base prototypes inside jQuery. We feel that others should also avoid decorating base prototypes...We can count on these assumptions, because we are vocal about them

I realize you weren't responding to me here but to repeat the major points: 1) As a stance, Sugar agrees to avoiding Object, but not the other prototypes. 2) We need to make a sepration here between "avoiding prototype modification as a stance", and "expecting prototype modification (assuming the "worst") for the sake of robust code". They are fundamentally different, and saying "we can count on these assumptions because we are vocal about them" represents a disconnect here. I always wear a seatbelt when I drive, but that doesn't mean when I drive my friends I should always assume that they will too.

We do not want to accept responsibility for adding extra type checking to support libraries that do enhance native prototypes with methods that have dangerous names.

I would argue that type checking is always necessary as you should never be making assumptions about prototypes. Doing so is akin to, if not identical to, the kind of assumptions you make by overwriting them in the first place. However, I'm willing to consider what you're saying as valid if you can suggest a means by which we can arrive on the definition of what a "dangerous method name" is...

Next thing we'll need to fix is someone who adds String.prototype.nodeType or something equally strange.

Remember, this isn't simply about someone adding a method and breaking everything. For this issue to arise means that the jQuery code is itself making assuptions. If the code was doing a check for nodeType on something that could even potentially be a string, I would be making exactly the same arguments and just as strenuously. A library should not be making assumptions about prototypes one way or another. The exception is of course for Object, which is why Sugar avoids them.

[gnarf]

@ermouth Perhaps you missed the last lines of my message. I'm not opposed to adding some extra type-check sanity to support a lib used by a small subset of our users. I don't think any of us really are. To be honest, I'm a bit offended that you think I am making these points from a point of 'power' or 'arrogance' worth calling me a "highness". I was adding my opinion, in an attempt to add a few extra words on the topic that @dmethvin didn't type out before he hit close.

Let me re-iterate my points since you seem more interested in giving us shit than being reasonable:

• I personally support adding more type checking when needed to avoid these problems.
• The team feels that most prototype decoration is dangerous and unnecessary, we don't treat anyone special here. We happen to not run into problems from your love-buddy crockford's .toJSON method he adds to prototypes because the method name is generally benign and not something we accidentally clash with. Also, it is a method that exists in the ECMA spec ever since JSON was added, so we are well aware of its potential to be there.
• This particular case is special. @andrewplummer removed this method from sugar, which allows us to kind of ignore the fact that the problem existed in the first place. New versions of sugar won't create this problem at all. If this were something that was going to persist, we may take a different action.
• We try to write the least amount of code necessary to complete the task at hand, and getting very explicit about type checking things causes more code, this is why we are generally against doing it.
• We don't think we are any better than you.

---

@andrewplummer - I am aware of the fact that it is jQuery making an assumption that got us into trouble in the first place, but at some level we must make some assumptions about the type of data we get. If we have a "namespace" property on the argument, we assume it's a string, because that's how we use it internally. This assumption got us into trouble here. It created a problem with your library, and for this I personally apologize even though I never touched this code. Thanks for bringing it to our attentions, and thanks for removing it so we don't have to worry about it.

The particular point I made about nodeType is actually a very important one. jQuery's code base uses the presence of a .nodeType property as a way to detect elements. If someone were to add String.prototype.nodeType jQuery would start assuming every string passed to it was an element instead. There are a lot of nodeType checks in the codebase, and if someone did do this, it would create havoc.

---

The moral of this story, prototype decoration can cause problems with other libraries hidden deep inside some method due to JavaScript's duck typed nature. This is why we highly suggest against it.

[andrewplummer]

@gnarf37

This issue is actually more complex than it appears, so I want to be explicit. I don't think that the fact that the code assumes the property namespace to be a string is the issue. Instead I believe the problem is that it assumes any property with special meaning on an object that has the potential to be of built-in type. In this case the variable event could be one of three things: a string, a vanilla object, or a jQuery.Event object. So the question is "is it safe to assume the internal property namespace on any or all of these?"

1. If you know that it is a jQuery.Event object, then of course I think it's perfectly fine to assume that namespace is a string, or anything you want as it's an internal object.

2. If you know that it is vanilla object, then this is the middle-ground. Of course it may or may not be safe to assume that Object.prototype is untouched (even Sugar would not affect this), but it may be more complex than that. Let's say that the object passed did not have a namespace property on it but inherited from an object that does. This would be an example of how this code could break even if Object.prototype remained untouched. I would argue that to be properly robust here a hasOwnProperty check is in order. If the property is directly defined, however, then I would say that from the signature of the method itself it's fair to assume it to be a string... I don't see any problem with that.

3. If you know that it is a string, then it doesn't make sense to assume any properties on it at all. It should not even be able to get this far. However note that here the hasOwnProperty check describe above would also serve as a fix here (in fact it's probably even better than the fix I proposed here). If you used this method it would be possible to alleviate the issue without even requiring a type check in the first place, which would also satisfy @otakustay's idea about duck-typing.

We try to write the least amount of code necessary to complete the task at hand, and getting very explicit about type checking things causes more code, this is why we are generally against doing it.

I really don't agree with this on a fundamental level. In the first place I think that a method should properly check its input whenever possible and concerns about "more code" should be secondary. Of course it's fair to contain this within bounds of reason, but in any case I don't see this being an issue that necessarily generates huge amounts of code. For this example, this method is essentially saying "I will accept anything that has a property namespace defined on it". What it should be saying (imo) is one of two things:

1. I will accept any object (typeof === "object") that has a property namespace defined on it.
2. I will accept anything that has a property namespace defined directly on it (hasOwnProperty)

In retrospect, I actually believe the 2nd way is the best way, and I can most definitely update this PR to reflect that. But let's just continue with this for a second. This kind of pattern should be common around the jQuery library so a method could be created to do exactly that. Whereas right now you have:

if( event.namespace ) 
  ...

this could easily become:

if( userInputHasPropertyDirectlyDefined(event, 'namespace') ) {
}

function userInputHasPropertyDirectlyDefined (obj, property) {
  return obj.hasOwnProperty(property);
}

Function name is hilariously long, but after minification it would add as little as six bytes (plus the actual method code itself), and this code could be reused everywhere. Again I'm not suggesting you sweep the codebase but if you were to be doing this check every time you accept user input, I argue that it would make jQuery much more robust as a whole.

nodeType ... create havoc

The nodeType example is an interesting one. I've actually tried my hand at element/node detection before, so I know how hard it can be. However let's say that you had a method that accepts arbitrary input... If you were to depend on the nodeType property to detect elements then the most proper check would probably be "nodeType" in obj && !obj.hasOwnProperty('nodeType') as nodeType is never a direct property. In that sense of course hasOwnProperty could not be used as in the example above. Now of course I realize that this check is probably being used everywhere so performance issues may exist. However in this case a simple typeof obj === 'object' is in order, no? (typeof is fast)

I'm simply listing up a way that even if String.prototype.nodeType were modified jQuery could continue to function properly.

All in all it sounds to me that you're more open than I first thought (well.. at least you @gnarf37 ) and are treating this particular instance as a special case, so that's encouraging. I appreciate it.

[andrewplummer]

@gnarf37 btw @ermouth is ferocious. His bug reports to me have the same tone. Please don't take it personally :)

[ermouth]

@gnarf37 Again.
— You added very curvy limitation over entire language grammar without saying a word about it. "Don't extend prototypes" is just a zero-meaning motto. Even just a list of names used by $ internally seem to be good palliative in this case.
— Migrate plugin does not fix this incompatibility between 1.9- and 1.9 – this is good reason to mention this fact in manuals at least.
— jQuery perform type check in faulty way – this way is not only potentially faulty, it's faulty in real life as we see. It's good reason to correct the way you typecheck despite of your principles about prototypes. Your principles have no point of application in this particular case.

And the fact Andrew did remove the .namespace doesn't mean this problem is dead horse – I think it's just early bird that will grow up in elephant if dropped.

And I'm sorry about you feel offensed. I just pointed out in such straightforward manner that your "should avoid using prototypes" is outrageous.

Thank you again.

[moll]

And another person hit by this issue.

This is plainly bad duck type checking on jQuery's side that has to be fixed or clearly and loudly documented (which would be foolish given that @andrewplummer already fixed this above). Considering how widespread jQuery's use with 3rd party code is, it's reckless to not code defensively.

[andrewplummer]

Okay, I've updated the request to use jQuery's internal core_hasOwn (hasOwnProperty) instead of typechecking namespace as a string. Not sure why it's not showing up here but you can see it here: https://github.com/andrewplummer/jquery/commit/2fa53463fc0b196586ca23c090cb6e53253c7290

Too many commits in the end here but minus the back and forth, the two lines:

type = event.type ? event.type : event,
namespaces = event.namespace ? event.namespace.split(".") : [];

are now:

type = core_hasOwn( event, "type" ) ? event.type : event,
namespaces = core_hasOwn( event, "namespace" ) ? event.namespace.split(".") : [];

Very simple. Instead of asking if "type/namespace" are properties (dot operator) it is now asking if they are direct properties (hasOwnProperty).

No fuss. No type checking required. Closer to the original intent, and doesn't have to travel up the prototype chain so it saves CPU cycles.

My tests are all green now (as are yours of course).

For your consideration.

[gnarf]

Two very important things:

• Can you run that test case through about 5 layers of making it simpler? Add the property to the proto, call jQuery.Event(), test for no error, and delete the property on the proto?
• Can you sign our CLA: http://contribute.jquery.org/CLA/

I'd like to land this today so it can make it out in 1.9.1

[andrewplummer]

Ok looks like the commits showed up in the end (guess it just takes some time), but I simply created a new PR to spare your history from those reverted commits.

This PR simply contains the change to core_hasOwn on those 2 lines, plus a much simplified unit test that only tests the basic trigger(str) which was what was breaking.

#1153

[andrewplummer]

Also, I signed the CLA.