737056 - NS_CompareVersions is a confusing API

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(no longer active)]

It is really easy to use this API incorrectly without noticing.  See for example bug 735713.  I'd really like us to get rid of this API and replace it with something like this:

namespace mozilla {

struct Version {
  Version(const char* versionString);
  // ...
};

bool operator < (const Version& lhs, const Version& rhs);
// define the rest of the comparison ops

}

This will let consumers write code like:

if (mozilla::Version(appVersion) > productInfoBlock.productVersion) {
  // do something
}

instead of:

if (1 == NS_CompareVersions(appVersion, productInfoBlock.productVersion)) {
  // do something
}

This makes the code much easier to read, and bugs like bug 735713 will be easier to catch.

I'll happily mentor a new contributor on this.

Comment 1

[Mark Capella [:capella]]

Is this Mac OS X specific? I'll help with a good first bug/mentor, but develop under WIN7.

Comment 2

[(no longer active)]

No, this is all cross platform code.

Comment 3

[Mark Capella [:capella]]

Was hoping to work this, but got pulled into something that'll take all my time :(

Comment 4

[Peng Kang]

Hi, I would like to work on this task.

Comment 5

[(no longer active)]

Great to hear this, Peng!

Comment 0 includes a description of what we should do here.  Please let me know if you need more information, or if you hit any problems.  Thanks!

Comment 6

[Peng Kang]

Attachment: Definition of version structure and its comparison operators

Comment 7

[Peng Kang]

Hi Ehsan,

I just defined a Version structure for the purpose of version check. The comparisons are done by calling NS_CompareVersions. I'm not sure how should I test this code on my own so I just upload it without testing...

Is there anything else that I should add to this Version structure? Also I don't know how to do the testing once I modified the code. Do you know where I can get some tutorials on this? Thanks!

Comment 8

[(no longer active)]

Comment on attachment 611936 [details] [diff] [review]
Definition of version structure and its comparison operators

This looks mostly good.  Here are the things to do in order to finish this work:

1. Please make sure that you submit a patch file.  Please see <https://developer.mozilla.org/en/Creating_a_patch> for instructions on how to do that.
2. Please make the versionContent member private.  You can make the comparison operators members of the Version class in order to make it possible to access this member in those functions.
3. Please move the contents of nsVersionComparator.h to nsVersionComparator.cpp and remove that file.  We don't want any callers to be able to access NS_CompareVersions any more.  It's still fine to use that as an internal implementation method.  Note that you should probably move the implementation of the operators to nsVersionComparator.cpp.
4. Please change all of the usages of NS_CompareVersions in the code base to use the new mozilla::Version API.
5. Please rename NS_CompareVersions to CompareVersions or something like that, because the "NS_" prefix might make people think that it is a publicly usable API.

Thanks!

Comment 9

[Benjamin Smedberg]

Note that for the time being the scriptable nsIVersionComparator API should remain unchanged; if we are going to change the scriptable API we should do it as a separate patch/bug.

Comment 10

[(no longer active)]

(In reply to Peng Kang from comment #7)
> Hi Ehsan,
> 
> I just defined a Version structure for the purpose of version check. The
> comparisons are done by calling NS_CompareVersions. I'm not sure how should
> I test this code on my own so I just upload it without testing...
> 
> Is there anything else that I should add to this Version structure? Also I
> don't know how to do the testing once I modified the code. Do you know where
> I can get some tutorials on this? Thanks!

For testing, here are the things that you need to do:

1. Make sure that all of the occurrences of NS_CompareVersions have been switched over to the new API.
2. Make sure that you can build Firefox successfully.
3. Write a C++ test for the mozilla::Version class.  Please see <https://developer.mozilla.org/en/Compiled-code_automated_tests> for instructions on how to do that.
4.  Once you have a patch which is nearly ready, I will submit it to the try server for you <https://wiki.mozilla.org/ReleaseEngineering/TryServer>.  The try server is an infrastructure that we have for building and testing patches on all of the platforms that we support.  This will make sure that your patch will not break the platforms which you have not tested yourself.

Please let me know if you have any questions!

Comment 11

[(no longer active)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Note that for the time being the scriptable nsIVersionComparator API should
> remain unchanged; if we are going to change the scriptable API we should do
> it as a separate patch/bug.

Sure, this bug is only about the C++ API.

Comment 12

[Peng Kang]

Attachment: New Version Comparison API

Comment 13

[Peng Kang]

Hi, 

I just uploaded a patch of version comparison API. I successfully re-built with all the changes that I made, but I haven't done testing yet.

I didn't remove msVersionCompare.h and put the structure definition of Version there.

There's a separate definition of Version structure in toolkit/xre/MacQuirks.h

Because the code having ns_CompareVerions in toolkit/mozaaps/update/updater/archivereader.cpp was involved in a previous bug and is disabled, I didn't change that file.

As the comparisons are done by calling CompareVersions, I think there should not be a problem as long as I modified all the calling of NS_CompareVersions right... Anyway, I will finish testing in probably one or two days.

Peng

Comment 14

[(no longer active)]

Comment on attachment 612656 [details] [diff] [review]
New Version Comparison API

Review of attachment 612656 [details] [diff] [review]:
-----------------------------------------------------------------

This looks very good, Peng!  Thanks!  :-)

Here's some preliminary comments on the patch.

::: dom/plugins/base/nsPluginHost.cpp
@@ +2721,5 @@
> +    }
> +    else {
> +      vdiff = -1;
> +    }
> +  }

Again please create the Version objects once.

::: toolkit/xre/MacQuirks.h
@@ +236,5 @@
>  
>    return result;
>  }
>  
> +struct Version{

Hmm, why can't you just include nsVersionComparator.h here?

::: toolkit/xre/nsAppRunner.cpp
@@ +2937,5 @@
>  
> +    //if (NS_CompareVersions(mAppData->minVersion, gToolkitVersion) > 0 ||
> +    //    NS_CompareVersions(mAppData->maxVersion, gToolkitVersion) < 0) {
> +    if(mozilla::Version(mAppData->minVersion) > mozilla::Version(gToolkitVersion) ||
> +       mozilla::Version(mAppData->maxVersion) < mozilla::Version(gToolkitVersion)){

Please declare the objects once before using them.

::: xpcom/base/nsVersionComparatorImpl.cpp
@@ +58,5 @@
> +    }
> +    else {
> +      *aResult = -1;
> +    }
> +  }

You can construct the Version objects for A and B once at the beginning and then use the local variables in the conditions.

::: xpcom/components/ManifestParser.cpp
@@ +408,3 @@
>      if ((c == 0 && comparison & COMPARE_EQ) ||
>  	(c < 0 && comparison & COMPARE_LT) ||
>  	(c > 0 && comparison & COMPARE_GT))

So this is the code pattern that I'd like us to avoid.  Please rewrite this as below:

mozilla::Version valueVersion(...(aValue));
mozilla::Version testDataVersion(...(testData));

if (valueVersion == testDataVersion)
  ...
else if (valueVersion < testDataVersion)
  ...
...

::: xpcom/glue/nsVersionComparator.cpp
@@ +308,5 @@
>  }
>  
>  
>  PRInt32
> +CompareVersions(const PRUnichar *A, const PRUnichar *B)

Make this function static please.

@@ +343,5 @@
>  }
>  #endif
>  
>  PRInt32
> +CompareVersions(const char *A, const char *B)

Same here.

@@ +407,5 @@
> +    free(versionContentW);
> +#else
> +    free(versionContent);
> +#endif
> +  }

You can make all of the functions above inline in the header.

@@ +477,5 @@
> +    char* rhsContent = rhs.readContent();
> +    bool result = CompareVersions(versionContent, rhsContent) == 0;
> +    free(rhsContent);
> +    return result;
> +  }

So, I think it's probably a good idea to add a private member like this:

bool Version::Compare(const Version& rhs, PRInt32 aExpectedValue) const;

This will do the CompareVersions dance and will return true if the return value matches aExpectedValue.  You can also assert that the return value is always either -1, 0, or 1.  Then you can implement the operators inline in terms of this function to avoid code duplication.

