Python 3: Fix error with accPropServer - #9900
Conversation
The VARIANT out-param had not been initialised correct. Comtypes was not initialising VARIANT.vt to VT_EMPTY, and attempting to call VariantClear (oleauto.h) before assigning it the value returned from getProp. Instead, we implement a low-level (require a this param) getProp method, which is then provided pointers to the out-params, and can initialise them correctly.
|
I'll attach a try build link shortly. |
|
Please use the following try build to test for issues. |
feerrenrut
changed the title
LeonarddeR
left a comment
LeonarddeR
left a comment
There was a problem hiding this comment.
Just some small things. Having said that, I was pretty sure that I had a similar low level implementation, though yet that implementation didn't work.
This comment has been minimized.
This comment has been minimized.
I did't actually look at your implementation. The key things that took me time to realise:
|
- Encompass getProp in a try block, ensure all exceptions are caught and logged. - Tidy type hints - Use clearer method to set pointer values - Remove redefined S_OK
| except Exception: | ||
| # Preset values for "no prop value", in case we return early. | ||
| pfGotProp.contents.value = self.DOES_NOT_HAVE_PROP | ||
| pvarValue.contents.vt = VT_EMPTY |
There was a problem hiding this comment.
Here you're setting contents.vt and below you're setting contents.value. Why is that?
If you prefer subscripting with [0], I can certainly live with that. Just not sure what's the preferred way of doing this.
There was a problem hiding this comment.
.vt must be set, to ensure that comtypes does not attempt to clear an uninitialised VARIANT , or more accurately, it's safe when it does clear it. When you assign a value, comtypes automatically converts the value being assigned, but first it clears the VARIANT. Out params don't seem to have a guarantee of being initialised. So .vt which, I assume stands for variant type, could be anything. It's explicitly unsafe to clear an uninitialised variant, see variantClear Docs for the following snippet:
Do not use VariantClear on unitialized variants; use VariantInit to initialize a new VARIANTARG or VARIANT.
Rather than setting .vt we could call VariantInit
I suspect this was causing the issue I was having when trying to override name a while back.
I don't really have a preference for getting to the pointer contents, I'll leave it as is.
There was a problem hiding this comment.
I'm leaning towards using VariantInit rather than setting vt, but the server annotation sample does it like this.
There was a problem hiding this comment.
This stackoverflow answer convinces me to use it.
| pfGotProp.contents.value = self.HAS_PROP | ||
| pvarValue.contents.value = ret[0] | ||
| except Exception as e: # catch and log all exceptions so they are not swallowed by caller. | ||
| log.exception() |
There was a problem hiding this comment.
I think I still prefer returning something else than S_OK here. May be S_FALSE is a good one here, or comtypes.hresult.E_FAIL? Not sure how hresult codes work.
There was a problem hiding this comment.
I'm not so sure, I don't think we should be reporting an error unless it is the callers fault. If we would succeed with another value, then this essentially translates to a "look-up error", that outcome is handled by the pfGotProp outparam.
|
@feerrenrut: related to this is the list of monkey patches we apply to comtypes. From code inspection/investigation, it turns out that at least the patch to support byref in variants can be removed. |
4 participants
Link to issue number:
#9768
Summary of the issue:
The
VARIANTout-param had not been initialised correct.Comtypes was not initialising
VARIANT.vttoVT_EMPTY, and attempting to callVariantClear(oleauto.h) before assigning it the value returned fromgetProp.Description of how this pull request fixes the issue:
Instead, we implement a low-level (require a
thisparam)getPropmethod, which is then provided pointers to the out-params, and can initialise them correctly.Testing performed:
Run locally from source.
Known issues with pull request:
None
Change log entry:
None