Skip to main content

Bug Tracker

Side navigation

#9425 closed bug (wontfix)

Opened May 25, 2011 08:26PM UTC

Closed July 05, 2011 03:25AM UTC

Last modified July 08, 2011 08:47AM UTC

$.isWindow() is easily wrong.

Reported by: yahelc@gmail.com Owned by: rwaldron
Priority: low Milestone: 1.next
Component: core Version: 1.6.1
Keywords: Cc:
Blocked by: Blocking:
Description

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.

Attachments (0)
Change History (17)

Changed May 25, 2011 08:29PM UTC by yahelc comment:1

_comment0: Ugh. Major fail. Can someone with privileges fix the code formatting? Sorry :(1306355999615380

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.

Changed May 25, 2011 08:34PM UTC by yahelc comment:2

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

Changed May 25, 2011 08:35PM UTC by yahelc comment:3

_comment0: From code-blocks: \ FIRST: \ isWindow: function( obj ) { \ return obj && typeof obj === "object" && "setInterval" in obj; \ } \ \ SECOND: \ var x = {"setInterval" : "foo" }; \ \ THIRD: \ isWindow: function( obj ) { \ return obj && typeof obj === "object" && obj.window === window; \ }1306355816731557
_comment1: 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. \ 1306355994964452

(removed)

Changed May 25, 2011 09:17PM UTC by rwaldron comment:4

component: unfiledcore
owner: → rwaldron
priority: undecidedlow
status: newassigned

Changed May 25, 2011 09:19PM UTC by timmywil comment:5

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.

Changed May 25, 2011 09:30PM UTC by rwaldron comment:6

This solution fails in IE6

Changed May 25, 2011 10:13PM UTC by timmywil comment:7

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.

Changed June 03, 2011 11:34AM UTC by rkatic comment:8

Changed June 04, 2011 08:26PM UTC by rkatic comment:9

_comment0: @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.1307219445189220

@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.

Btw. window has no a "constructor" property in IE6/7/8.

Changed July 04, 2011 02:29PM UTC by anonymous comment:10

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.

Changed July 04, 2011 11:59PM UTC by timmywil comment:11

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.

Changed July 05, 2011 03:25AM UTC by rwaldron comment:12

resolution: → wontfix
status: assignedclosed

+1

Changed July 07, 2011 03:23PM UTC by rkatic comment:13

_comment0: @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. So, we could at least do: \ \ {{{ \ return !!obj && typeof obj === "object" && "setInterval" in obj && !hasOwn.call(obj, "setInterval"); \ }}}1310052402596462

@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

Changed July 07, 2011 11:29PM UTC by rwaldron comment:14

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.

Changed July 08, 2011 12:37AM UTC by yahelc comment:15

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.

Changed July 08, 2011 12:42AM UTC by rwaldron comment:16

@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.

Changed July 08, 2011 08:47AM UTC by rkatic comment:17

_comment0: Having such property is an edge case, but not always a bad practice 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 cases should be handled correctly. I would not be surprised with a report of a similar bug in the future (for isWindow or isPlainObject).1310115007734017
_comment1: Having such property is an edge case, but not always a bad practice 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).1310329745554745

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).