Issue 5728 - Replace Services.vc.compare

Issue 5728 - Replace Services.vc.compare

[Manish Jethani, 2017-09-19 07:53:15]

https://codereview.adblockplus.org/29548719/diff/29548720/lib/coreUtils.js
File lib/coreUtils.js (right):

https://codereview.adblockplus.org/29548719/diff/29548720/lib/coreUtils.js#ne...
lib/coreUtils.js:50: result = (comp1 == "*" ? Number.MAX_VALUE : parseInt(comp1,
10) || 0) -
I'd just like to point out that parseInt doesn't just parse strings as NaN, it
can also yield Infinity. In the unlikely case where one of the values is a
number too large, this will give the wrong result. Number.MAX_VALUE - Infinity
is -Infinity, for example. If both values are too large, you'll have Infinity -
Infinity, which is NaN and not 0.

Of course this is unlikely but I just thought I'd point it out.

For what it's worth, the previous version of this function does not handle this
correctly either, nor does the native version in Gecko.

A simple way around this:

  if (comp1 == "*")
    result = comp2 == "*" ? 0 : Infinity;
  else if (comp2 == "*")
    result = -Infinity;
  else
    result = (parseInt(comp1, 10) || 0) - (parseInt(comp2, 10) || 0) || 0;

The last "|| 0" is needed because Infinity is true and Infinity - Infinity is
NaN. Alternatively that expression could be "parseInt(comp1, 10) | 0 -
parseInt(comp2, 10) | 0".

https://codereview.adblockplus.org/29548719/diff/29548720/lib/coreUtils.js#ne...
lib/coreUtils.js:51: (comp2 == "*" ? Number.MAX_VALUE : parseInt(comp2, 10) ||
0);
Also of course this version differs from the previous one in that it doesn't
handle version numbers like "54.0b1", but I take it that is intentional.

Here are some actual Firefox version numbers for reference:

https://ftp.mozilla.org/pub/firefox/releases/

[Manish Jethani, 2017-09-19 08:21:41]

https://codereview.adblockplus.org/29548719/diff/29548720/lib/coreUtils.js
File lib/coreUtils.js (right):

https://codereview.adblockplus.org/29548719/diff/29548720/lib/coreUtils.js#ne...
lib/coreUtils.js:50: result = (comp1 == "*" ? Number.MAX_VALUE : parseInt(comp1,
10) || 0) -
On 2017/09/19 07:53:14, Manish Jethani wrote:
> Alternatively that expression could be "parseInt(comp1, 10) | 0 -
> parseInt(comp2, 10) | 0".

Bitwise OR-ing with 0 will mess up any values larger than 2 ^ 31 - 1, by the
way, so it's not a good idea either.

[Wladimir Palant, 2017-09-19 08:25:53]

It seems that you took the simplified implementation from the unit tests for
this. That implementation was never meant to be complete, it is only good enough
for the tests to pass.

I suggest that you take the implementation from lib/compat.js instead, without
any changes to make reviewing simpler. If you want to change the implementation,
there is still time for that later. You should also migrate the
versionComparator unit tests from adblockpluschrome repository.

For reference, we are using this code to compare extension versions among other
things. So it should be able to deal with version numbers like "2.99.0.1838beta"
at the very least.

[Wladimir Palant, 2017-09-19 08:38:48]

