Opened 12 years ago

Closed 10 years ago

#6818 closed bug (notabug)

_removeCurrentsFromItems modifies a list it is iterating over with a for(i=0...) style loop

Reported by: Skaffen Owned by:
Priority: minor Milestone: 2.0.0
Component: ui.sortable Version: 1.8.7
Keywords: Cc:
Blocked by: Blocking:

Description

I posted a comment on this on the closed ticket #5985 but I guess no-one saw it.

The outer of the loop is: for (var i=0; i < this.items.length; i++) {

Which has an inner loop which does an iteration and can do:

if(list[j] == this.items[i].item[0])

this.items.splice(i,1);

So the meaning of items[i] will change if the splice goes off, which means that this loop can skip doing some of the comparisons it should be doing and potentially even have this.items[i] cause an error if the list shrinks below what the current [i] value is. I've no idea really what _removeCurrentsFromItems is for, so I can't cook up a test case - hopefully someone who knows more about the purpose of this function can cook up a test case which will demonstrate the bug.

The fix to the code would be iterate backwards like other similar code does, e.g. for (var i = this.items.length - 1; i >= 0; i--), and to break out of the inner 'j' loop if a match is found that triggers this.items.splice(i,1).

Change History (2)

comment:1 Changed 10 years ago by Scott González

Milestone: 1.9.02.0.0

comment:2 Changed 10 years ago by mikesherov

Resolution: notabug
Status: newclosed

Thanks for contributing! The code in reference no longer exists as is. Since there's no test case (as was asked for), I need to mark this as notabug.

Note: See TracTickets for help on using tickets.