::: xpcom/glue/nsVersionComparator.h
@@ +57,5 @@
>   */
> +//PRInt32 NS_COM_GLUE
> +//NS_CompareVersions(const char *A, const char *B);
> +
> +namespace mozilla{

Nit: please add a space before braces here and elsewhere.

@@ +77,5 @@
> +    bool operator< (const Version& rhs);
> +    bool operator<= (const Version& rhs);
> +    bool operator> (const Version& rhs);
> +    bool operator>= (const Version& rhs);
> +    bool operator== (const Version& rhs);

You forgot operator!=.  :-)

Also, please make all of these operators const.

@@ +83,5 @@
> +  private:
> +    char* versionContent;
> +#ifdef XP_WIN
> +    PRUnichar* versionContentW;
> +#endif

On Windows, you probably want this to be a union, with a boolean tag to decide which one is valid.

Comment 15

[Benjamin Smedberg]

Also before final patch submission, please take a look through the coding style guide:

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

In particular things like "if(" need to be unsnuggled, and there are some bracing and whitespace/indentation issues that need to be addressed. But this looks like a good start!

Comment 16

[Peng Kang]

Attachment: Revised Version Comparison API

Comment 17

[Peng Kang]

Attachment: Revised Version Comparison API

Comment 18

[Peng Kang]

Sorry that I accidentally made my second patch obsolete...

The major changes are shown in the second patch, and in the third patch, I fixed a few "bad" code that I left out in the second patch.

Comment 19

[(no longer active)]

Comment on attachment 613141 [details] [diff] [review]
Revised Version Comparison API

Review of attachment 613141 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsPluginHost.cpp
@@ +2709,5 @@
>    if (PL_strcmp(values[0], "Version"))
>      return rv;
>  
>    // kPluginRegistryVersion
>    //PRInt32 vdiff = NS_CompareVersions(values[1], kPluginRegistryVersion);

Please remove the old code that you've commented out.  If somebody needs to figure out what the old code looked like, they can go through Mercurial history.

@@ +2712,5 @@
>    // kPluginRegistryVersion
>    //PRInt32 vdiff = NS_CompareVersions(values[1], kPluginRegistryVersion);
>    PRInt32 vdiff;
> +  mozilla::Version version(values[1]);
> +  mozilla::Version kPRVersion(kPluginRegistryVersion);

Nit: please name this prVersion.  We use the "k" prefix for constant data.

::: xpcom/base/nsVersionComparatorImpl.cpp
@@ +58,3 @@
>        *aResult = -1;
>      }
>    }

Hmm, thinking about this a bit more, it doesn't make a lot of sense to implement this on top of mozilla::Version.  You can just call CompareVersions directly here.

::: xpcom/glue/nsVersionComparator.cpp
@@ -41,5 @@
> -#include <string.h>
> -#if defined(XP_WIN) && !defined(UPDATER_NO_STRING_GLUE_STL)
> -#include <wchar.h>
> -#include "nsStringGlue.h"
> -#endif

Why did you remove this hunk?

Comment 20

[(no longer active)]

Comment on attachment 613144 [details] [diff] [review]
Revised Version Comparison API

