457777 - bookmark items in distribution.ini are not added if the item number has more than one digit

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Kev Needham [:kev]]

Attachment: Sample distribution/distribution.ini file highlighting issue

Bookmark items with an id number of two digits or more are ignored by the distribution.js module. Bookmark items with an id number of one digit are parsed and added properly. The issue applies to both bookmark toolbar entries and bookmark menu entries.

Attached is a sample distribution.ini file that contains three bookmark toolbar items, one with an item number of "1", one with an item number of "10", and one with an item number of "100". Only the bookmark with an item number of "1" is added to the toolbar.

Comment 1

[Kev Needham [:kev]]

Attachment: Patch to ensure iid comparison is int to int

because Gavin is awesome, he pointed out to me today that there was a string vs. int comparison issue. editing line 157 of distribution.js such that it reads:

   if (maxItemId < parseInt(iid))

seems to do the trick nicely.

thunder, could you have a look and verify that's all that needs to be done to the file?

Comment 2

[Dan Mills [:thunder]]

Comment on attachment 425119 [details] [diff] [review]
Patch to ensure iid comparison is int to int

Holy cow, I'd forgotten about this bug.  Indeed, interpreting those as strings would be trouble.  I didn't realize that /(\d+)/ would end up giving me a string (boo).

I just skimmed distribution.js:

http://mxr.mozilla.org/mozilla-central/source/browser/components/distribution.js?force=1

And it looks like the only instance of this bug there.

r+ as far as I'm concerned

Comment 3

[Kev Needham [:kev]]

Excellent. Need this added to 1.9.1, 1.9.2, and Central. Who should be cc'd for review and landing?

Comment 4

[Marco Bonardo [:mak]]

(In reply to comment #3)
> Excellent. Need this added to 1.9.1, 1.9.2, and Central. Who should be cc'd for
> review and landing?

you already have review. i can land this on central, then you just need to ask approvals.

Comment 5

[Kev Needham [:kev]]

That'd be great, Marco. Thanks.

Comment 6

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ff46fa92babd

once got a green you're clear to ask approval for branches.

Comment 7

[Dan Mills [:thunder]]

Thanks Marco!

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 425119 [details] [diff] [review]
Patch to ensure iid comparison is int to int

Hmm, actually, this patch isn't quite ideal, since iid is still being assigned to maxItemId as a string, so you're still relying on implicit conversion (just in the opposite direction - maxItemId to an int). Would be cleaner to just set iid = parseInt(iid) right after it's assigned to, I think.

Comment 9

[Kev Needham [:kev]]

Attachment: Revised update to address comment #8

Per Gavin's comment #8, revised the patch to set iid = parseInt(iid) after assignment.

Comment 10

[Kev Needham [:kev]]

Marco, can you back out the old patch?

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #10)
> Marco, can you back out the old patch?

I'll undo the previous change once i push the new patch, in a single push.

Comment 12

[Marco Bonardo [:mak]]

backout: http://hg.mozilla.org/mozilla-central/rev/b439b81118a6
push: http://hg.mozilla.org/mozilla-central/rev/2f0b9d7d8c56

Comment 13

[Kev Needham [:kev]]

Asking for acceptance into 1.9.1.next and 1.9.2.next, as it addresses a long-standing bug with existing partner repacks that have more than 10 items in a bookmark folder as part of their customizations. Would also like to ensure this is included in any future releases.

Thanks!

Comment 14

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

Definitely wanted; if those patches are safe, can we get them nominated for the appropriate branches?

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 425437 [details] [diff] [review]
Revised update to address comment #8

Yes, the patch is safe enough, just a string->int cast.

Comment 16

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

Comment on attachment 425437 [details] [diff] [review]
Revised update to address comment #8

a=beltzner for 1.9.2 and 1.9.1

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f27577d95821

Comment 18

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

Marco: can you check this into the branches?

Comment 19

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-1.9.1/rev/d6d370859447