Closed Bug 603021 Opened 12 years ago Closed 12 years ago

Hash comparisons should be case insensitive

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
minor

Tracking

()

VERIFIED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jorgev, Assigned: KWierso)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

This was reported in the Add-ons Forum: https://forums.addons.mozilla.org/viewtopic.php?f=7&t=1760. This add-on is hosted outside of AMO and uses an update hash generated using McCoy. Attempting to update this add-on shows a "Error Downloading" on Firefox 4 (both beta 6 and Minefield), while it works fine on 3.6.

Steps to reproduce:
1) Download an old version of the add-on. Since I couldn't find one, I just downloaded the latest and changed the version number. I'm attaching this XPI for testing.
2) In the Add-on Manager check for updates for the add-on.

Result:
On Firefox 4, the update is found, and apparently downloaded correctly (the request returns correctly), but then the error appears, presumably due to post-processing of the add-on.
The hash in the update.rdf is in uppercase but we're doing a case sensitive comparison to the calculated hash which will fail.
blocking2.0: --- → betaN+
Summary: Add-on Manager fails to update add-on with update hash → Hash comparisons should be case insensitive
Whiteboard: [good first bug]
I am the creator of the addon, if you want a previous version, please find a link for it below. I will be happy to answer any questions.

http://www.k3ltic.com/utools/utools0.771.xpi
so i need to set the case of the update hash to lower? is this something i must fix, or is it a genuine bug?
This is a real bug that needs to be fixed, but as a workaround I believe all you need to do is change the hash to lowercase.
Attached patch toLowerCase() (obsolete) — Splinter Review
Something like this?
Attachment #482110 - Flags: review?(dtownsend)
Jorge/Wes/Dave, Thank you very much for this update, I thought it was something I was doing wrong. I will change the case to lowercase for future releases.
Comment on attachment 482110 [details] [diff] [review]
toLowerCase()

Almost exactly like that, but with a testcase to verify the behaviour ;)
Attachment #482110 - Flags: review?(dtownsend) → review-
Attached patch with testSplinter Review
Would this work? I'm not really up on test writing, so I just copied browser_hash.js with an all-caps hash.
Attachment #482110 - Attachment is obsolete: true
Attachment #482309 - Flags: review?(dtownsend)
Comment on attachment 482309 [details] [diff] [review]
with test

Nice work, thanks
Attachment #482309 - Flags: review?(dtownsend) → review+
Assignee: nobody → kwierso
Whiteboard: [good first bug] → [has patch]
Landed, sorry something went wrong with the checkin name though. http://hg.mozilla.org/mozilla-central/rev/8fd1189ae15c
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b8
Dave, I have tried to verify the fix by using the uTools extension. I have installed the above mentioned version 0.771 but when I check for updates none will be found, while the update.rdf lists one: http://www.k3ltic.com/utools/update.rdf
(In reply to comment #11)
> Dave, I have tried to verify the fix by using the uTools extension. I have
> installed the above mentioned version 0.771 but when I check for updates none
> will be found, while the update.rdf lists one:
> http://www.k3ltic.com/utools/update.rdf

Doesn't that update.rdf file show maxVersion as 4.0b7pre? Trunk's at b8pre, and I don't think there's a way to override the update checker's version requirements. (ACR only overrides installation)
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Yeah, that's it. The max version is now set to 4.0b8pre. So I was able to verify that now with the beta 7 release. Works as expected with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.