Closed Bug 1335235 Opened 3 years ago Closed 3 years ago
Disable BSTR cache in mscom::Main
59 bytes, text/x-review-board-request
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/d675af74f99c Add disabling of the COM BSTR cache to mscom::MainThreadRuntime; r=jimm
Aaron, did you want to get this uplifted to 53?
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
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.
You need to log in before you can comment on or make changes to this bug.