Clean up IDispatch::Invoke implementations

[eric@adblockplus.org]

Background

We have two functions in the plugin code that implement IDispatch, CPluginUserSettings and CPluginClass. Their implementations of Invoke are inconsistent and difficult to read, mixing Windows-specific interface code with plugin behavior.

Cleaning up this code is related to eliminating ATL, as the classes ATL::CComBSTR and ATL::CComVariant have both been used in the interface code. Nevertheless, the scope of this work is greater than just these two classes, as there are a number of other types at play.

What to change

• Eliminate sequential if-else if string comparison in favor of a switch to determine which method was called (CPluginUserSettings)
• Make argument handling (number of arguments, type checking, conversion to standard types) consistent and clear.
• Make return value handling consistent and clear.
• Isolate argument handling to these functions only. Eliminate delegation where DISPID instances are passed out of the Invoke function body.
• Factor out non-trivial dispatch method implementations to separate functions.
• Eliminate explicit life cycle management of Invoke results.

[sergz]

• I would like to have all these methods in idl file and implemented as overridden methods. It much more descriptive and covers the following mentioned items

• Make argument handling (number of arguments, type checking, conversion to standard types) consistent and clear.
• Make return value handling consistent and clear.
• Isolate argument handling to these functions only. Eliminate delegation where DISPID instances are passed out of the Invoke function body.
• Factor out non-trivial dispatch method implementations to separate functions.

• It's not a problem, it depends on the naming convention, but properties for some cases look better than setters/getters. It might be not for this place or for this round, because JS-part will require changes as well.

• What does

  Eliminate explicit life cycle management of Invoke results.
  

mean? We should follow the rules in COM world, ​http://msdn.microsoft.com/en-us/library/windows/desktop/ms686638(v=vs.85).aspx .
Does it mean, that we should prepare the result in some smart type and about the exit do something like

SmartType xxx;
...
retValue = xxx.Detach();
return hr;

or

SmartType xxx;
...
return xxx.CopyTo(retValue);

• Also it highly depends on the decision about using of ATL. ATL can save us here from doing a lot of thing manually and from mistakes.

[eric@adblockplus.org]

• There's no need to add more IDL than we already use, and indeed, what we already use could be eliminated. Already the type library being generated isn't being used. The details are down in how IE binds to a BHO, but in short, it doesn't even look at the type library. Nor does v8 need a type library; indeed from my experience in the debugger, it doesn't even seem to be caching Dispatch ID's.

• Changing the interface of the CPluginUserSettings (the C++ object) and Settings (the JS object) is very much indeed beyond the scope of this work. I'm not saying whether or not we want to do it, but if we do we should clean up the implementation first.

• On smart types

  we should prepare the result in some smart type
  

  Yes, that's what I mean overall, although your code example doesn't match how a COM server returns a result value (as opposed to its status return value). Regardless, the principle is to embody the lifecycle requirements into shared code so that we aren't doing it manually at each occurrence.

You can put all the life cycle management requirements in the constructors and destructors of such smart classes. In the case of IDispatch::Invoke in particular, argument and results are (in general) VARIANT, which also benefits from consistent conversion to and from standard C++ types.

Note: The existing ATL classes do _not_ do the right thing in all cases. They have their own lifecycle requirements that sometimes overlap with and sometime conflict with the requirements of COM parameters. And the ATL classes absolutely do not perform conversions to standard types well.

[sergz]

I would like to say that we indeed should define as much as possible in the idl file, because it is the best place to define COM interfaces. So everybody can open the idl-file and see the API without investigating of our implementation of IDispatch::Invoke (which firstly should be found). Such goals are stated in the issue:

Make argument handling (number of arguments, type checking, conversion to standard types) consistent and clear.
Make return value handling consistent and clear.
Isolate argument handling to these functions only. Eliminate delegation where DISPID instances are passed out of the Invoke function body.
Factor out non-trivial dispatch method implementations to separate functions.

Defining the interface in idl-file it becomes absolutely clear and more consistent. For example, compare

// returns comma separated list of domains
HRESULT GetWhitelistDomains([out, retval] BSTR* retValue);

vs

  // Currently somewhere in the middle of a long Invoke method
  else if (s_GetWhitelistDomains == method)
  {
    if (pDispparams->cArgs)
      return DISP_E_BADPARAMCOUNT;

    if (pVarResult)
    {
      std::vector<std::wstring> whiteList = settings->GetWhiteListedDomainList();
      CString sWhiteList;
      for (size_t i = 0; i < whiteList.size(); i++)
      {
        if (!sWhiteList.IsEmpty())
        {
          sWhiteList += ',';
        }
        sWhiteList += CString(whiteList[i].c_str());
      }

      pVarResult->vt = VT_BSTR;
      pVarResult->bstrVal = SysAllocString(sWhiteList);
    }
  }

It might be not the good example, but I guess it's enough to demonstrate the problems. JIC, one can get acquainted with Invoke method here ​http://msdn.microsoft.com/en-us/library/windows/desktop/ms221479(v=vs.85).aspx .
My comparison:

• In the current case: Manual mapping from DISPID to string, which is not only necessary, it's even wrong. The method GetIDsOfNames especially exists to properly map string method name to DISPID. As always, it's additional manual work with the place for mistakes. We don't respect some useful things (wFlags, exception info, puArgErr), which make the code less robust and dirty. We need to write dummy tests which test whether there is a method with the specified arguments, and it's not mere additional work, it's also a good place for mistakes.
• In the proposed case: One has to override the virtual method, compiler will notify us if we make some mistake in the name spelling or define something wrong in the arguments. The number of arguments, types of the arguments and the return value are clear. We don't have to fiddle around with VARIANT (I cannot say that here we correctly handle pVarResult), we precisely define the types in the method declarations. We don't need to implement GetIDsOfNames and Invoke methods which are very error prone. I'm not commenting here the comma separated string instead of exposing a collection, I expect here a lot of critics arguing that it's too sophisticated, I agree, it may be not easy.

What is wrong with the variant here? Firstly, it's just a good practice to set it to empty value at the beginning. It often happens when programmers don't initialize some variables (I had a lot of such experience working with Windows Explorer and I don't see any reason to repeat it again), for example.

VARIANT v;
someCall(&v);
if (myValid(v)) {
  myFree(v); // in the best case it will crash because v is not empty, there is a garbage from the stack.
}

The second thing someCall should do is to clear the arguments for the return value, the same as for double pointer. Here someCall should call VariantInit. JIC, the first thing is to check that it's not nullptr, it's omitted here. So we should do VariantInit(&retValueArg);, despite it's not stated ​http://msdn.microsoft.com/en-us/library/windows/desktop/ms221673(v=vs.85).aspx .
Now we can safely exit from the function at any point, the discussion of functions with multiple exist points is beyond of this scope.
When we are ready to set the return value, we should do the following

ATL::CComVariant retValue;
...
return retValue.Detach(&retValueArg);

CComVariant::Detach ​http://msdn.microsoft.com/en-us/library/b4ek69s3.aspx

But we don't have to care about it by avoiding of manual implementation of Invoke method.

If somebody believes that we can write good code, I have a bad news. Right now there are some bugs and the implementation does not cover a lot of edge cases. Also everybody can check the ratio (the number of code reviews reviewed by me where typos and mistakes are found)/(all reviewed by my) before committing, it's about one. The another indicator is that sometimes I find mistakes in the code which is already committed and due to some reason I was not a reviewer there. The latter shows that even code review does not help. So if it's possible I would like to get as much help as we can from the compiler, the proper tools and the good techniques. I'm pretty sure we should not increase the amount of the work and we should decrease the ability to make mistakes.

Now some questions:

There's no need to add more IDL than we already use, and indeed, what we already use could be eliminated.

Is there a goal to get rid of idl? It helps a lot, why do we need to get rid of it?

Already the type library being generated isn't being used.

We can use it, it's not big a problem. Even more, we don't have to write the code which works with it, it's done in ATL::IDispatchImpl. It's only additional work. Just a couple of days before I've seen how we added a special code in tests as a workaround to hide the mistake in our impl ​http://codereview.adblockplus.org/6267413888761856/ . Using ATL there will not such mistakes and we will not need such tests, which also contained mistakes.

The details are down in how IE binds to a BHO, but in short, it doesn't even look at the type library.

I don't doubt because ATL::IDispatchImpl is using it. I'm primarily talking about our own settings interface. ATL facilities to work with DWebBrowserEvents2 are not so easy, but it's not related to our idl-file and to our typelib. To reduce the potential mistakes with DWebBrowserEvents2 I need to check the details.

Nor does v8 need a type library;

It's not related to the topic, why is it here?

Nor does v8 need a type library; indeed from my experience in the debugger, it doesn't even seem to be caching Dispatch ID's.

Which part is implied here? We don't call too much of our methods from IE and we don't receive a lot of events from IE, the caching here is not important in my opinion. If it hurts the performance we definitely should fix that particular case and it's not certainly should be a problem of caching, may be we can fix it by design.

although your code example doesn't match how a COM server returns a result value (as opposed to its status return value).
Note: The existing ATL classes do _not_ do the right thing in all cases. They have their own lifecycle requirements that sometimes overlap with and sometime conflict with the requirements of COM parameters. And the ATL classes absolutely do not perform conversions to standard types well.

Could you clarify it?

[Jordanbelfort]

spam

[Juddy23]

spam