Closed Bug 1335235 Opened 3 years ago Closed 3 years ago

Disable BSTR cache in mscom::MainThreadRuntime

Categories

(Core :: IPC, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

Details

Attachments

(1 file)

Cached BSTRs are never evicted, so this is essentially a memory leak built into COM.
(Oh, and the reason why we don't need GetProcAddress here is because although SetOaNoCache is not present in the SDK headers, its symbols *are* exported by oleaut32.lib)
Comment on attachment 8831870 [details]
Bug 1335235: Add disabling of the COM BSTR cache to mscom::MainThreadRuntime;

https://reviewboard.mozilla.org/r/108368/#review109592

Looks like a good idea. I wonder if this will impact general app performance in some way though?

https://blogs.msdn.microsoft.com/oldnewthing/20150107-00/?p=43203/
Attachment #8831870 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #3)
> Comment on attachment 8831870 [details]
> Looks like a good idea. I wonder if this will impact general app performance
> in some way though?
> 
> https://blogs.msdn.microsoft.com/oldnewthing/20150107-00/?p=43203/

Worth keeping an eye on at least.

Given the nature of the way BSTRs are passed out by a11y APIs (many of these strings are unique to particular content, so depending on usage patterns they might just keep accumulating in the cache without ever being reused), I'm certain that we need this off for that case.
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d675af74f99c
Add disabling of the COM BSTR cache to mscom::MainThreadRuntime; r=jimm
https://hg.mozilla.org/mozilla-central/rev/d675af74f99c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Aaron, did you want to get this uplifted to 53?
Flags: needinfo?(aklotz)
Comment on attachment 8831870 [details]
Bug 1335235: Add disabling of the COM BSTR cache to mscom::MainThreadRuntime;

The patch is simple enough and this isn't specific to e10s, so sure, I'll request it.

Approval Request Comment
[Feature/Bug causing the regression]: Microsoft COM Automation
[User impact if declined]: Potential memory leaks under certain conditions (particularly a11y)
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: None
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Really simple patch
[String changes made/needed]: None
Flags: needinfo?(aklotz)
Attachment #8831870 - Flags: approval-mozilla-beta?
Comment on attachment 8831870 [details]
Bug 1335235: Add disabling of the COM BSTR cache to mscom::MainThreadRuntime;

Fixing a memory leak sounds good to me. This should land for beta 2.
Attachment #8831870 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Setting qe-verify- based on Aaron's assessment on manual testing needs (see Comment 8) and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.