For reference, the version format this code was originally supposed to handle is
the toolkit version format
(https://developer.mozilla.org/en-US/docs/Toolkit_version_format). While Firefox
still uses that format, AMO got more restrictive now (see
https://github.com/mozilla/addons-linter/blob/496ff99/src/schema/formats.js#L5),
and so we might be able to simplify this logic. Firefox version numbers like
"57.0a1" also match that regular expression.

[Sebastian Noack, 2017-09-19 22:53:43]

On 2017/09/19 08:25:53, Wladimir Palant wrote:
> You should also migrate the versionComparator unit tests from
adblockpluschrome
> repository.

I figured that the notification tests already cover the scenarios considered by
this implementation. However, if you insist, or if we are going to handle more
scenarios, I can add dedicated tests.

> So it should be able to deal with version numbers like "2.99.0.1838beta" at
the
> very least.

Yes, this implementation is not yielding the exact same result as
Sevices.vc.compare() did, in some scenarios involving non-numeric characters.
However, the way parseInt() works the part after including the first non-numeric
character in each component is just ignored. Hence "2.99.0.1838beta",
"2.99.0.1838b1" and "2.99.0.1838" are all considered equal. While this isn't
entirely accurate, it seems for what we are using it for, this would give the
desired result.

For example, if we show a notification to Firefox >=57 users, why would we want
to have Firefox 57 Beta users excluded? Or when we check the browser version to
predict the availability of a feature (where API detection isn't possible), as
we do in the platform and UI code, If the feature exists in Firefox 55, it most
certainly also exists in Firefox 55 Beta.

https://codereview.adblockplus.org/29548719/diff/29548720/lib/coreUtils.js
File lib/coreUtils.js (right):

https://codereview.adblockplus.org/29548719/diff/29548720/lib/coreUtils.js#ne...
lib/coreUtils.js:50: result = (comp1 == "*" ? Number.MAX_VALUE : parseInt(comp1,
10) || 0) -
On 2017/09/19 08:21:40, Manish Jethani wrote:
> On 2017/09/19 07:53:14, Manish Jethani wrote:
> > Alternatively that expression could be "parseInt(comp1, 10) | 0 -
> > parseInt(comp2, 10) | 0".
> 
> Bitwise OR-ing with 0 will mess up any values larger than 2 ^ 31 - 1, by the
> way, so it's not a good idea either.

How exactly might parseInt() return Infinity? It seems that too large numbers
are just rounded to a finite number, but never to Infinity, which makes sense
mathematically, since every possible finite number is closer to Number.MAX_VALUE
than to infinity. Neither does parseInt("Infinity") return Infinity, but NaN.

[Manish Jethani, 2017-09-20 01:15:38]

https://codereview.adblockplus.org/29548719/diff/29548720/lib/coreUtils.js
File lib/coreUtils.js (right):

https://codereview.adblockplus.org/29548719/diff/29548720/lib/coreUtils.js#ne...
lib/coreUtils.js:50: result = (comp1 == "*" ? Number.MAX_VALUE : parseInt(comp1,
10) || 0) -
On 2017/09/19 22:53:43, Sebastian Noack wrote:

> How exactly might parseInt() return Infinity? It seems that too large numbers
> are just rounded to a finite number, but never to Infinity, which makes sense
> mathematically, since every possible finite number is closer to
Number.MAX_VALUE
> than to infinity. Neither does parseInt("Infinity") return Infinity, but NaN.

Number.MAX_VALUE is approximately 1.79E+308, so for example 1.79E+309 (179
followed by 307 zeroes) yields Infinity.

  parseInt("179" + Array(307).fill(0).join(""), 10)

[Sebastian Noack, 2017-09-20 01:31:26]

https://codereview.adblockplus.org/29548719/diff/29548720/lib/coreUtils.js
File lib/coreUtils.js (right):

https://codereview.adblockplus.org/29548719/diff/29548720/lib/coreUtils.js#ne...
lib/coreUtils.js:50: result = (comp1 == "*" ? Number.MAX_VALUE : parseInt(comp1,
10) || 0) -
On 2017/09/20 01:15:38, Manish Jethani wrote:
> On 2017/09/19 22:53:43, Sebastian Noack wrote:
> 
> > How exactly might parseInt() return Infinity? It seems that too large
numbers
> > are just rounded to a finite number, but never to Infinity, which makes
sense
> > mathematically, since every possible finite number is closer to
> Number.MAX_VALUE
> > than to infinity. Neither does parseInt("Infinity") return Infinity, but
NaN.
> 
> Number.MAX_VALUE is approximately 1.79E+308, so for example 1.79E+309 (179
> followed by 307 zeroes) yields Infinity.
> 
>   parseInt("179" + Array(307).fill(0).join(""), 10)

Interesting. It's probably not a too relevant scenario, but I think the way I
account for it now, seems simple enough to just do it.

[Wladimir Palant, 2017-09-21 08:26:46]

On 2017/09/19 22:53:43, Sebastian Noack wrote:
> I figured that the notification tests already cover the scenarios considered
by
> this implementation. However, if you insist, or if we are going to handle more
> scenarios, I can add dedicated tests.

We have two options here:

1. Version comparison is a generic utility function, part of coreUtils module.
As such, it needs to correctly deal with all version numbers that we might see,
and its functionality needs to be verified by dedicated unit tests. This
functionality is non-trivial, so I'd definitely ask you to make any changes in a
separate review, *after* you've moved the existing code to this repository.

2. Version comparison is an internal detail of our notifications functionality,
defined in the notification module and not exported. We can discuss what limited
subset is necessary for notifications to work correctly. But even then,
"1.13.3.1808beta" is not equal to "1.13.3" - we definitely need the ability to
distinguish specific development builds from stable releases. Note also that we
might notify users about a bug in a Firefox 57 beta. If that bug is fixed in the
final release, then we want to limit the notification to "57.0b1" rather than
have it apply to "57.0" as well.

[Sebastian Noack, 2017-09-21 22:02:31]

message: On 2017/09/21 08:26:46, Wladimir Palant wrote:
> We have two options here:
> 
> ...
>
> 2. Version comparison is an internal detail of our notifications
functionality,
> defined in the notification module and not exported.

I like that idea. All instances outside of adblockpluscore are only interested
in the major version anyway, hence we can just use parseInt() there. This is in
fact what we are already doing adblockpluschrome, and we considered doing the
same in adblockplusui before.

> But even then, "1.13.3.1808beta" is not equal to "1.13.3"

These wouldn't have been considered equal by my implementation.

> Note also that we might notify users about a bug in a Firefox 57 beta.
> If that bug is fixed in the final release, then we want to limit the
> notification to "57.0b1" rather than have it apply to "57.0" as well.

That is a good point. I eventually changed the implementation, to account for
version suffixes.

[Wladimir Palant, 2017-09-22 10:11:49]

On 2017/09/21 22:02:31, Sebastian Noack wrote:
> > But even then, "1.13.3.1808beta" is not equal to "1.13.3"
> 
> These wouldn't have been considered equal by my implementation.

I see, I actually expected parseInt("1808beta", 10) to return NaN rather than
1808. Either way, the current implementation is clearly better.

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js...
lib/notification.js:85: (comp2 == "*" ? Infinity : parseInt(comp2, 10) || 0) ||
0;
The readability of this statement is very suboptimal. In particular, I didn't
immediately realize that "1.*" and "1.*" will compare correctly (`|| 0` converts
NaN to zero). I suggest adding an inner function to parse components:

  function parseComponent(comp)
  {
    if (comp == "*")
      return Infinity;
    return parseInt(comp, 10) || 0;
  }

Then you can change this into:

  let result = (parseComponent(components1[i]) - parseComponent(components2[i]))
|| 0;

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js...
lib/notification.js:91: return tail1 == tail2 ? 0 : !tail1 || tail2 && tail1 >
tail2 ? 1 : -1;
This part isn't covered by unit tests (so far these were assuming that version
comparison is tested elsewhere), so you need to add some. At the very least, you
need to verify that "27.0b1" < "27.0" and "27.0" < "27.1a1".

Also, this might be valid for compilers, but this statement is very hard to
read. I suggest that you change at least one conditional into a proper if
statement (better both) and add parentheses:

  if (tail1 == tail2)
    return 0;
  if (!tail1 || (tail2 && tail1 > tail2))
    return 1;
  return -1;

For reference, this code won't compare "57.0a9" and "57.0a10" correctly. We can
live with that I think, but you should add a comment noting that.

[Manish Jethani, 2017-09-22 23:23:41]

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js...
lib/notification.js:91: return tail1 == tail2 ? 0 : !tail1 || tail2 && tail1 >
tail2 ? 1 : -1;
FWIW "tail2 && tail1 > tail2" can be just "tail1 > tail2" as tail2 will never be
an empty string, just undefined, in which case the comparison will evaluate to
false anyway.

[Manish Jethani, 2017-09-22 23:28:52]

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js...
lib/notification.js:72: let regexp = /^(.*?)([a-z].*)?$/i;
If "57.0a9" vs "57.0a10" matters, we could handle it with a little extra effort:

  const re = /([\d*]+\.?[\d*]*)?([a-zA-Z]+)?(\d+)?/;

  let [, numA1, strB1, numC1, strD1] = re.exec(v1);
  let [, numA2, strB2, numC2, strD2] = re.exec(v2);

Then we can simply repeat the number and string comparisons for the C and D
components respectively.

[Sebastian Noack, 2017-09-23 00:32:51]

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js...
lib/notification.js:72: let regexp = /^(.*?)([a-z].*)?$/i;
On 2017/09/22 23:28:51, Manish Jethani wrote:
> If "57.0a9" vs "57.0a10" matters, we could handle it with a little extra
effort:
> 
>   const re = /([\d*]+\.?[\d*]*)?([a-zA-Z]+)?(\d+)?/;
> 
>   let [, numA1, strB1, numC1, strD1] = re.exec(v1);
>   let [, numA2, strB2, numC2, strD2] = re.exec(v2);
> 
> Then we can simply repeat the number and string comparisons for the C and D
> components respectively.

This would add notable complexity to this implementation, this regular
expression alone is rather complex, and incorrect (the 4th group is missing).

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js...
lib/notification.js:85: (comp2 == "*" ? Infinity : parseInt(comp2, 10) || 0) ||
0;
On 2017/09/22 10:11:49, Wladimir Palant wrote:
> The readability of this statement is very suboptimal. In particular, I didn't
> immediately realize that "1.*" and "1.*" will compare correctly (`|| 0`
converts
> NaN to zero). I suggest adding an inner function to parse components:
> 
>   function parseComponent(comp)
>   {
>     if (comp == "*")
>       return Infinity;
>     return parseInt(comp, 10) || 0;
>   }
> 
> Then you can change this into:
> 
>   let result = (parseComponent(components1[i]) -
parseComponent(components2[i]))
> || 0;

I kinda liked the one-liner, but what I like about your suggestion is that we
don't need those two temporary variables. Done.

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js...
lib/notification.js:91: return tail1 == tail2 ? 0 : !tail1 || tail2 && tail1 >
tail2 ? 1 : -1;
On 2017/09/22 10:11:49, Wladimir Palant wrote:
> This part isn't covered by unit tests (so far these were assuming that version
> comparison is tested elsewhere), so you need to add some. At the very least,
you
> need to verify that "27.0b1" < "27.0" and "27.0" < "27.1a1".
> 
> Also, this might be valid for compilers, but this statement is very hard to
> read. I suggest that you change at least one conditional into a proper if
> statement (better both) and add parentheses:
> 
>   if (tail1 == tail2)
>     return 0;
>   if (!tail1 || (tail2 && tail1 > tail2))
>     return 1;
>   return -1;
> 
> For reference, this code won't compare "57.0a9" and "57.0a10" correctly. We
can
> live with that I think, but you should add a comment noting that.

Done.

https://codereview.adblockplus.org/29548719/diff/29551721/lib/notification.js...
lib/notification.js:91: return tail1 == tail2 ? 0 : !tail1 || tail2 && tail1 >
tail2 ? 1 : -1;
On 2017/09/22 23:23:41, Manish Jethani wrote:
> FWIW "tail2 && tail1 > tail2" can be just "tail1 > tail2" as tail2 will never
be
> an empty string, just undefined, in which case the comparison will evaluate to
> false anyway.

This is actually true (it wasn't in earlier when I extracted the tails
differently). But I have my doubts that Wladimir would approve a simplification,
relying on the fact that any compassion with undefined returns false. ;)

FWIW, I'm not sure either, how great an idea that is. If we'd ever touch this
code again, and some change causes tailN to be an empty string instead again, it
wouldn't be very obvious what implication this has on the logic here.

[Wladimir Palant, 2017-09-25 08:25:22]

LGTM