291582 - isValidVersion() checks too strictly ("0.10" is not allowed)

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Anbo Motohiko]

User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.8b2) Gecko/20050422 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.8b2) Gecko/20050422 Firefox/1.0+

Bug 286034 attachment 181593 [details] [diff] [review] changes isValidVersion() function to use regexp.
After this change, Fx says some extensions/themes have invalid version string.
e.g. All-in-One gestures says "0.14.1" and this version cannot be installed.


In bug 286034 comment 56, ben says:
> Index: toolkit/mozapps/update/src/nsUpdateService.js.in
> 	- fix implementation of UpdateItem to correspond to interface changes
> 	- simplify version checker "valid version?" test using regexp
The change seems to be simplify that function _only_, so I think this is not
expected behavior.

Reproducible: Always

Steps to Reproduce:
1.Install latest version of all-in-one gestures extension.
2.
3.

Actual Results:  
A dialog appears. That says: "0.14.1" is not a valid Version String.

Expected Results:  
Installation is completed successfully.

Comment 1

[Peter van der Woude [:Peter6]]

I assume you compiled your build yourself ?
There have been no Win32 builds available since Ben's checkin of bug 286034

Comment 2

[Anbo Motohiko]

(In reply to comment #1)
> I assume you compiled your build yourself ?
> There have been no Win32 builds available since Ben's checkin of bug 286034
No.
I use beast-trunk build.

Comment 3

[Anbo Motohiko]

Attachment: patch

This patch brings back old behavour.
Now "2.0", "2.5.1", "7" and "5.0.0.2004072315" are valid Version numbers (These
samples are from FVF spec.).
http://www.mozilla.org/projects/firefox/extensions/update.html

Comment 4

[Robert Strong (they/them - no direct email)]

Confirming

Comment 5

[Robert Strong (they/them - no direct email)]

This also affects Venkman

Comment 6

[Peter van der Woude [:Peter6]]

Confirming, this patch makes all extensions work again

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to comment #6)
> Confirming, this patch makes all extensions work again
Not so fast... this patch fixes extensions that have a version number with more
than 1 digit (e.g. x.xx.x, etc.)... there are several extensions that don't have
a GUID for the em:id which are also broken due to a similar check. Some
extensions affected are keyconfig, tabx, at least one of the versions of adblock
on the extensionsmirror.

I'm not saying this patch doesn't make sense... I think it does. I just don't
want people wondering why this doesn't fix all extensions.

Comment 8

[Peter van der Woude [:Peter6]]

Yeah, you're right.
I couldn't get all to work and switching back/formard between pre-286034 and
post-286034 builds disabled the lot again

Comment 9

[Robert Strong (they/them - no direct email)]

Note: A separate but similar issue to this regarding extension versioning is
that extensions with alpha characters in their version will still fail. The only
one I know of for sure is a version of greasemonkey for the Trunk - I had 0.3a
and it appears there is also a 0.3b
http://greasemonkey.mozdev.org/

Comment 10

[Anbo Motohiko]

(In reply to comment #9)
> Note: A separate but similar issue to this regarding extension versioning is
> that extensions with alpha characters in their version will still fail. The only
> one I know of for sure is a version of greasemonkey for the Trunk - I had 0.3a
> and it appears there is also a 0.3b
> http://greasemonkey.mozdev.org/

Quoted from "Extension Versioning":
http://www.mozilla.org/projects/firefox/extensions/update.html
> Versions like "2.0b" or "4.2 Preview" are not valid.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 181614 [details] [diff] [review]
patch

How about using (?: and ) for non-capturing parens?

Also use \d instead of [0-9].

Consider tightening up with an alternation to disallow .001?  That gets uglier,
but it is doable.

/be

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

patch checked in as part of 291831

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 181614 [details] [diff] [review]
patch

a=asa

Comment 14

[Caleb]

> Consider tightening up with an alternation to disallow .001?  That gets uglier,
> but it is doable.
> 

Here's a regex I came up with: 
^(?:0\.|[1-9]+(?:0+[1-9]*)*\.){0,3}(?:0$|[1-9]+(?:0+[1-9]*)*)\+?$

It may seem ugly, but it's pretty fast (only about 21 iterations for
"5.0.0.2004072315001", while the previous one is 9)

But this one will not allow the following version numbers (just examples):
 - 05.5
 - 00000001.1
 - 1.01
 - 1.1.001

The regex makes sure that there are no 0 prefixes.

Comment 15

[Jason Chu]

Hmm...I think \d* can be used instead of (?:0+[1-9]*)* because after checking
for a non-zero opener, any digit is legal.  Also, I think there was an extra $
in there unless 1.0+ isn't allowed for some reason.

^(?:0\.|[1-9]+\d*\.){0,3}(?:0|[1-9]+\d*)\+?$

Comment 16

[Caleb]

(In reply to comment #15)
> Hmm...I think \d* can be used instead of (?:0+[1-9]*)* because after checking
> for a non-zero opener, any digit is legal.  Also, I think there was an extra $
> in there unless 1.0+ isn't allowed for some reason.
> 
> ^(?:0\.|[1-9]+\d*\.){0,3}(?:0|[1-9]+\d*)\+?$

You are right Jason. Was a bit foolish of me to write something as complex as
(?:0+[1-9]*)* when it is easily subsituted by \d* :)

About that $, yes it should not be there.

Comment 17

[timeless]

out of curiosity, are we ok with accepting numbers that don't look like 0-9?

Comment 18

[Derek Davenport]

Those +s after [1-9] are unnecessary
^(?:0\.|[1-9]\d*\.){0,3}(?:0|[1-9]\d*)\+?$

Comment 19

[Caleb]

(In reply to comment #18)
> Those +s after [1-9] are unnecessary
> ^(?:0\.|[1-9]\d*\.){0,3}(?:0|[1-9]\d*)\+?$

Even better, and much quicker.

Can someone make an updated patch with this regex implemented? This should
better go in before 1.1a

Comment 20

[Anbo Motohiko]

(In reply to comment #11)
> Consider tightening up with an alternation to disallow .001?  That gets uglier,
> but it is doable.
> 
> /be
I disagree with that.
People often say 'Firefox 1.0.3' as 'Firefox 1.03'. But current implementation
handles that '1.03' is same as '1.3'. Is this really correct? Most people,
include addons developers, think that '1.03' is a '0 minor and 3 release' version.
When we allow zero-lead numbers, we should also modify Version.compare function.

If we take this way, '1.10' is no longer newer than '1.3', isn't it?
Allowing 'zero-lead numbers' makes users, especially me, confuse.


Please document about these (zero-lead numbers are allowed or not, and if
allows, leading zeros will be ignored or not).

Comment 21

[Anbo Motohiko]

(In reply to comment #20)
> (In reply to comment #11)
> > Consider tightening up with an alternation to disallow .001?  That gets uglier,
> > but it is doable.
> > 
> > /be
> I disagree with that.
I'm confused.

I agree disallowing '0.001'.

Sorry for bug spams.

Comment 22

[Anbo Motohiko]

Attachment: patch based on comment #15~19

(In reply to comment #19)
> (In reply to comment #18)
> > Those +s after [1-9] are unnecessary
> > ^(?:0\.|[1-9]\d*\.){0,3}(?:0|[1-9]\d*)\+?$
> 
> Even better, and much quicker.
> 
> Can someone make an updated patch with this regex implemented? This should
> better go in before 1.1a
Sure. Thank you for following-up!

Comment 23

[Brendan Eich [:brendan]]

Someone on IRC asked why I hadn't commented -- it's cuz I was not cc'd.

Based on the post-checkin work in this bug, how about filing a new bug?

/be

Comment 24

[Worcester12345]

(In reply to comment #23)
> Someone on IRC asked why I hadn't commented -- it's cuz I was not cc'd.
> 
> Based on the post-checkin work in this bug, how about filing a new bug?
> 
> /be

Was this ever filed? Bug #?

Comment 25

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

Comment on attachment 182018 [details] [diff] [review]
patch based on comment #15~19

obsoleted by 300731