Bug Tracker

Modify

Ticket #9897 (closed bug: fixed)

Opened 3 years ago

Last modified 2 years ago

try-catch isPlainObject detection

Reported by: bkrausz Owned by: rwaldron
Priority: low Milestone: 1.6.3
Component: core Version: 1.6.2
Keywords: Cc:
Blocking: Blocked by:

Description

Related to #7743 & #7780

A mildly convoluted case, but it caused me some hours of pain:  http://nerdlife.net/jquery_bug_test.html

IE8 & 9 both throw exceptions on this, but they're silent for some reason unless you have IE9 script debugging on, in which case "SCRIPT65535:" is thrown. I understand the desire to not special-case every host object, but it seems worth throwing a try/catch around the checks there to return false (as jaubourg suggested in #7780) given that nothing but a non-plain object should be causing an exception here.

At the very least that would prevent a silent exception from being thrown.

Change History

comment:1 Changed 3 years ago by rwaldron

  • Owner set to bkrausz
  • Priority changed from undecided to low
  • Status changed from new to pending
  • Component changed from unfiled to core

Thanks for taking the time to contribute to the jQuery project! Please provide a complete reduced test case on jsFiddle to help us assess your ticket!

Additionally, be sure to test against the jQuery Edge version to ensure the issue still exists. To get you started, use this boilerplate:  http://jsfiddle.net/FrKyN/ Open the link and click to "Fork" (in the top menu) to get started.

comment:2 Changed 3 years ago by bkrausz

  • Status changed from pending to new

I did not use jsfiddle because this requires a popup window on the same domain, which it doesn't look like you can do with their tech (at least not easily without some hacks).

Tested on 27291f, confirmed still there.

comment:3 Changed 3 years ago by rwaldron

  • Status changed from new to closed
  • Resolution set to invalid

This is not a jQuery issue. The exception being thrown was caused by you calling console.log(). As soon as I added firebug-lite, the errors dissappeared..

Tested with:

 http://dl.dropbox.com/u/3531958/9897.html

Result:

 http://gyazo.com/04d78335e66aaa3fd019ba4cf45c77dd.png

comment:4 Changed 3 years ago by bkrausz

The exception is being thrown in the extend function for me...check  http://nerdlife.net/jquery_bug_test.html again...even when you don't enable debugging on IE9, the alert I added after the extend function does not get executed.

This time at least is does throw an actual exceptions:

Line: 525
Error: Unexpected call to method or property access.
Last edited 3 years ago by bkrausz (previous) (diff)

comment:5 follow-up: ↓ 7 Changed 3 years ago by rwaldron

  • Status changed from closed to reopened
  • Resolution invalid deleted

Ugh. Possibly the check for obj.constructor

OK, thanks

comment:6 Changed 3 years ago by rwaldron

  • Owner changed from bkrausz to rwaldron
  • Status changed from reopened to assigned

comment:7 in reply to: ↑ 5 Changed 3 years ago by bkrausz

Replying to rwaldron:

Ugh. Possibly the check for obj.constructor

OK, thanks

Bingo, it's a permission issue there. I don't think there's any way to pre-detect it, as this looks like an IE bug (or at least spec deviation). Would recommend a try/catch around that block:

diff --git a/src/core.js b/src/core.js
index 715d73a..e061996 100644
--- a/src/core.js
+++ b/src/core.js
@@ -500,10 +500,16 @@ jQuery.extend({
                        return false;
                }
 
-               // Not own constructor property must be Object
-               if ( obj.constructor &&
-                       !hasOwn.call(obj, "constructor") &&
-                       !hasOwn.call(obj.constructor.prototype, "isPrototypeOf") ) {
+               try {
+                       // Not own constructor property must be Object
+                       if ( obj.constructor &&
+                               !hasOwn.call(obj, "constructor") &&
+                               !hasOwn.call(obj.constructor.prototype, "isPrototypeOf") ) {
+                               return false;
+                       }
+               } catch ( e ) {
+                       // In some very specific cases (ex: IE accessing window.location of another window)
+                       // constructor accesses throw an exception (see #9897)
                        return false;
                }

comment:8 follow-up: ↓ 9 Changed 3 years ago by rwaldron

I was actually in the middle of replying with this:

This is actually a case covered here: #7743 & #7780

jQuery.isPlainObject() is simply not meant to be used with host objects such as window.location. Host objects have no obligation to implement standard internal properties that native objects do, this makes it extraordinarily hard to test host objects

comment:9 in reply to: ↑ 8 Changed 3 years ago by bkrausz

Sorry for the collision, I had assumed this was queued up to be looked at later :).

I agree that this is a weird use case that should be avoided, but throwing an exception (or sometimes pseudo-silently failing) is a pretty bad failure case when doing this. Given that an object that throws an exception when trying to access the constructor is most certainly not plain, it seems worth the added lines to detect.

This adds another inconsistency to isPlainObject across different browsers, but if the goal is to detect situations where normal operations on objects should not be performed, then this seems better than an exception.

comment:10 Changed 3 years ago by rwaldron

No significant performance difference to be seen:  http://jsperf.com/isplainobject-with-try-catch-junk

comment:12 follow-up: ↓ 13 Changed 3 years ago by dmethvin

This seems like a pretty strange situation, and we've been working hard to eliminate try/catch since it often catches things we don't want to catch. (I do see that you've tried to think through what else might be caught, that's good.)

If the url needs to be passed back, why not use window.location.href?

comment:13 in reply to: ↑ 12 Changed 3 years ago by bkrausz

Replying to dmethvin:

This seems like a pretty strange situation, and we've been working hard to eliminate try/catch since it often catches things we don't want to catch. (I do see that you've tried to think through what else might be caught, that's good.)

If the url needs to be passed back, why not use window.location.href?

In this specific case, this was a bug in my code, and the solution was to just toString() the object (which I should have done in the first place).

More generally, we have a system to transfer data between windows so that they can persist between pages. Due to arbitrary garbage collection policies in IE we deep copy everything in the new window. Generic deep-copying like this where you're not 100% sure what's going to be sent into it is the reason I think we should handle the failure case...there's no obvious reason for isPlainObject to throw an exception, it would be like isArray throwing an exception.

comment:14 Changed 3 years ago by Dave Methvin

  • Status changed from assigned to closed
  • Resolution set to fixed

Merge pull request #445 from rwldrn/9897

Fixes #9897. Wrap obj.constructor test in try/catch to avoid problems with host objects. Thanks to bkrausz.

Changeset: e57057739bf0b272113b4b7aa0f3efa369d3fd69

comment:15 Changed 3 years ago by dmethvin

  • Milestone changed from None to 1.6.3

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.