Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ia2utils HyperlinkGetter: Switch to CComPtr #9152

Merged
merged 2 commits into from Jan 14, 2019

Conversation

Projects
None yet
3 participants
@jcsteh
Copy link
Contributor

jcsteh commented Jan 14, 2019

Link to issue number:

Addresses part of #9133.

Summary of the issue:

When rendering a document for browse mode in Firefox, failure in retrieving an embedded object causes an uncaught exception, thus crashing Firefox. See this Firefox crash report.

Description of how this pull request fixes the issue:

HyperlinkGetter (in nvdaHelper/common/ia2utils) uses the _com_ptr_t smart pointer for COM objects. _com_ptr_t throws C++ exceptions for COM errors (other than E_NOINTERFACE) when doing a QueryInterface. Normally, there should not be any COM errors when doing a QI to IAccessible2 on an IAccessibleHyperlink for an embedded object in Firefox. However, it has happened (it's the cause of the above crash) and it is theoretically possible; e.g. if the object died after being retrieved but before QI, since a11y calls do not block the content main thread in Firefox.

We had the choice to switch to CComPtr (which guarantees not to throw exceptions) or catch the exceptions. Since we're moving to CComPtr in newer code anyway, I went with the former.

Testing performed:

Tested rendering World War I in Firefox and compared the load times to check they were in the same ballpark. I can't test for the crash, since I can't reproduce it and it is very rare.

Known issues with pull request:

None known.

Change log entry:

Bug fixes:

- Fixed a rare browse mode crash in Firefox.

ia2utils HyperlinkGetter: Switch to CComPtr (instead of _com_ptr_t) s…
…o C++ exceptions don't get thrown for COM errors.

Newer code uses CComPtr already, so it made sense to switch rather than catching the exceptions.

@jcsteh jcsteh requested a review from michaelDCurran Jan 14, 2019

@michaelDCurran michaelDCurran merged commit 2a63710 into nvaccess:master Jan 14, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2019.1 milestone Jan 14, 2019

@jcsteh jcsteh deleted the jcsteh:ia2HyperlinkGetterNoExceptions branch Jan 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.