Issue 5643369099296768: Issue 1147 - Fix installation when IE is opened (Closed)
DescriptionThis does in fact fix the installer workflow - it works (like Eric suggested). However the debug message about leaked MSIHANDLEs still persists. I have created a separate issue for that https://issues.adblockplus.org/ticket/1187 for installer cleanup project.
Patch Set 1 #
Total comments: 6
MessagesTotal messages: 10
LGTM, because the one issue I have is not a defect. It's just a bit of almost-certainly redundant code that has little effect on total quality. http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/inst... File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:25: if ((procHandle == NULL) || (procHandle == INVALID_HANDLE_VALUE)) return false; This line is redundant. The previous check on tmpHandle takes care of the NULL case and OpenProcess doesn't return INVALID_HANDLE_VALUE (not according to the documentation). On the other hand, INVALID_HANDLE_VALUE is -1, so this line is also harmless. If you don't want to remove the line for extra-safety, at least make a comment that it's redundant.
Despite I'm not a reviewer here and it's already committed, I will leave my comment anyway. http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/inst... File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:25: if ((procHandle == NULL) || (procHandle == INVALID_HANDLE_VALUE)) return false; - There is no reason to have tmpHandle. - As Eric pointed, the non critical trouble here is only additional comparison against INVALID_HANDLE_VALUE. just make the `if (...` line like this `if (NULL == procHandle)` http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:36: Windows_Module_Handle user32Dll(tmpModule); Similar here. The problem was in the missed validation `if (NULL == user32Dll)`, there is no need for tmpModule.
http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/inst... File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:25: if ((procHandle == NULL) || (procHandle == INVALID_HANDLE_VALUE)) return false; On 2014/08/08 15:17:36, sergei wrote: > - There is no reason to have tmpHandle. No. See below. http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:36: Windows_Module_Handle user32Dll(tmpModule); On 2014/08/08 15:17:36, sergei wrote: > Similar here. The problem was in the missed validation `if (NULL == user32Dll)`, > there is no need for tmpModule. It would help to understand the class in question first. The defect before was that there was no temporary variable. These handle classes throw an exception when constructed with nullptr as the argument. The smart pointer template is instantiated with a not-null policy.
http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/inst... File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:36: Windows_Module_Handle user32Dll(tmpModule); > These handle classes throw an exception when constructed with nullptr as the argument. But why? No one standard smart pointer or wrapper does it, such behaviour looks superfluous restriction.
On 2014/08/08 15:45:24, sergei wrote: > But why? Because the template is instantiated with a non-null policy. Didn't I say that already? If you allow a null object in, it violates the class invariant that it's not null, so the constructor has to throw an exception. > No one standard smart pointer or wrapper does it, such behaviour looks > superfluous restriction. I read this argument as saying "I don't want to read documentation." It's all pretty clear when you look at the code. Instantiating the template with a not-null policy makes the class behave more like a reference. Instantiating with a null-allowed policy makes it behave more like a pointer. Are you actually arguing against references? Also see std::reference_wrapper in <functional>.
On 2014/08/08 16:29:35, Eric wrote: > On 2014/08/08 15:45:24, sergei wrote: > > But why? > > Because the template is instantiated with a non-null policy. Didn't I say that > already? I don't understand why you react this way. It's evident from what you said that you understood what Sergei meant: Why does the handle wrapper check for null. It's a valid question. > > No one standard smart pointer or wrapper does it, such behaviour looks > > superfluous restriction. > > I read this argument as saying "I don't want to read documentation." I don't want to read documentation either, if it can be avoided. Handles can be null, a wrapper for a handle should be able to represent that state. And as Sergei said, smart pointers generally accept null. So can our AutoHandle.
On 2014/08/08 16:37:52, Felix H. Dahlke wrote: > I don't understand why you react this way. I have become increasingly frustrated over the last couple of week dealing with strident comments based in ignorance. > It's evident from what you said that > you understood what Sergei meant No, I really don't understand any more. I can't tell it's a stalking horse for something else, a naive question based on ignorance, a personal prejudice, or legitimate technical concern. > Why does the handle wrapper check for null. > It's a valid question. And I've answered it twice. It's instantiated with a non-null policy. The same module also contains a null-permitted policy, and it's possible to instantiate the template with that one, too. This particular class is not instantiated with a null-permitted policy; it's instantiated with the not-null policy. It's a reasonable question why that template is instantiated the way it is. I can't see that anybody has actually asked that question yet, though. In the narrow instance of the present function, it likely doesn't need to be. Then again, the simple fix (already committed) is just to check for null before instantiation. > I don't want to read documentation either, if it can be avoided. The whole MSI system is the single worst piece of software I have ever had to deal with. As a rule of thumb, it is completely impossible to do _anything_ related to MSI with wading through a ton of documentation. The whole reason I wrote a policy-based handle class was to deal with all that nonsense moderately sanely. Part of it was the option to have a non-null policy.
Some more comments below. As for this review, it's LGTM from my side. We should focus on the fix here, not on a broader scale refactorings. That's what issue 1184 is for, I've created one for this topic: https://issues.adblockplus.org/ticket/1189#ticket On 2014/08/08 17:16:23, Eric wrote: > No, I really don't understand any more. I can't tell it's a stalking horse for > something else, a naive question based on ignorance, a personal prejudice, or > legitimate technical concern. Definitely the latter here. I would have written the same, in a similar way. > > Why does the handle wrapper check for null. > > It's a valid question. > > And I've answered it twice. It's instantiated with a non-null policy. That boils down to: It checks for null because it checks for null. > The same > module also contains a null-permitted policy, and it's possible to instantiate > the template with that one, too. This particular class is not instantiated with > a null-permitted policy; it's instantiated with the not-null policy. IMO that would have made more sense here - the value CAN be null, that way we could check for it without the temporary variable. > > I don't want to read documentation either, if it can be avoided. > > The whole MSI system is the single worst piece of software I have ever had to > deal with. As a rule of thumb, it is completely impossible to do _anything_ > related to MSI with wading through a ton of documentation. I was talking about documentation of our own code. Consistency can help a lot here: We already have a handle wrapper that does handle null as a value. Ideally we would just use that one here. > The whole reason I wrote a policy-based handle class was to deal with all that > nonsense moderately sanely. Part of it was the option to have a non-null policy. I can see the point of that, but I think it'd be helpful to have two typedefs for that, let's say: WindowsHandle and NonnullWindowsHandle. Since handles can be null, I intuitively expect a handle wrapper to handle (pun intended) that too. Both Sergei and I assumed this, I believe that backs the point up.
On 2014/08/08 17:43:58, Felix H. Dahlke wrote: > IMO that would have made more sense here - the value CAN be null, that way we > could check for it without the temporary variable. I'm not going to disagree. Windows_Handle was used in only in a single place in the code previously, and it was advantageous to declare it not-null to simplify the code of its first usage. As a rule, reference-style classes lead to cleaner code, since many objects tend to be used more than they're constructed. In that case of, say, a factory function, checking for null explicitly before construction isolates all the null checking in that one place, and it eliminates it everywhere else. The present case it's used as a transient value, and using a reference-style declaration is slightly wordier. > I was talking about documentation of our own code. Consistency can help a lot > here: We already have a handle wrapper that does handle null as a value. Ideally > we would just use that one here. Windows_Handle was needed after the handle generic was already written. It was very quick to make it work. typedef handle< HANDLE, Disallow_Null, Windows_Generic_Destruction > Windows_Handle I think I had to write Windows_Generic_Destruction, but that's close to a one-liner around CloseHandle(...). > I think it'd be helpful to have two typedefs > for that, let's say: WindowsHandle and NonnullWindowsHandle. Since handles can > be null, I intuitively expect a handle wrapper to handle (pun intended) that > too. Both Sergei and I assumed this, I believe that backs the point up. typedef handle< HANDLE, Allow_Null, Windows_Generic_Destruction > WindowsHandle typedef handle< HMODULE, Allow_Null, Windows_Generic_Destruction > ModuleHandle typedef handle< HANDLE, Disallow_Null, Windows_Generic_Destruction > WindowsHandleRef Probably easier to just to make these declarations. Frankly, you don't want something named AutoHandle in the installer code because there are so many different kinds of handles running around; AutoHandle just isn't specific enough a name. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
