Custom action library, initial version

Custom action library, initial version

[Eric, 2013-10-28 21:55:35]

Finally up. Sorry for the delay. Spent far too much time with Hg getting a clean
version for review.

[Wladimir Palant, 2013-10-29 08:49:26]

This isn't a complete line-by-line review, given that this isn't final code I
merely tried to get an overview. My main concern here is overarchitecturing - we
need only a very simple logic for our custom actions. Also, feel free to take a
look at https://adblockplus.org/en/coding-style - your coding style doesn't
match the rest of our codebase (excessive spaces and class names with
underscores).

http://codereview.adblockplus.org/22887001/diff/1/installer/installer.gyp
File installer/installer.gyp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/installer.gyp#new...
installer/installer.gyp:14: 'msvs_cygwin_shell': 0,
This should be added to defaults.gypi if we need it - not sure why
adblockplus.gyp works without.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/database.cpp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/database.cpp:27: }
I cannot really imagine the situation where our custom action would open any
database but the one belonging to the current installation. This means that we
can definitely simplify the logic here:

Database constructor takes a session reference. It also calls
MsiGetActiveDatabase() to get the database handle and it is obvious why the
destructor closes that handle then. Neither get_active_database() nor a derived
Installation_Database class are necessary.

Note that I generally don't accept the "we might need it at some point" as a
valid argument when introducing complexity into the codebase. If we don't know
right now why we will need it then we don't need it. If the requirements change
later we can still make the approach more complex then - but we don't have the
increased maintenance burden in the meantime.

Frankly, this same logic might apply to the Database object itself - we aren't
developing a generic solution, reading from the database should only be
necessary if we want the custom action to be configurable, something that IMHO
isn't a requirement.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/property.cpp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/property.cpp:29: */
The logic is actually common for any API that doesn't allocate the buffer itself
(and thus mandates which deallocator you have to use). Please reformulate this
comment to use a neutral tone. Dropping the first two lines should be
sufficient, these don't really have any informational value.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/property.cpp:33: DWORD length = sizeof( buffer1 ) /
sizeof( TCHAR ) ;
sizeof(TCHAR) => sizeof(buffer1[0])?

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/property.cpp:54: }
Any reason why this isn't an if..else.. block? Also, this exception isn't very
helpful, it should contain the error code at the very least.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/property.cpp:65: throw std::runtime_error( "Error
setting property" ) ;
This exception isn't very helpful, it should contain the error code at the very
least.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/record.cpp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/record.cpp:2: * \file record.cpp Implementation of
Session class.
Nope, it's the Record class ;)

Generally, I'm unsure that this kind of abstraction is really necessary. Having
a class to abstract away one or two MSI function calls looks like an overkill to
me. We could use the same approach as with the Windows handles here - create an
AutoMSIHandle class that will hold an MSI handle and close it automatically in
the destructor. We can overload the cast to MSIHANDLE to make sure that helper
class can be used with any MSI functions directly.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/session.h (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/session.h:9: #include "record.h"
You should forward-declare the classes instead of the includes here (best
practices, not creating unnecessary dependencies).

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/session.h:83: friend Property::Property( Session &
session, std::wstring name ) ;
The main purpose of this class is holding the session handle - getting access to
it shouldn't be that complicated. I suggest having a public cast overload:

operator MSIHANDLE()
{
  return handle;
}

Side-note: you have one friend declaration in the Session class, the other in
the Immediate_Session class for some reason.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/session.h:143: };
As with the Database object, I'm not a fan of the complexity here - I don't see
why we need a class hierarchy rather than a single Session object (or even an
AutoMSIHandle object).

[Eric, 2013-10-29 14:00:58]

>My main concern here is overarchitecturing - we
>need only a very simple logic for our custom actions. 

See the long comment on Database for an explanation.

>Also, feel free to take a
>look at https://adblockplus.org/en/coding-style - your coding style doesn't
>match the rest of our codebase (excessive spaces and class names with
>underscores).

I do admit I find code with more-than-average whitespace easier to read than
what most people write. (That includes using underscores as internal whitespace
in identifiers.) The longer I've coded, the more I've come to appreciate
whitespace. Having originally coded on 80x25 terminal interfaces, I understand
where compactness came from. To be sure, there's no actual statement on this
kind of whitespace in the coding style document, either directly nor in the
Mozilla one incorporated by reference.

I did at least remember to use 2-space indents, which I find too shallow for my
personal taste.

We can change the underscores to camelCase if you like. (Personally, I find it
noticeably slower to read.)

http://codereview.adblockplus.org/22887001/diff/1/installer/installer.gyp
File installer/installer.gyp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/installer.gyp#new...
installer/installer.gyp:14: 'msvs_cygwin_shell': 0,
On 2013/10/29 08:49:26, Wladimir Palant wrote:
> This should be added to defaults.gypi if we need it - not sure why
> adblockplus.gyp works without.

This declaration is in /libadblockplus/common.gypi, which appears in the command
line of /createsolution.bat. It's the only thing from that file needed here.

There would be no harm, I think, duplicating it in defaults.gypi, but I've left
any such kind of modification out of this branch for now.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/database.cpp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/database.cpp:27: }
Before I get into everything, let me start off by saying that if this code were
only appear only in the CA, I would completely agree and we'd have a single
class representing the installation database. Yet the bad news is that we may
need this outside the CA. The reason for a database class hierarchy is that
there's a decent chance we'll need to build some custom build tools. Initially,
I wrote this just to wrap my head around the Windows Installer API, which makes
a bunch of Windows-like assumptions. Later, I realized we might need to keep it.