Review of attachment 613144 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/xre/nsUpdateDriver.cpp
@@ +245,5 @@
>  
>    //if (NS_CompareVersions(appVersion, buf) > 0)
> +  mozilla::Version versionApp(appVersion);
> +  mozilla::Version versionBuf(buf);
> +  if (versionApp > versionBuf) {

This is overkill, since we're not going to use versionApp and versionBuf elsewhere in this function.  I think the previous code was better.

Comment 21

[(no longer active)]

Peng, I think this is nearly ready for review.  When you're done addressing my comments, please create a new patch which includes all of your changes, and attach it to the bug and mark it for review from :bsmedberg.

Thanks!

Comment 22

[Peng Kang]

Attachment: New Version Comparison API

::: xpcom/glue/nsVersionComparator.cpp
@@ -41,5 @@
> -#include <string.h>
> -#if defined(XP_WIN) && !defined(UPDATER_NO_STRING_GLUE_STL)
> -#include <wchar.h>
> -#include "nsStringGlue.h"
> -#endif

>Why did you remove this hunk?

This block of code is moved to nsVersionComparator.h because some of the functions defined in these libraries are used in the .h file.

Also to make the CompareVersions functions callable from the outside, I removed the "static" keywords from the definitions of CompareVersions.

Comment 23

[Benjamin Smedberg]

Comment on attachment 613502 [details] [diff] [review]
New Version Comparison API

Thank you for taking this on! I'm going to mark this r-, so that I can see the final patch with the revisions I have mentioned below:

>Bug 737056 - New version comparison API

Please make this commit message more descriptive: "Replace NS_CompareVersions with a C++ API which is harder to misuse" or something like that.

>diff --git a/dom/plugins/base/nsPluginHost.cpp b/dom/plugins/base/nsPluginHost.cpp

>   // kPluginRegistryVersion
>-  PRInt32 vdiff = NS_CompareVersions(values[1], kPluginRegistryVersion);
>+  PRInt32 vdiff = CompareVersions(values[1], kPluginRegistryVersion);
>+  mozilla::Version version(values[1]);

Why are you continuing to use CompareVersions here? Can't you migrate all of this to mozilla::Version?

>   // Registry v0.13 and upwards includes the architecture
>-  if (NS_CompareVersions(values[1], "0.13") >= 0) {
>+  mozilla::Version version013("0.13");
>+  if (version >= version013) {

In many of these cases, we're comparing versions against string literals: I think it makes sense for mozilla::Version to have method overrides for that case specifically so that you don't need the intermediate "version013" locals here, so that you can write this:

  if (version >= "0.13")

>diff --git a/toolkit/xre/MacQuirks.h b/toolkit/xre/MacQuirks.h

Have you tried to build this on mac? The comment about not linking with libxul appears to be relevant, unless you've changed this library to link against the standalone XPCOM glue.

>diff --git a/xpcom/glue/nsVersionComparator.h b/xpcom/glue/nsVersionComparator.h

>+namespace mozilla {
>+
>+struct NS_COM_GLUE Version {

style nit, please put the opening brace of classes and functions on the next line.

>+  Version(const char* versionString) {
>+    uniCharValid = false;
>+    versionContent = strdup(versionString);
>+  }

It appears to be a requirement that we never compare a PRUnichar Version with a char Version, but this doesn't appear to be documented except as an assertion in the impl. If this is the case, why do we have one class to represent both of them? This is creating unnecessary complexity with manual memory management. I think you'd be better off with a mozilla::Version for the char* variant (which is most common) and mozilla::VersionW for the PRUnichar* variant.

Also, the PRUnichar* variant can be in an #ifdef XP_WIN, since it is only used on Windows.

When you have that, you can just store the version string in a nsCString or nsString, and skip all the manual memory management.

>+  PRUnichar* readContentW() const {
>+    assert(uniCharValid);
>+    return wcsdup(versionContentW);
>+  }

There is no reason I can see for readContent/readContentW this method to allocate/duplicate the string. Just hand out a pointer to the internal constant buffer.

>+  bool operator< (const Version& rhs) const {
>+    return Compare(rhs, -1, -1);
>+  }

I think that the intermediate method "Compare" and especially its use of "lowerExpectedIndicator" and "upperExpectedIndicator" in these series of methods is strange. Why not just inline these directly to CompareVersions like this?:

bool operator< (const Version& rhs) const {
  return CompareVersions(mStr.get(), rhs.mStr.get()) < 0;
}

>+};
>+
>+}

please add "// namespace mozilla" after this brace so that it is clear what it is

>+
>+#ifdef XP_WIN
> PRInt32 NS_COM_GLUE
>-NS_CompareVersions(const char *A, const char *B);
>+CompareVersions(const PRUnichar *A, const PRUnichar *B);
>+#endif
>+
>+PRInt32 NS_COM_GLUE
>+CompareVersions(const char *A, const char *B);

You have renamed these methods without the NS_ prefix but have not moved them into the mozilla:: namespace, polluting the global namespace with a very generic method. Can you either keep the old name or move these functions into the mozilla namespace?

Optionally, it would be good to rename these files altogether, so that they are Version.h/cpp and are included via #include "mozilla/Version.h". That is not necessary for this patch, and I can do it afterward if the build scripts frighten you ;-)

>diff --git a/xpcom/glue/nsVersionComparator.cpp b/xpcom/glue/nsVersionComparator.cpp

>+bool Version::Compare(const Version& rhs, PRInt32 lowerExpectedIndicator,
>+		      PRInt32 upperExpectedIndicator) const

As noted above, I don't think this method is necessary at all.

Comment 24

[(no longer active)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #23)
> >   // Registry v0.13 and upwards includes the architecture
> >-  if (NS_CompareVersions(values[1], "0.13") >= 0) {
> >+  mozilla::Version version013("0.13");
> >+  if (version >= version013) {
> 
> In many of these cases, we're comparing versions against string literals: I
> think it makes sense for mozilla::Version to have method overrides for that
> case specifically so that you don't need the intermediate "version013"
> locals here, so that you can write this:
> 
>   if (version >= "0.13")

The constructor for mozilla::Version is not explicit, so this would just work (it would create a Version temporary and then would call Version::operator>=(const Version&) const).

I agree that dropping the Version objects for the literals would make the code more readable.

> >diff --git a/toolkit/xre/MacQuirks.h b/toolkit/xre/MacQuirks.h
> 
> Have you tried to build this on mac? The comment about not linking with
> libxul appears to be relevant, unless you've changed this library to link
> against the standalone XPCOM glue.

Yeah, this is included in nsBrowserApp.cpp, so I don't think it's going to link.  How can we put nsVersionCompare.cpp in the XPCOM glue library?

> >+#ifdef XP_WIN
> > PRInt32 NS_COM_GLUE
> >-NS_CompareVersions(const char *A, const char *B);
> >+CompareVersions(const PRUnichar *A, const PRUnichar *B);
> >+#endif
> >+
> >+PRInt32 NS_COM_GLUE
> >+CompareVersions(const char *A, const char *B);
> 
> You have renamed these methods without the NS_ prefix but have not moved
> them into the mozilla:: namespace, polluting the global namespace with a
> very generic method. Can you either keep the old name or move these
> functions into the mozilla namespace?

I think removing the NS_ prefix is necessary in order for them not to confuse people who know the old name.  Putting CompareVersions inside the mozilla namespace is definitely a great idea.

Comment 25

[Benjamin Smedberg]

I believe that nsVersionComparator.cpp is already in the standalone glue (it is listed here, in any case: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/objs.mk#53

So it may be just that we need to link the interpose library (built from dom/plugins/ipc/interpose) to the standalone glue. I'm just not clear on what other problems that might cause. There has to be a reason that the code was copied wholesale in the first place. So I really don't know the correct answer to this question, which may require either some trial-and-error or leaving that particular case alone.

Comment 26

[Benjamin Smedberg]

cc'ing benwa and smichaud who are the last-touchers of plugin-interpose, even though I'm the original reviewer of record back in bug 621117.

Comment 27

[Peng Kang]

Attachment: New Version Comparison API

I moved CompareVersions functions into mozilla namespace and they are still callable from the outside. In some places CompareVersions are used because otherwise we need to invoke comparisons twice.

I don't know what I should do about /toolkit/xre/MacQuirks.h b/toolkit/xre/MacQuirks.h, so I just left it there...

Comment 28

[(no longer active)]

I've submitted this patch to the try server to see if it builds fine on Mac (and also to see if it passes our tests on all platforms):

http://tbpl.mozilla.org/?tree=Try&rev=55e688722b9c

Comment 29

[Mozilla RelEng Bot]

Try run for 55e688722b9c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=55e688722b9c
Results (out of 152 total builds):
    exception: 1
    success: 133
    warnings: 15
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-55e688722b9c

Comment 30

[Peng Kang]

Hi,

I see all debugs related to this bug are successful. Does this mean the patch works all right?

Comment 31

[Benjamin Smedberg]

Comment on attachment 614403 [details] [diff] [review]
New Version Comparison API

The tryserver build indicates that this patch does not build at all on mac. This is definitely because of the changes to MacQuirks.h. I suggest that if you don't have a mac to build with, you just undo the changes in MacQuirks.h and leave that particular file alone.

> +    mozilla::Version toolKitVersion(gToolkitVersion);
> +    if (mozilla::Version(mAppData->minVersion) > toolKitVersion ||
> +        mozilla::Version(mAppData->maxVersion) < toolKitVersion) {

I don't think you need "toolitVersion" here, just use

if (mozilla::Version(mAppData->minVersion> > toolkitVersion)

In nsVersionComparatorImpl::Compare please reindent the PromiseFlatCString(B) so that it lines up with the first argument.

I'd like to see the final patch again. Thanks for doing this.

Comment 32

[Peng Kang]

Attachment: New Version Comparison API

The file MacQuirks.h is changed back to the original version.

Comment 33

[Peng Kang]

Attachment: New Version Comparison API

MacQuirks.h 's content is restored.

Comment 34

[Benjamin Smedberg]

Comment on attachment 615473 [details] [diff] [review]
New Version Comparison API

Thank you! Please add "r=bsmedberg" to the checkin comment and we can get this landed, probably for the Firefox 15 cycle, since we restricting the Firefox 14 cycle for Android stabilization.

Comment 35

[Peng Kang]

Attachment: New Version Comparison API

Great!

Thank you very much!

Comment 36

[(no longer active)]

Thanks a lot, Peng!

I pushed this to the Birch branch, which will be merged to mozilla-central in less than a week!

https://hg.mozilla.org/projects/birch/rev/c3d4c0bdd4bf

Comment 37

[(no longer active)]

I backed this out because of build failures:

https://hg.mozilla.org/projects/birch/rev/6ed97f081fc3

See the build failure log on Linux for example:

https://tbpl.mozilla.org/php/getParsedLog.php?id=11015210&tree=Birch&full=1#error0

(Here's the link to the full build results: https://tbpl.mozilla.org/?tree=Birch&rev=c3d4c0bdd4bf)

It seems like the patch does not update the call site to NS_CompareVersions in toolkit/mozapps/update/updater/archivereader.cpp.  Peng, would you mind taking a look, please?

Thanks!

Comment 38

[Peng Kang]

Attachment: New Version Comparison API

I forgot to build on the updated mozilla_central. Sorry!

Comment 39

[(no longer active)]

I submitted this to the try server before relanding: http://tbpl.mozilla.org/?tree=Try&rev=f0e51f14ecc9

Comment 40

[Mozilla RelEng Bot]

Try run for f0e51f14ecc9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f0e51f14ecc9
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f0e51f14ecc9

Comment 41

[(no longer active)]

OK, the try server run was all green this time, so I landed this on Birch again:

https://hg.mozilla.org/projects/birch/rev/62c44b96f769

Thanks again!

Comment 42

[Peng Kang]

It's great working with you guys. Thanks!

Comment 43

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/62c44b96f769

Comment 44

[Josh Aas]

I don't see docs on the API that describe how to use the new API. If I call mozilla::CompareVersions(a, b) will it return 1 if a is higher than b or if b is higher than a? And I'm assuming this does return -1, 0, or 1?

I can read the code but I shouldn't have to. Seems like docs should go here:

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsVersionComparator.h

Comment 45

[(no longer active)]

(In reply to Josh Aas (Mozilla Corporation) from comment #44)
> I don't see docs on the API that describe how to use the new API. If I call
> mozilla::CompareVersions(a, b) will it return 1 if a is higher than b or if
> b is higher than a? And I'm assuming this does return -1, 0, or 1?
> 
> I can read the code but I shouldn't have to. Seems like docs should go here:
> 
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsVersionComparator.
> h

Done: http://hg.mozilla.org/mozilla-central/rev/a807b20be0bf

Comment 46

[Peng Kang]

Thank you Ehsan!

I should have done this earlier...