Bug Tracker

Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#7256 closed bug (wontfix)

gracefully handle bad user/intermediate input values

Reported by: rene7705@… Owned by:
Priority: low Milestone:
Component: global Version: 1.4.2
Keywords: needsreview Cc:
Blocked by: Blocking:

Description

Hi..

jQuery has problems with bad userinput.

animate() can set elem.style[name]=BAD_VALUE, and thus throw a javascript error because there is no try {} catch {} structure in place. Depending on circumstances and the browser used this can be a big problem for an app, and it's hard to debug, even with firebug.

ideally, each catch{} would call $.log(msg), which calls console.log(timePassedSinceStartOfScript+msg) and console.trace() if available. add a way to bind to the $.log() too, so i can display such warnings elsewhere aswell.

i built a set of free components that incorporates this fix (no logging though), a set that won't work without the try-catch fix. http://code.google.com/p/rene7705-assorted-libraries/downloads/list After installation in htdocs/code, browse to /code/libraries_rv/logAndHandler-x.y.z and check the html source to see that it uses a modified jquery core. Simply diff with the original.

there are 2 forum threads that mention this issue; http://forum.jquery.com/topic/bugfix-for-jquery-1-4-2-internet-explorer-8-jquery-fx-step http://forum.jquery.com/topic/bugfix-illegal-argument-in-jquery-style-try-catch-fixes-it

i'm willing to donate some of my time back to the jquery team and help identify and "fix" these "bugs".. (I admit they're usually user created bugs, but also see it as an opportunity to make jquery more robust.)

Change History (7)

comment:1 Changed 9 years ago by rene7705@…

My gut estimate is that there are about 5 to 20 of these potential bug spots in the jquery core.

Fixing them would add maybe 1 to 2kb to the source version.

comment:2 Changed 9 years ago by snover

Keywords: needsreview added

There are arguments on both sides for actually doing something about this.

On the for-side, we already discard errors to .css as of 1.4.3 to make IE match other browsers.

On the con-side, it’s a (relatively) lot of extra code to handle invalid user input like this, there are not many complaints about current behaviour, adding a try/catch is only going to make IE’s performance even worse, and the fix to .css was done to handle valid user input that IE just happens to not support (like rgba), not invalid input that nobody supports.

Marking for review.

comment:3 Changed 9 years ago by Rick Waldron

Component: unfiledglobal
Priority: undecidedlow

If by review you mean, "share opinion", I believe developers should be held responsible for sanitizing their own values before passing them into .animate() or anything similar.

comment:4 Changed 9 years ago by dmethvin

Here's the contract: Don't pass garbage to jQuery and jQuery won't barf. Using try/catch to swallow a bunch of programmer errors leads to code that is harder to debug. Rethrowing a specific error message for each mistaken input sounds great but nobody would want to pay the size/performance penalty.

comment:5 Changed 9 years ago by rene7705@…

Fixing these particular bugs would not add many KBs to the jquery sources. It's probably under 4kb, even under 3kb. Any performancy penalty is also only hit when bad input is encountered. And a small one at that.

I also disagree with rwaldron, I believe components should be reporting bad userinput (or bad calculation results!) properly instead of silently failing.

The whole point of having a component "do the work" in an abstract manner is to isolate the app-programmer from the gritty details. If we follow the paradigm put forth by rwaldron and dmethvin, then the programmer DOES need to know about the gritty details. Plus the whole stack of code that makes up the component.

On the other hand, if the component properly reports bad inputs, the app-programmer can spend way more time on his own level of detail rather than the API's that he's using through the component.

There is usually plenty of complexity in my components that use jquery, and frankly it's hard to keep the input sanitized at all times, especially when i'm experimenting with complicated animation sequences. For a demo, see http://mediabeez.ws/lah in a non-ie browser. As you click on details, or click details away, there are several animation calls running at virtually but not exactly the same time in quick succession, followed by a final app-window resize.

If just 1 of the calls leads to bad DOM input, which can also happen in the intermediate jquery code i suspect, then the whole animation fails. Because jquery does not report & trace it, it can take the whole day to check the userinput and intermediate-input values that make up the complete animation. It's that delay in dev of my app that's caused me to add the try-catch structure. I want to try out several different animation sequences in 1 day, not spend the full day fixing just 1 anim seq. (k, that's a simplification.)

A log entry and a console.trace() call would probably reduce hours of searching to mere minutes of fixing.

comment:6 Changed 9 years ago by rene7705@…

And dont try to say that i could check which call causes the problem by evaluating what's still visible where and what isn't. Other animation calls may still get executed _after_ the error is encountered.

It's (more often than you think) virtually impossible to figure out which call it was by looking at the output.

comment:7 Changed 9 years ago by addyosmani

Resolution: wontfix
Status: newclosed

Although I do sympathize with rene7705, three team members have already expressed their opinions on this being a bad idea and I'll be adding a fourth vote with respect to our position on input sanitization. Whilst in a perfect world it might be 'nice' to include specific error messages for each mistaken input, there are 1) simply too many variants of what we've seen developers try to feed jQuery as valid input and 2) far too many points in the codebase that would require a try/catch/error output handler integrated for us to seriously consider doing this. Unfortunately, for the moment developers will need to ensure that they sanitize their data/values before feeding it into jQuery.

Note: See TracTickets for help on using tickets.