Here's a brief version why.

1) Getting automatic translation means generating a transform file and
incorporating it into an MSI file.

2) The existing tools to do this have deficits.
2a) There's no WiX tool for one piece, and the WiX tool for another has a
defect.
2b) The Microsoft tools, while freely available, don't seem to be packaged by
default in Visual Studio (at least the Express edition). That means incurring
yet another external dependency. The tool suite is called "Windows Installer
Development Tools"
http://msdn.microsoft.com/en-us/library/windows/desktop/aa372834%28v=vs.85%29...
. From that page:
>The following tools are only available in the Windows SDK Components for
Windows Installer Developers.
2c) There are some tools out there (both Microsoft and independent) that use the
Automation interface with the Windows Scripting Host. While I may be willing to
use Microsoft NMAKE, this is beyond even me.

3) All these tools seem to be largely wrappers around the Windows Installer API.
For example, the tool "msitran.exe" (which generates transforms, MST files)
seems to be mostly a wrapper around the API call MsiDatabaseGenerateTransform.
All the heavy lifting is already done, so it looks to be feasible just to write
our own wrappers.

4) None of the command line tools are targeted at the use case we have. That
means we'll need some kind of top-level logic for the build regardless. We can
put it into the build system (with a script, in all likelihood) or into our own
tool. If we want to avoid a new dependency to the code base and end up writing
our own tool, there's no need to script it for the build system separately.

5) The Windows Installer API uses a single kind of handle for anything involving
a database, whether open during a CA or in a tool. Some of the API calls that
take databases overlap, some don't.

Therefore, given the possibility that we need tools, I spent a small bit of time
putting in the stubs for a class hierarchy, but more importantly, writing
documentation for what was happening for future recall.

If we end up needing our own tools, we'll want the code. If we don't, we can
remove it.

(Note: This comment likely deserves to be copied into Discourse.)

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/property.cpp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/property.cpp:29: */
I'll rewrite the comment. Reading it again, it doesn't capture the API behavior
adequately to explain the code.

FYI, the Windows registry call does something similar, but with behavior
different enough that you can't use generic code to handle both cases. That's
what's the screwy part.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/property.cpp:33: DWORD length = sizeof( buffer1 ) /
sizeof( TCHAR ) ;
On 2013/10/29 08:49:26, Wladimir Palant wrote:
> sizeof(TCHAR) => sizeof(buffer1[0])?

TCHAR is, as I recall, a wchar_t for Unicode compiles and a char otherwise. The
syntax is a way of avoiding another line of code to define the buffer length 64
separately.

