Bug Tracker

Modify

Ticket #11150 (closed bug: invalid)

Opened 2 years ago

Last modified 2 years ago

$.map inconsistent behavior

Reported by: davestein Owned by: davestein
Priority: undecided Milestone: None
Component: unfiled Version: 1.7.1
Keywords: Cc: danheberden
Blocking: Blocked by:

Description

I came across an issue where IE8 will iterate through an object too many times. If an object has 3 items and we use $.map on it, and alter the indexes during the loop for all 3 items, it will loop 6 times. The fiddle shows what I mean since it's pretty hard for me to explain.

 http://jsfiddle.net/davestein/xKzyV/9/

Anyway I'm aware it's an IE8 bug. The strange thing is it seems as though $.map attempts to work around it. If I do $.map in IE9 or IE9's IE8 mode it works fine. If I do $.map in IE8 it does not work.

I'm aware that IE8 mode is not identical to IE8. So as a constant I used native JS and it broke in all cases. If jQuery wasn't trying to account for it, I'm unsure why $.map would work on IE8 mode, but not IE8.

Change History

comment:1 Changed 2 years ago by davestein

Oh as I note my workaround is:

var x = { a_1 : 1 }; var copy = $.extend( {}, x ); $.map( copy, function( value, key ) {

var new_key = key.replace( 'a_', ); x[new_key] = value; delete( x[key] );

});

comment:2 Changed 2 years ago by rwaldron

  • Owner set to davestein
  • Status changed from new to pending

Can you reduce that fiddle to something a little simpler?

comment:3 Changed 2 years ago by Dave Stein <dave@…>

I went to make it simpler and then found another interesting point of confusion.

 http://jsfiddle.net/davestein/xKzyV/16/
Yields bad results every time, no matter what browser/mode/function.

 http://jsfiddle.net/davestein/xKzyV/9/
Does not yield bad results for $.map in IE8 mode as I had previously reported.

Expected Results: a_1 1, a_2 2, a_3 3

Bad results: a_1 1, a_2 2, a_3 3, 1 1, 2 2, 3 3

comment:4 Changed 2 years ago by rwaldron

I'm not sure this is something jQuery can fix:

 http://gyazo.com/bd96a4c4d1ec26f0863e9eede7b755dc.png

comment:5 Changed 2 years ago by Dave Stein <dave@…>

Any idea why it works fine in fiddle version 9, in IE8 mode? That's the part that confuses me most. I'd expect it to duplicate in all situations.

comment:6 Changed 2 years ago by rwaldron

  • Cc danheberden added

I'm not sure, I'll have danheberden take a look - he most recently authored changes to $.map, so he may have insight that I don't immediately see

comment:7 Changed 2 years ago by danheberden

DaveMethvin was in there recently too. I don't recall any issues with IE8 that I ran into via tests 'n stuff. The disparity between IE8 and IE8 mode seems sensible, sense they aren't the same version.

Yay bugs!

comment:8 Changed 2 years ago by Dave Stein <dave@…>

I'm wondering if map can fix the issue the way I did myself. If it iterates on a copy instead of actual object, and the callback just returns arguments of value and index, I don't see how that would break existing code.

It also seems "this" inside callback refers to window instead of object so that's one less thing to worry about for such a change.

Dan, the disparity is def sensible, if it was at least consistent between my fiddles :) Maybe I'm missing a minute detail of difference between the two because I'm using functions in one and not the other. I can't think of why that would change the outcome.

comment:9 Changed 2 years ago by danheberden

The biggest thing I see as a problem is the cost of copying first - <IE8 is already awfully slow with map, and a copy would just be that much more work (thinking where $.map is used internally as an immediate pain point).

Sounds like the first step would be testing for when this happens?

comment:10 Changed 2 years ago by rwaldron

Something to note... $.map is being used here for "side effects" which is technically an incorrect use. $.map is expected to return an array, if you want to iterate values and do something with them that will produce side effects, then use $.each.

comment:11 Changed 2 years ago by danheberden

Additionally, the bug in question seems to be directly related to deleting keys on an object in IE8, not map's function of iterating.

comment:12 Changed 2 years ago by dmethvin

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

To follow up on rwaldron's comment, would you say it's safe practice to modify the incoming object while you're iterating it? In every language I've used, modifying a collection in the midst of iteration is, at minimum, a point of concern. It is for the very reason that you see here.

For example, C++ STL iterators don't support modifying the collection while it's being iterated. Microsoft Win32 API filesystem calls like FindNextFile behave inconsistently when you add or remove files from the current directory while iterating it. The same goes for Java's Iterator, where items can only be deleted (no additions) during iteration.

Sure enough, ECMA-262, section 12.6.4 says:

The mechanics and order of enumerating the properties (step 6.a in the first algorithm, step 7.a in the second) is not specified. Properties of the object being enumerated may be deleted during enumeration. If a property

that has not yet been visited during enumeration is deleted, then it will not be visited. If new properties are added to the object being enumerated during enumeration, the newly added properties are not guaranteed to be visited in the active enumeration. A property name must not be visited more than once in any enumeration.

One way to get a consistent set of results is to obtain the list of keys in advance, before starting the iteration. But that's expensive and potentially wasteful so many iteration classes/implementations try to avoid it.

It's easy to create a new object rather than trying to mess with the current one while it's being iterated. That is actually the semantics of a .map() operator, since it came from functional programming where you would be shot, then drawn and quartered, for mutating an incoming object!

Since this is an edge case and documented to have undefined behavior, if you absolutely must modify the current object then write code that gets the keys in advance.

Version 0, edited 2 years ago by dmethvin (next)

comment:13 Changed 2 years ago by Dave Stein <dave@…>

Thanks for the detailed answer Dave. I'm going to stick with getting keys in advance then. I still have to wonder why I got different results between the two fiddles though.

And to your point rwaldron, I'm going to have our code changed from $.map to $.each. I almost never use $.map, didn't click in my head to have coworker switch his $.map to $.each because of side effect.

comment:14 Changed 2 years ago by rwaldron

Glad to have been a help - thanks again for the report.

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.