Closed Bug 1418448 Opened 2 years ago Closed 2 years ago

Clean up BSTRs for DynamicIA2Data

Categories

(Core :: Disability Access APIs, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: aklotz, Assigned: Jamie)

References

Details

(Whiteboard: aes+)

Attachments

(3 files)

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.
(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).
(See comment 1.)
Flags: needinfo?(aklotz)
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.
Assignee: nobody → jteh
Flags: needinfo?(aklotz)
Summary: Separate ClearDynamicIA2Data into separate functions for initialization vs cleanup → Clean up BSTRs for DynamicIA2Data
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.
Attachment #8931573 - Flags: review?(aklotz) → review+
Comment on attachment 8931572 [details]
Bug 1418448 part 2: AccessibleHandler: Clean up DynamicIA2Data.

https://reviewboard.mozilla.org/r/202698/#review209130
Attachment #8931572 - Flags: review?(aklotz) → review+
Comment on attachment 8931571 [details]
Bug 1418448 part 1: Accessible HandlerProvider: Clean up DynamicIA2Data correctly.

https://reviewboard.mozilla.org/r/202696/#review209132
Attachment #8931571 - Flags: review?(aklotz) → review+
Pushed by jteh@mozilla.com:
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 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.
Attachment #8931571 - Flags: approval-mozilla-beta?
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.
Attachment #8931572 - Flags: approval-mozilla-beta?
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 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+.
Attachment #8931571 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8931572 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.