I don't know how much using TCHAR matters here. I cribbed using this type from
the WiX CA code.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/property.cpp:54: }
On 2013/10/29 08:49:26, Wladimir Palant wrote:
> Any reason why this isn't an if..else.. block? Also, this exception isn't very
> helpful, it should contain the error code at the very least.

There are three other error codes defined for the API. (As also for the above
switch statement.) I hadn't added them in yet; I don't know if they'll matter.

See also below.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/property.cpp:65: throw std::runtime_error( "Error
setting property" ) ;
On 2013/10/29 08:49:26, Wladimir Palant wrote:
> This exception isn't very helpful, it should contain the error code at the
very
> least.

Admittedly, the exceptions here are stubs. I don't think that any of the errors
ever occur unless the API itself crashes (a Windows defect) or the code calling
them messes up (an application defect). This comment applies to a most of the
API calls in this code base.

Given how prevalent this pattern is, though, what's needed an exception class
that takes a message and a Windows error code. I haven't written it yet.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/record.cpp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/record.cpp:2: * \file record.cpp Implementation of
Session class.
On 2013/10/29 08:49:26, Wladimir Palant wrote:
> Nope, it's the Record class ;)
Oops. Bad copypasta.

> Generally, I'm unsure that this kind of abstraction is really necessary.
Having
> a class to abstract away one or two MSI function calls looks like an overkill
to
> me.

For the main reason this class exists, see the explanation on class Database.
Class Record is separate because it's used both in database queries and also as
an argument vector for MsiProcessMessage, which is currently used for logging
and we expect to use for dialog boxes in the CA proper. If we need offline
database tools, then we'll want this class. If we don't, then the right thing to
do is just to incorporate this whole class into a wrapper around
MsiProcessMessage. I don't know yet what we'll need.

> We could use the same approach as with the Windows handles here - create an
> AutoMSIHandle class that will hold an MSI handle and close it automatically in
> the destructor. We can overload the cast to MSIHANDLE to make sure that helper
> class can be used with any MSI functions directly.

That would be nice, if possible. The problem is that some handles are allocated
within the API as side effects, and you don't close those explicitly. Some (like
the one here for Record) do need to be explicitly closed. It's just not
consistent. On the other hand, I don't see a need to go full generic here and
use a trait class to manage the differences in handle lifetimes either; that's
premature at the very best.

I don't know what the case is with lifetimes for record handles generally.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/session.h (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/session.h:9: #include "record.h"
"record.h" I think is some dead code I forgot to remove. I'll have to test that.

"property.h" is necessary for the friend declaration.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/session.h:83: friend Property::Property( Session &
session, std::wstring name ) ;
On 2013/10/29 08:49:26, Wladimir Palant wrote:
> The main purpose of this class is holding the session handle - getting access
to
> it shouldn't be that complicated.

Well, it's a design goal of mine to hide the handles entirely. Lifetime
management of these things isn't consistent (see comment on a previous file),
and so my preference was to make the right thing happen in the class code rather
than exposing them at all.

[Wladimir Palant, 2013-10-29 15:06:38]

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/database.cpp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/database.cpp:27: }
Ok, I understand it that we will need a tool to generate MST files. But is it
really a good idea to write that tool in C++? I would prefer Python (with
ctypes) which is a more maintainable option.

Even *if* we want to use C++ for that, I don't see why we want to share code
between custom actions and that tool. From all I can tell, there is exactly zero
overlap between the MSI functions that these need to use.

Feel free to move the general discussion to Discourse.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/property.cpp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/property.cpp:33: DWORD length = sizeof( buffer1 ) /
sizeof( TCHAR ) ;
On 2013/10/29 14:00:58, Eric wrote:
> On 2013/10/29 08:49:26, Wladimir Palant wrote:
> > sizeof(TCHAR) => sizeof(buffer1[0])?
> 
> TCHAR is, as I recall, a wchar_t for Unicode compiles and a char otherwise.

What I meant: you can replace sizeof(TCHAR) by sizeof(buffer1[0]) - this is the
generic way of calculating the size of an array, without having to mention its
type again.

