412600 - for each over Arrays considered harmful

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

There are a few places where front end JS code uses | for each | to iterate over an Array. If any other code adds to the Array prototype, this will cause the | for each | iteration to likely fail.

Currently, there seems to be no code in Firefox that modifies the Array prototype, but extensions could and one already has (bug 404122).

We should be more defensive in our code and stop using | for each | on Arrays.

Some files that use | for each | on Arrays:
http://mxr.mozilla.org/seamonkey/source/browser/components/places/content/utils.js
http://mxr.mozilla.org/seamonkey/source/browser/components/places/content/editBookmarkOverlay.js
http://mxr.mozilla.org/seamonkey/source/browser/components/places/content/treeView.js

Comment 1

[Dave Townsend [:mossop]]

Is it worth putting some kind of strict warning in when using for each over arrays perhaps?

Comment 2

[Nickolay_Ponomarev]

I'd rather we disallow messing with Array/Object.prototype. for [each]..in is a very convenient construct, would be a shame to give it up.

for..in is affected too; both can be made work properly with hasOwnProperty (or whatever it's called).

Comment 3

[Dave Townsend [:mossop]]

(In reply to comment #2)
> I'd rather we disallow messing with Array/Object.prototype. for [each]..in is a
> very convenient construct, would be a shame to give it up.

If that's possible then I think we probably should seriously consider it. Our code and the code of multiple add-ons are sharing this scope, any one of them changing a core prototype can break a lot of stuff.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #3)
> (In reply to comment #2)
> > I'd rather we disallow messing with Array/Object.prototype. for [each]..in is a
> > very convenient construct, would be a shame to give it up.
> 
> If that's possible then I think we probably should seriously consider it. Our
> code and the code of multiple add-ons are sharing this scope, any one of them
> changing a core prototype can break a lot of stuff.
> 

I would rather use Array.forEach(...) in our code and not put any restrictions on the Array prototype.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We can't make the prototypes immutable.  We should code defensively here, and avoid |for each| to enumerate things if we're not going to be careful about filtering the results.  (We should also get SiteAdvisor to avoid mutating the prototype, as a best practice, but being robust in an extensible environment means heeding Postel here.)

Comment 6

[Timothy Joseph Duavis]

Attachment: Proposed solution for bug 412600 ready for review.

Replaced the 'for each' traversing the array.prototype with a regular for loop and restricted the iteration using the 'length' property.

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 298185 [details] [diff] [review]
Proposed solution for bug 412600 ready for review.

r=me, thanks.

drivers: low impact defensive posturing

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 298185 [details] [diff] [review]
Proposed solution for bug 412600 ready for review.

a=beltzner for 1.9

Comment 9

[Reed Loden [:reed]]

Checking in browser/components/places/content/editBookmarkOverlay.js;
/cvsroot/mozilla/browser/components/places/content/editBookmarkOverlay.js,v  <--  editBookmarkOverlay.js
new revision: 1.17; previous revision: 1.16
done
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.30; previous revision: 1.29
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.96; previous revision: 1.95
done

Comment 10

[JT Olds]

Reopened issue with bug 431909 - for each over Arrays still considered harmful

Comment 11

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv