Skip to main content

Bug Tracker

Side navigation

#13715 closed bug (duplicate)

Opened April 03, 2013 12:00AM UTC

Closed April 03, 2013 12:57AM UTC

Last modified April 04, 2013 04:15AM UTC

isPlainObject won't work correctly when constructor property is changed

Reported by: fhcj2 Owned by:
Priority: undecided Milestone: None
Component: unfiled Version: 1.9.1
Keywords: Cc:
Blocked by: Blocking:
Description

When you instantiate a non Object class (not a plain object), and add it its own constructor property, the function will take it for a Plain Object.

See it here: http://jsfiddle.net/fhcj2/J8Yrx/

Attachments (0)
Change History (11)

Changed April 03, 2013 12:20AM UTC by fhcj2 comment:1

_comment0: I think it's cause of this block of code: \ {{{ \ if ( ( obj.constructor && \ !core_hasOwn.call( obj.constructor.prototype, "isPrototypeOf" ) ) || \ !core_hasOwn.call(Object.getPrototypeOf(obj).constructor.prototype, "isPrototypeOf") ) { \ return false; \ } \ }}} \ \ Since every object has its own prototype, which leads to the constructor who created it, I think it could be changed for: \ \ {{{ \ if (!core_hasOwn.call(Object.getPrototypeOf(obj).constructor.prototype, "isPrototypeOf") ) { \ return false; \ } \ }}} \ \ I tested it and it fixed the problem. What do you guys think? Is it ready for a pull request?1364952203289343

I think it's cause of this block of code:

if ( obj.constructor &&
		!core_hasOwn.call( obj.constructor.prototype, "isPrototypeOf" ) ) {
	return false;
}

Since every object has its own prototype, which leads to the constructor who created it, I think it could be changed for:

if (!core_hasOwn.call(Object.getPrototypeOf(obj).constructor.prototype, "isPrototypeOf") ) {
	return false;
}

I tested it and it fixed the problem. What do you guys think? Is it ready for a pull request?

Changed April 03, 2013 12:57AM UTC by rwaldron comment:2

resolution: → duplicate
status: newclosed

Duplicate of #13571.

Changed April 03, 2013 01:27AM UTC by fhcj2 comment:3

Actually this issue is about jQuery 2.0 (had just cloned it from the repository, but forgot to change it here, sry..)

But anyway this is an issue that happens both in 2.0 and 1.9.1 versions of jQuery, and I think it's not related to "Ticket #13571", since it covers a specific situation when the object is added a "constructor" property. Would you explain why you considered it as a duplicate? =\\

Changed April 03, 2013 01:35AM UTC by m_gol comment:4

Your jsFiddle specificely uses jQuery 1.9.1, not 2.0.x. Also, if you change the version, your test no longer returns true in the last case.

Are you sure you have a problem with specifically jQuery 2.0? Because it doesn't seem so.

Changed April 03, 2013 01:50AM UTC by fhcj2 comment:5

Just noticed that now that you said... But I even recloned it right now and the error still occurs on the compiled (by grunt) version from the repository. It says in the code:

/*!
 * jQuery JavaScript Library v2.0.0-pre
 * http://jquery.com/
 *
 * Includes Sizzle.js
 * http://sizzlejs.com/
 *
 * Copyright 2005, 2013 jQuery Foundation, Inc. and other contributors
 * Released under the MIT license
 * http://jquery.org/license
 *
 * Date: 2013-04-03
 */

Would that "2.0.0b1" or "2.x" from jsfiddle be any different from this one? Ty for the reply btw =]

Changed April 03, 2013 01:55AM UTC by m_gol comment:6

_comment0: jQuery edge 2.x is code from code.jquery.com/jquery-git2.js which is always the latest jQuery compilation from master. If you think your compiled code does sth different, try comparing those files. \ \ Isn't it possible that your browser cached jQuery 1.9? I'd suppose this could be the reason as the jsFiddle clearly shows the problem doesn't exist in jQuery 2.01364954167387852

jQuery edge 2.x is code from code.jquery.com/jquery-git2.js which is always the latest jQuery compilation from master. If you think your compiled code does sth different, try comparing those files (using, for example, some command line tool like diff).

Isn't it possible that your browser cached jQuery 1.9? I'd suppose this could be the reason as the jsFiddle clearly shows the problem doesn't exist in jQuery 2.0

Changed April 03, 2013 02:07AM UTC by rwaldron comment:7

I'm not sure what part is unclear, both are directly related to detecting "plain object"-ness when an object's constructor has been changed—either by defining the prototype with an object (and thereby changing the prototype.constructor property) or by adding an own property that happens to be called "constructor". Once you modify a special property, such as "constructor", all bets are off.

Changed April 03, 2013 03:01AM UTC by fhcj2 comment:8

Sry, guys.. My mistake. That example would only cover the error in 1.x really. Made another jsfiddle script using the 2.x jQuery version.

http://jsfiddle.net/fhcj2/cNNA4/

Wouldn't it be an error? The change I proposed would fix it too...

Changed April 03, 2013 04:00AM UTC by m_gol comment:9

Your change moves the check one level below. It can fix your specific use case but it's not a golden solution. For one, your code will completely break when invoked on object created via Object.create(null) - null prototype doesn't have a constructor field, obviously.

Changed April 03, 2013 05:45AM UTC by fhcj2 comment:10

Didn't remember that function.. But I think perhaps a simple "!= null" would solve that problem, right? since it takes an object to reach this code.

var objProto = Object.getPrototypeOf(obj);
if (objProto != null && !core_hasOwn.call(objProto.constructor.prototype, "isPrototypeOf") ) {
	return false;
}

Although it wouldn't cover another (possible)problem which also happens in jQuery 2.x: http://jsfiddle.net/fhcj2/jwpqK/

Since an object created this way seems to behave exactly as a "plain object", I'm not sure if it's not right for it to be identified as such. In the case it is not considered a bug, maybe this code would suffice?

Changed April 04, 2013 04:15AM UTC by m_gol comment:11

Well, this additional check doesn't help in all cases since you just moved it one level lower. If instead of assigning MyClass.prototype to null you assign it to Object.create(null), the code will throw. Sure, it's catched later but using exceptions for flow control is not a good practice. Not to mention your code no longer recognizes {} as a plain object which kind of misses the point...

Like rwaldron said, when you change the prototype field of the constructor function or the constructor field of an object, all weird kinds of things could happen.

P.S. There is no need to type $(document).ready by hand in jsFiddle, since you chose onLoad in the left jsFiddle pane all the code is invoked after window load event occured (and, thus, jQuery document ready, too).