Attributes: Avoid infinite recursion on non-lowercase attribute getters#3134
Attributes: Avoid infinite recursion on non-lowercase attribute getters#3134mgol merged 3 commits intojquery:masterfrom
Conversation
test/unit/attributes.js
Outdated
| "SELECTED" | ||
| ], function() { | ||
| try { | ||
| jQuery( "<div>" ).attr( this ); |
There was a problem hiding this comment.
Should this be a test that mixed-case attributes are correctly retrieved? This just ensures they don't throw but not that they yield correct results.
There was a problem hiding this comment.
You're right, I'll change that.
What do you think about the try-catch approach? Or should I just check assertions and rely on QUnit to fail when it throws? I thought the try-catch approach is better here where I'm specifically testing that no error is thrown.
|
This does seem like it will be more effective at avoiding the infinite loop. 😺 |
|
@dmethvin I was surprised we have no tests for that! (oops) |
|
PR updated. I added another commit that fixes an older test that was introducing side effects, in turn causing the test I've just added to fail only on even runs. 😄 |
|
|
||
| attrHandle[ name.toLowerCase() ] = function( elem, name, isXML ) { | ||
| var ret, handle, | ||
| lowercaseName = name.toLowerCase(); |
There was a problem hiding this comment.
If there's going to be a variable anyway, why not introduce it earlier?
There was a problem hiding this comment.
Where earlier? That's the beginning of the function, this name is different
than the one before its definition.
Michał Gołębiowski
There was a problem hiding this comment.
Only if someone repurposes it, but point taken.
There was a problem hiding this comment.
Yeah, it's only +2 bytes so I didn't want to break that unofficial contract.
|
Any further remarks? Is this good to land? |
One test in the attribute module was overwriting jQuery.expr.attrHandle.checked and wasn't restoring the original state after it finished. It started causing issues for another checked-related test.
Attributes are no longer always treated as lowercase, although hooks for them are. This commit fixes a no longer correct comment.
|
LGTM |
1 similar comment
|
LGTM |
Attribute hooks are determined for the lowercase versions of attribute names but this has not been reflected in the bool attribute hooks. The code that temporarily removed a handler to avoid an infinite loop was removing an incorrect handler causing stack overflow. Fixes jquerygh-3133 Refs jquerygh-2914 Refs jquerygh-2916 Closes jquerygh-3134
Attribute hooks are determined for the lowercase versions of attribute names but this has not been reflected in the bool attribute hooks. The code that temporarily removed a handler to avoid an infinite loop was removing an incorrect handler causing stack overflow. Fixes jquerygh-3133 Refs jquerygh-2914 Refs jquerygh-2916 Closes jquerygh-3134
6 participants
Summary
Attribute hooks are determined for the lowercase versions of attribute names
but this has not been reflected in the bool attribute hooks. The code that
temporarily removed a handler to avoid an infinite loop was removing an
incorrect handler causing stack overflow.
Fixes gh-3133
Refs gh-2914
Refs gh-2916
+2 bytes
Checklist
Mark an
[x]for completed items, if you're not sure leave them unchecked and we can assist.If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.comThanks! Bots and humans will be around shortly to check it out.