$.isWindow() is easily wrong.

[[email protected]…]

I know it's probably just an internal function, but I noticed this is how jQuery detects if an object is a window or not:

isWindow: function( obj ) {
                return obj && typeof obj === "object" && "setInterval" in obj;
        }

But, that is easily broken by

var x = {"setInterval" : "foo" };

Wouldn't a better test be:

isWindow: function( obj ) {
                return obj && typeof obj === "object"  && obj.window === window; 
}

since the window object contains itself infinite times (ie, window.window===window.window.window), whereas (if I'm not mistaken) a regular object wouldn't be able to met that condition.

[yahelc]

Ticket should read:

I know it's probably just an internal function, but I noticed this is how jQuery detects if an object is a window or not:

isWindow: function( obj ) {
		return obj && typeof obj === "object" && "setInterval" in obj;
	}

But, that is easily broken by

var x = {"setInterval" : "foo" };

Wouldn't a better test be:

isWindow: function( obj ) {
		return obj && typeof obj === "object"  && obj.window === window; 
}

since the window object contains itself infinite times (ie, window.window===window.window.window), whereas (if I'm not mistaken) a regular object wouldn't be able to met that condition.

[yahelc]

Here's the test case: ​http://jsfiddle.net/bvrZT/

[yahelc]

(removed)

[Timmy Willison]

That wouldn't work because we need to be able to check windows different than the one to which jQuery is attached. Considering how unlikely setInterval will be a key in an object (and that we haven't had a ticket with a practical use case), I think setInterval is still the best solution. We could add some logic to determine if it was a plain object, but I don't think it's necessary and we'd take a perf hit.

[Rick Waldron]

This solution fails in IE6

[Timmy Willison]

We could perhaps speed it up a little and ensure setInterval is on the prototype:

return obj && typeof obj === "object" && hasOwn.call(obj.prototype, "setInterval");

This would break if someone decides to extend the prototype of their object with something called setInterval, but that just seems wrong.

[Robert Katić]

Patch: ​https://github.com/jquery/jquery/pull/402

[Robert Katić]

@timmywil: I suppose you meant obj.constructor.prototype. The problem is that relying on the constructor (or any other property) of window is dangerous because it is the global object. Is it wrong to have an public "constructor"? Maybe, but there is chances that some code there has it (voluntarily or not).

To avoid such problems, in my patch, I makes an additional check in case there is a global "hasOwnProperty". This check is performed only if there is an "length" property, and window has one.

[anonymous]

I think the following code is a good way to check:

isWindow: (function () {
	var window = (function () {
		return this;
	}());
	return function ( obj ) {
		return obj && typeof obj === 'object' && obj === window;
	};
}())

Since the this in "Function Invocation Pattern" always be the global object(in a browser that is window), so it can check precisely.But somehow i think this solution has some problems, maybe someone can point that out? And i am not going to give a test case as you can reuse the above's.

[Timmy Willison]

I think we can consider making a change to isWindow when there is a more practical use case presented where it breaks. It's best practice not to duplicate commonly used global method names such as setInterval and we have not seen problems since isWindow was introduced.

[Rick Waldron]

+1

[Robert Katić]

@timmywil: Although it seams wrong to have a a "setInterval" property in a prototype, it is not so wrong to have such property in a plain object that acts as a map.

EDIT: Removed a solution that would fail in IE6,7,8

[Rick Waldron]

This is absurd and only encourages people write bad code. The idea that someone might create an object with a "setInterval" method and they might just check this object with $.isWindow() is an edge case at best.

[yahelc]

I agree with the rationale of closing this ticket (and I'm the filer!), but I object to the notion that such a change "encourages" people to write bad code. jQuery is designed to run in hostile environments, and specifically takes precautions against sloppy coding (like if someone does something stupid like var undefined = "blah";). Those protections don't encourage sloppy coding; they're just smart.

[Rick Waldron]

@yahelc if you ask anyone on the bugs team, they will tell that I am the first to vote "for the user" and add code that helps ease the pain of dumb mistakes - but this particular case is far too edge case for me to think it's worthwhile. I strongly doubt that anyone building actual tested web applications will run into this problem.

[Robert Katić]

Having such property is an edge case, but not always a bad practice, specially not in cases of plain objects.

Currently having a map (plain object) like:

var names = {
  setInterval: 1,
  nodeType: 1
  foo: 1
}

and testing it against isWindow or isPlainObject would give wrong results.

Even if edge cases, isWindow and isPlainObject are part of the public API, that means that also all rare (but still valid) cases should be handled correctly. I would not be surprised with a report of a similar bug in the future (for isWindow or isPlainObject).