But now that you mention it: yes, you should use WCHAR instead of TCHAR and call
MsiGetPropertyW() explicitly. Given that the result is a wstring, we don't want
generic behavior here - we want to get Unicode no matter what.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/property.cpp:65: throw std::runtime_error( "Error
setting property" ) ;
Forgot to mention that: where are these exceptions caught? If they aren't (which
is what it looks like right now), does it mean that the installer will crash if
it hits an unexpected situation?

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/record.cpp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/record.cpp:2: * \file record.cpp Implementation of
Session class.
On 2013/10/29 14:00:58, Eric wrote:
> > We could use the same approach as with the Windows handles here - create an
> > AutoMSIHandle class that will hold an MSI handle and close it automatically
in
> > the destructor. We can overload the cast to MSIHANDLE to make sure that
helper
> > class can be used with any MSI functions directly.
> 
> That would be nice, if possible. The problem is that some handles are
allocated
> within the API as side effects, and you don't close those explicitly.

Well, these handles just don't need to be wrapped inside AutoMSIHandle then...

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/session.h (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/session.h:83: friend Property::Property( Session &
session, std::wstring name ) ;
On 2013/10/29 14:00:58, Eric wrote:
> Well, it's a design goal of mine to hide the handles entirely. Lifetime
> management of these things isn't consistent (see comment on a previous file),
> and so my preference was to make the right thing happen in the class code
rather
> than exposing them at all.

Lifetime management is easily solved - either you create an AutoMSIHandle
wrapper for the handle or not (the latter being an exception, for the hInstall
handle of the custom action only). As I said, I don't see the value of having a
class to abstract away a single MSI call (and we have a dozen of such classes
here). IMHO that design goal is flawed.

[Eric, 2014-03-28 13:56:38]

Old but relevant comment I forget to write down earlier.

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
File installer/src/installer-lib/property.cpp (right):

http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
installer/src/installer-lib/property.cpp:65: throw std::runtime_error( "Error
setting property" ) ;
On 2013/10/29 15:06:38, Wladimir Palant wrote:
> Forgot to mention that: where are these exceptions caught? If they aren't
(which
> is what it looks like right now), does it mean that the installer will crash
if
> it hits an unexpected situation?

The answer is a general one. 

Every time you use C++ outside of your own main(), you need to provide a
top-level try statement with a catch-all (that is, "catch(...)") block to catch
exceptions. You need this even if your own code doesn't throw, since both the
standard library and even the language itself can throw. (Divide by zero
evidently throws on certain platforms.)

For a DLL, the top levels include the DLL initialization code and the DLL entry
points. In addition, callbacks from the operating system should be considered
top-level. The rule of thumb is that if you need __stdcall (or WINAPI, a typedef
to it) to declare a function, you need a top-level try statement around the body
of that function.

[Oleksandr, 2014-07-15 14:20:46]

On 2014/03/28 13:56:38, Eric wrote:
> Old but relevant comment I forget to write down earlier.
> 
>
http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
This can be closed, since it's not relevant anymore and we have the Whole
Installer review now.

> File installer/src/installer-lib/property.cpp (right):
> 
>
http://codereview.adblockplus.org/22887001/diff/1/installer/src/installer-lib...
> installer/src/installer-lib/property.cpp:65: throw std::runtime_error( "Error
> setting property" ) ;
> On 2013/10/29 15:06:38, Wladimir Palant wrote:
> > Forgot to mention that: where are these exceptions caught? If they aren't
> (which
> > is what it looks like right now), does it mean that the installer will crash
> if
> > it hits an unexpected situation?
> 
> The answer is a general one. 
> 
> Every time you use C++ outside of your own main(), you need to provide a
> top-level try statement with a catch-all (that is, "catch(...)") block to
catch
> exceptions. You need this even if your own code doesn't throw, since both the
> standard library and even the language itself can throw. (Divide by zero
> evidently throws on certain platforms.)
> 
> For a DLL, the top levels include the DLL initialization code and the DLL
entry
> points. In addition, callbacks from the operating system should be considered
> top-level. The rule of thumb is that if you need __stdcall (or WINAPI, a
typedef
> to it) to declare a function, you need a top-level try statement around the
body
> of that function.

[Eric, 2014-07-18 16:41:48]

See review for "Whole installer"
http://codereview.adblockplus.org/6202981292703744/