Skip to content

Python3: implement new way of converting COMError to CallCancelled#9795

Merged
michaelDCurran merged 2 commits into
threshold_py3_stagingfrom
i9763
Jun 24, 2019
Merged

Python3: implement new way of converting COMError to CallCancelled#9795
michaelDCurran merged 2 commits into
threshold_py3_stagingfrom
i9763

Conversation

@michaelDCurran
Copy link
Copy Markdown
Member

Link to issue number:

Fixes #9763

Summary of the issue:

When running NVDA from the threshold_py3_staging branch, it fails to start as watchdog cannot replace _ctypes.COMError's constructor with its own custom version in order to raise CallCanceled.

Description of how this pull request fixes the issue:

Rather than raising CallCancelled in COMError's constructor (which is no longer possible):
Wrap the low-level ctypes function call for all comtypes methods and properties, and catch COMError. If the COMError is due to a cancelled COM call, raise CallCancelled. otherwise keep raising the original COMError.

We do this in the following way:
For the duration of importing comtypes for the very first time, we replace ctypes.WINFUNCTTYPE with our own custom version.
As comtypes imports WINFUNCTYPE specifically by name, comtypes will keep our custom version, even after we replace the original after comtypes has been imported.
Comtypes uses our custom version of WINFUNCTYPE in order to generate its ctypes function caller objects, in comtypes._cominterface_meta._make_methods.
Our custom version of WINFUNCTYPE calls the original WINFUNCTYPE, but returns a custom subclass of the resulting function prototype CFuncPtr class.
This custom subclass overrides __call__ and wraps the super call in a try block, catching COMError and raising CallCancelled if the COMError is for a cancelled COM call, otherwise continuing to raise the original COMError.

In order to do this, a couple things had to be moved around:

  • CallCancelled had to be moved from watchdog to the top of core.
  • comtypesMonkeyPatches (where we do the replacing of WINFUNCTYPE) had to be moved to the top of core, before -- before it was slightly lower down after the fixing of the comtyps.gen interface cache directory.

Finally, one extra change was made to baseObject.AutoPropertyObject's property cache getter code:
Previously, when fetching a property, we would try directly fetching it from the property cache, and if it raised KeyError, in the except block we would then actually fetch the property for real.
Although this code still worked in Python3, it produced a rather strange traceback if an exception (E.g. CallCancelled) was raised from inside the property itself. In short Python3 would log the KeyError traceback, and then underneath state that while handling this exception, another exception occured, which was the actual exception in the property.
To make this more readable, the code was slightly restructured to just set a 'missing' variable to True in the except block for KeyError, and then completely outside of the try except blocks, fetch the real property if missing is True. Therefore, if the real property raises an exception, it won't be seen as part of the KeyError exception, which was very missleading.

Testing performed:

  • Used NVDA for a while, ensuring that COMErrors were still being caught in general by all our NVDAObject properties etc. This was confirmed by looking at the log and not noticing any strange COMError tracebacks. Also the debugWarning about accRole failing (like it seems to do a lot in Windows) still showed up, which is logged when catching COMError.
  • Using the following test case
    in Internet Explorer, ensured that NVDA was able to cancel COM calls, and that CallCancelled was being raised and correctly logged, by placing focus on the "message" button in the test case, opening the Python console, and calling:
nav.HTMLNode.click()

Which is known to freeze NVDA's main thread (as that button shows a javascript alert). Then alt+tabbin away from the Python console, which causes NVDA's watchdog to kick in and try cancelling the call.
Once NVDA recovered, looking in the Python Console output, confirmed that the correct CallCancelled traceback was printed.

Known issues with pull request:

None.

Change log entry:

None.

…r to CallCancelled by hooking WinFunctionType's __call__ in the context of comtypes.
Copy link
Copy Markdown
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only two things regarding typos in comments.
Also, is all the stuff that's now in comtypesMonkeyPatches still valid, particularly the VARIANT.value.fset monkeypatch? May be it has been implemented in comtypes after we wrote this patch, and it has never been removed?

Comment thread source/comtypesMonkeyPatches.py Outdated
import _ctypes

# A version of ctypes.WINFUNCTYPE
# that produces a WinFunctionType class whos instance will convert COMError into a CallCancelled exception when called as a function.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no native English speaker, but I'm pretty sure whos should be whose.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have confirmed we still need all fixes for comtypes, except for the one to support VT_BYREF. However, there code is slightly different to hours, so I am reluctant to remove ours, especially in this pr, without first finding a good example to test with.
From memory the only thing we ever called that took values byref was MS Word's Window.getPoint.

Comment thread source/comtypesMonkeyPatches.py Outdated
def new_WINFUNCTYPE(restype,*argtypes,**kwargs):
cls=old_WINFUNCTYPE(restype,*argtypes,**kwargs)
class WinFunctionType(cls):
# We must manually pull the manditory class variables from the super class,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manditory > mandatory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants