Issue #41 - Bring method of determining IE version up to date

src/shared/Utils.cpp

 #include <memory>
 #include <stdexcept>
 #include <vector>
 
 #include <Windows.h>
 #include <ShlObj.h>
 
 #include "Utils.h"
 
 namespace
@@ skipping 110 matching lines @@
 }
 
 void ReplaceString(std::wstring& input, const std::wstring placeholder, const std::wstring replacement)
 {
   size_t replaceStart = input.find(placeholder);
   if (replaceStart != std::string::npos)
   {
     input.replace(replaceStart, placeholder.length(), replacement);
   }
 }
+
+namespace
+{
+  /**
+   * An open key in the system registry.
+   */
+  class Registry_Key

[Oleksandr, 2014/07/15 18:25:11]

I'd vote to move class definition to Utils.h and leave only implementation here.
Or better yet to have something like Registry.h and Registry.cpp.

[Eric, 2014/07/20 18:18:46]

We actually need a more radical organization than that. For issue 404 "Validate
Internet Explorer version in the installer", we should have this code in a
library shared both by the plugin/engine and the installer. We don't have a
place for that yet.

https://issues.adblockplus.org/ticket/404

I'd recommend a second change set where we alter the build system as well as
move this code into its own source files.

+  {
+    /**
+     * Handle to an open registry key.
+     */
+    HKEY key;
+
+    /**
+     * Flag whether the registry key is predefined or not.
+     *
+     * Predefined key values are considered "always open" and should not be closed in the destructor.
+     */
+    bool is_predefined;
+
+  public:
+    /**
+     * Enumeration for predefined root keys.
+     */
+    enum class Predefined
+      : unsigned int

[Oleksandr, 2014/07/15 18:25:11]

I don't understand the concept behind this class. Can't we just pass stuff like
HKEY_CLASSES_ROOT to our functions directly? Why do we need to map these
constants to our enum?

[Eric, 2014/07/20 18:18:46]

The way the Windows registry API works, you must always pass some predefined
registry key to the API. The enum isolates a dependency on having the symbols
floating around to an implementation class. It doesn't matter for the narrow
purpose in this class, but when we move the code to act as more of a library
function, we don't need to incur a header dependency defining and of the
predefined keys.

Alternately, we could change the constructor signature to accept another
'Registry_Key' and then predefine some static class members. That seems less
efficient. The other alternative, using an HKEY in the constructor signature,
doesn't accomplish the goal of isolating header dependencies to the
implementation.

[Oleksandr, 2014/07/25 22:23:46]

I think this is an overkill. I'd be ok if we assumed that the caller has
HKEY_CLASSES_ROOT and stuff defined. It gets defined by windows.h after all.
Let's get a third opinion here. @Sergei?

On 2014/07/20 18:18:46, Eric wrote:

[Eric, 2014/07/26 15:54:45]

I'm inclined to agree with you for practical reasons. The current implementation
doesn't even satisfy my own criteria for eliminating the need to include
windows.h. The trouble is HKEY, which is declared a pointer to an
otherwise-unused class. I tried using void * instead, but I was getting some
errors with type conversions, which I'm pretty sure it were defects in VS 2012.
Whatever. I'm giving up on perfect encapsulation.

I'll do a rework in a change set to follow the present one on formatting.

+    {
+      HKCR = 0,   // HKEY_CLASSES_ROOT
+      HKCC,       // HKEY_CURRENT_CONFIG
+      HKCU,       // HKEY_CURRENT_USER
+      HKLM,       // HKEY_LOCAL_MACHINE
+      HKU         // HKEY_USERS
+    };
+
+    /**
+     * Constructor to open a key starting an one of the predefined roots.
+     * Opens the key with read-only access.
+     */
+    Registry_Key( Predefined root, std::wstring key_name );
+
+    /**
+     * Destructor closes the key if it's not a predefined one.
+     */
+    ~Registry_Key();
+
+    /**
+     * Retrieve a value from a name-value pair within the present key.
+     *
+     * Throws if the name is not found within the dictionary.
+     * Throws if the name is found but is not a string.
+     */
+    std::wstring value_wstring( std::wstring name ) const;
+
+  private:
+    /**
+     */
+    inline HKEY hkey_of_predefined( Predefined root )
+    {
+      static HKEY predefined_hkeys[5] = 
+      {
+        HKEY_CLASSES_ROOT,
+        HKEY_CURRENT_CONFIG,
+        HKEY_CURRENT_USER,
+        HKEY_LOCAL_MACHINE,
+        HKEY_USERS
+      };
+      return predefined_hkeys[ (unsigned int) root ];
+    }
+  };
+
+  Registry_Key::Registry_Key( Predefined root, std::wstring key_name )
+    : is_predefined( key_name.empty() )
+  {
+    HRESULT hr = RegOpenKeyExW( hkey_of_predefined( root ), key_name.c_str(), 0, KEY_QUERY_VALUE, &key );
+    if ( hr != ERROR_SUCCESS || !key )
+    {
+      throw std::runtime_error( "Failure in RegOpenKeyExW" );
+    }
+  }
+
+  Registry_Key::~Registry_Key()
+  {
+    if ( ! is_predefined )
+    {
+      RegCloseKey( key );
+    }
+  }
+
+  std::wstring Registry_Key::value_wstring( std::wstring name ) const
+  {
+    /*
+     * Step one is to determine the presence of the value, along with its type and byte size. 
+     */
+    DWORD type;
+    DWORD size = 0;
+    HRESULT hr = ::RegQueryValueExW( key, name.c_str(), 0, &type, 0, &size );
+    if ( hr != ERROR_SUCCESS )
+    {
+      throw std::runtime_error( "Failure in RegQueryValueEx to query name" );
+    }
+    if ( type != REG_SZ )
+    {
+      throw std::runtime_error( "Value is not string type" );

[Oleksandr, 2014/07/15 18:25:11]

Just returning an empty string and logging the error would be fine in
"exceptional" cases here, IMHO. That's what we do in the end anyway. This would
also eliminate all the try-catch blocks.

[Eric, 2014/07/20 18:18:46]

As a rule, eliminating exception handling shouldn't be a goal in itself.

In the present case, the try-catch statements in the version functions below
could be eliminated _if_ we had exception handlers at all the entry points
already. We don't, so they're in the code below as a safety net.

As for this particular exception, unless IE is malfunctioning reporting its own
version number or something has been playing in the registry, this condition
should never be hit. That's exactly what exceptions are for. Logging the
exception can happen once in the code, at the highest level, rather than having
it sprinkled throughout the code base.

+    }
+
+    /*
+     * Step two is to allocate a buffer for the string and query for its value.
+     * Note that 'size' is the size in bytes, which we need for the system call,
+     *   but that 'psize' is the size in words, which we need to manipulate the wchar_t array 'p'.
+     */
+    // Round the byte size up to the nearest multiple of two.
+    size = ( size + 1 ) & ~ DWORD(1);
+    size_t psize = size >> 1;
+    // We need to allocate a temporary buffer to receive the value, because there's no interface to write directly into the buffer of std::basic_string.
+    std::unique_ptr< wchar_t[] > p( new wchar_t[ psize ] );

[Oleksandr, 2014/07/15 18:25:11]

std::wstring p;
p.resize(psize);
RegQueryValueExW(...., &p[0],...)

should work just as fine 

[Eric, 2014/07/20 18:18:46]

Except that string buffers are 'const' and you're not supposed to write into
them.

Even if it might not cause problems at the present, it's still a violation of
contract and may cause problems in the future.

+    hr = RegQueryValueExW( key, name.c_str(), 0, 0, reinterpret_cast<BYTE *>( p.get() ), &size );
+    if ( hr != ERROR_SUCCESS )
+    {
+      throw std::runtime_error( "Failure in RegQueryValueExW to retrieve value" );
+    }
+
+    /*
+     * Step three is to construct a return string.
+     *
+     * There's the possibility of an extra terminating null character in the return value of the query.
+     * If so, we have to decrement the length of the return value to eliminate it.
+     * If it persists, it will interfere with later string operations such as concatenation.
+     */
+    if ( p[ psize - 1 ] == L'\0' )
+    {
+      --psize;
+    }
+    return std::wstring( p.get(), psize );  
+  }
+
+  /**
+   * Internal implementation of the IE version string.
+   *
+   * This version throws exceptions for its errors, relying on its caller to handle them.
+   *
+   * Quoting http://support.microsoft.com/kb/969393:
+   *   "Note The version string value for Internet Explorer 10 is 9.10.9200.16384, and the svcVersion string value is 10.0.9200.16384."
+   * [EH 2014-06-20] My current version of IE 11 is reporting these values:
+   *   Version 9.11.9600.17041
+   *   svcVersion 11.0.9600.17041
+   */

[Oleksandr, 2014/07/15 18:25:11]

I think it's better to remove most of the comments in this file. What's going on
in the code should be self evident, and if it's not - one should just double
check against the documentation.

[Eric, 2014/07/20 18:18:46]

Well, that's what the problem is, that the documentation for this isn't exactly
a quick read, nor is it completely clear. Once you know what's going on, it's
clear enough, but that's not the standard of judgement for whether a comment is
needed.

Also, this comment is in a Doxygen header, since it's part of the specification
of the function itself, rather than just an implementation detail.

+
+  std::wstring IE_version_string()
+  {
+    Registry_Key IE_key( Registry_Key::Predefined::HKLM, L"Software\\Microsoft\\Internet Explorer" );
+    std::wstring version( IE_key.value_wstring( L"Version" ) );
+    /*
+     * We're expecting a version string that matches the regex "^[1-9]\.".
+     * Since IE versions 10 and 11 use a version string that begins with "9.", this simplistic parsing method works adequately.
+     * Warning: IE version 12 and later might not behave the same, so this function should be reviewed with each new release.
+     */
+    if ( version[1] != '.' || version[0] < L'0' || version[0] > L'9' )
+    {
+      throw std::runtime_error( "IE version string has unexpected format" );
+    }
+    if ( version[0] != L'9' )
+    {
+      return version;
+    }
+    // Assert the major version in the "Version" value is 9.
+    /*
+     * Version 9 might either be an actual version 9 or it might represent a version >= 10
+     * If the value named "svcVersion" exists, we'll report that instead.
+     */
+    try
+    {
+      return IE_key.value_wstring( L"svcVersion" );
+    }
+    catch ( ... )
+    {
+      return version;
+    }
+  }
+}
+
+
+std::wstring ABP::IE::installed_version_string()
+{
+  try
+  {
+    return IE_version_string();
+  }
+  catch ( ... )

[Oleksandr, 2014/07/15 18:25:11]

Catching specifically std::runtime_error would make more sense here

[Eric, 2014/07/20 18:18:46]

I referred to this 'catch' block above. If we had proper exception handling
throughout the code, I'd just eliminate the try-catch block here in its entirety
(as well as the one below). The user of this function is the one that has to
decide how it's going to behave differently if there's a failure in retrieving
the version number.

+  {
+    return L"";
+  }
+}
+
+int ABP::IE::installed_major_version()
+{
+  try
+  {
+    std::wstring version = IE_version_string();
+    /*
+     * If the second character is a period, we assume that the major version is a single digit.
+     */
+    if ( version[ 1 ] == L'.' )
+    {
+      return version[ 0 ] - L'0';
+    }
+    // Assert IE_version_string() did not verify the syntax of the version string.
+    /*
+     * In this case, we'll assume that the version returned is two digits followed by a period.
+     * If not, we'll assume an error.
+     */
+    if ( version[ 0 ] < L'0' || version[ 0 ] > L'9' || version[ 1 ] < L'0' || version[ 1 ] > L'9' || version[ 2 ] != L'.' )
+    {
+      return 0;
+    }
+    return 10 * ( version[ 0 ] - L'0' ) + ( version[ 1 ] - L'0' );
+  }
+  catch ( ... )
+  {
+    return 0;
+  }
+}
+
+