1418448 - Clean up BSTRs for DynamicIA2Data

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Jamie asked me some questions in an email about this function (formerly known as ClearIA2Data), and I agree that this is a problem.

I think the reason why it developed this way was that I was using it for both initialization and for cleanup. Really the ZeroMemory implementation should be used for initialization, whereas for cleanup, we should be using SysFreeString, VariantClear, etc.

Setting this to be dependent on bug 1416986 so that we avoid some nasty merge conflicts.

We definitely should uplift this to 58.

Comment 1

[James Teh [:Jamie]]

(In reply to Aaron Klotz [:aklotz] from comment #0)
> Really the ZeroMemory implementation
> should be used for initialization, whereas for cleanup, we should be using
> SysFreeString, VariantClear, etc.

I'm not sure using ZeroMemory is good for initialisation either. As I understand it, the Clear function gets called if the Build function returns due to a failure. However, Build could return after successfully fetching several BSTR properties. Suppose accName succeeded, but accDescription failed. In that case, we still need to SysFreeString the name BSTR.

What is the reasoning behind using ZeroMemory anyway? As I understand it, the struct gets initialised to 0 in GetAndSerializePayload anyway, and if Build fails, we don't serialize it. I guess this isn't true for Refresh (since the caller passes the struct), but we could just refresh into a clean struct and copy (or ZeroMemory the struct before building).

Comment 2

[James Teh [:Jamie]]

(See comment 1.)

Comment 3

[James Teh [:Jamie]]

Discussed this with Aaron. We do need to free the BSTRs in the case where init fails. So rather than splitting this function, we just want to make it smarter (only cleaning up stuff that isn't null).

We also need to do this in AccessibleHandler.

Comment 4

[James Teh [:Jamie]]

Attachment: Bug 1418448 part 1: Accessible HandlerProvider: Clean up DynamicIA2Data correctly.

DynamicIA2Data contains several BSTRs that need to be freed with SysFreeString.
Previously, we just did a ZeroMemory without actually freeing them.
This cleanup code is placed in a header file so it can be used by AccessibleHandler as well.

Review commit: https://reviewboard.mozilla.org/r/202696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202696/

Comment 5

[James Teh [:Jamie]]

Attachment: Bug 1418448 part 2: AccessibleHandler: Clean up DynamicIA2Data.

We need to clean up the VARIANT and BSTRs in DynamicIA2Data in the handler as well.
We do this in two places:
1. Before reading a new payload (because we need to clean up the existing payload); and
2. When we're being destroyed.

Review commit: https://reviewboard.mozilla.org/r/202698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202698/

Comment 6

[James Teh [:Jamie]]

Attachment: Bug 1418448 part 3: Accessible Handler/Provider: Unify release of interfaces in StaticIA2Data.

Both Provider and Handler need to release the interfaces in StaticIA2Data.
Therefore, move this into a common function to avoid duplication pain in future.

Review commit: https://reviewboard.mozilla.org/r/202700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202700/

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8931573 [details]
Bug 1418448 part 3: Accessible Handler/Provider: Unify release of interfaces in StaticIA2Data.

https://reviewboard.mozilla.org/r/202700/#review209128

::: accessible/ipc/win/handler/HandlerDataCleanup.h:19
(Diff revision 1)
>  namespace a11y {
>  
>  inline void
> +ReleaseStaticIA2DataInterfaces(StaticIA2Data& aData)
> +{
> +  // Only interfaces of the IA2 object should be released here, never other

This comment could use some clarification as to what "the IA2 object" is. Something like "the proxied object that this handler wraps" or something?

I leave the wording to you, but to the uninitiated this is probably confusing as-is.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8931572 [details]
Bug 1418448 part 2: AccessibleHandler: Clean up DynamicIA2Data.

https://reviewboard.mozilla.org/r/202698/#review209130

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8931571 [details]
Bug 1418448 part 1: Accessible HandlerProvider: Clean up DynamicIA2Data correctly.

https://reviewboard.mozilla.org/r/202696/#review209132

Comment 10

[James Teh [:Jamie]]

Comment on attachment 8931573 [details]
Bug 1418448 part 3: Accessible Handler/Provider: Unify release of interfaces in StaticIA2Data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202700/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f52f6885fc5
part 1: Accessible HandlerProvider: Clean up DynamicIA2Data correctly. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/57aa3370993f
part 2: AccessibleHandler: Clean up DynamicIA2Data. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/79bf386e2fb3
part 3: Accessible Handler/Provider: Unify release of interfaces in StaticIA2Data. r=aklotz

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/3f52f6885fc5
https://hg.mozilla.org/mozilla-central/rev/57aa3370993f
https://hg.mozilla.org/mozilla-central/rev/79bf386e2fb3

Comment 13

[James Teh [:Jamie]]

Comment on attachment 8931571 [details]
Bug 1418448 part 1: Accessible HandlerProvider: Clean up DynamicIA2Data correctly.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1363595.
[User impact if declined]: Memory leaks when accessibility is used.
[Is this code covered by automated tests?]: No, because we don't have a mechanism for automated testing of platform accessibility code.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Part 2 from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only affects accessibility. Simply frees strings we know were allocated.
[String changes made/needed]: None.

Comment 14

[James Teh [:Jamie]]

Comment on attachment 8931572 [details]
Bug 1418448 part 2: AccessibleHandler: Clean up DynamicIA2Data.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1363595.
[User impact if declined]: Memory leaks when accessibility is used.
[Is this code covered by automated tests?]: No, because we don't have a mechanism for automated testing of platform accessibility code.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Part 1 from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only affects accessibility. Simply frees strings we know were allocated.
[String changes made/needed]: None.

Comment 15

[James Teh [:Jamie]]

Note that although I'm requesting uplift for parts 1 and 2, I'm not requesting it for part 3. Part 3 is only code cleanup (removes duplication) and does not change any behaviour.

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8931571 [details]
Bug 1418448 part 1: Accessible HandlerProvider: Clean up DynamicIA2Data correctly.

Fix a potential memory leak issue when accessibility is used. Beta58+.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/273b7318c36e
https://hg.mozilla.org/releases/mozilla-beta/rev/27bbc48e99c7