Bug Tracker

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#9425 closed bug (wontfix)

$.isWindow() is easily wrong.

Reported by: yahelc@… Owned by: Rick Waldron
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.

Change History (17)

comment:1 Changed 8 years ago by 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.

Last edited 8 years ago by yahelc (previous) (diff)

comment:2 Changed 8 years ago by yahelc

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

comment:3 Changed 8 years ago by yahelc

(removed)

Last edited 8 years ago by yahelc (previous) (diff)

comment:4 Changed 8 years ago by Rick Waldron

Component: unfiledcore
Owner: set to Rick Waldron
Priority: undecidedlow
Status: newassigned

comment:5 Changed 8 years ago by timmywil

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.

comment:6 Changed 8 years ago by Rick Waldron

This solution fails in IE6

comment:7 Changed 8 years ago by timmywil

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.

comment:9 in reply to:  7 Changed 8 years ago by 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.

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

Last edited 8 years ago by Robert Katić (previous) (diff)

comment:10 Changed 8 years ago by 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.

comment:11 Changed 8 years ago by timmywil

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.

comment:12 Changed 8 years ago by Rick Waldron

Resolution: wontfix
Status: assignedclosed

+1

comment:13 Changed 8 years ago by 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

Last edited 8 years ago by Robert Katić (previous) (diff)

comment:14 Changed 8 years ago by 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.

comment:15 Changed 8 years ago by 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.

comment:16 Changed 8 years ago by 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.

comment:17 Changed 8 years ago by 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).

Last edited 8 years ago by Robert Katić (previous) (diff)
Note: See TracTickets for help on using tickets.