1335235 - Disable BSTR cache in mscom::MainThreadRuntime

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Cached BSTRs are never evicted, so this is essentially a memory leak built into COM.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1335235: Add disabling of the COM BSTR cache to mscom::MainThreadRuntime;

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

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

(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 3

[Jim Mathies [:jimm]]

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/

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

(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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d675af74f99c
Add disabling of the COM BSTR cache to mscom::MainThreadRuntime; r=jimm

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d675af74f99c

Comment 7

[Julien Cristau [:jcristau]]

Aaron, did you want to get this uplifted to 53?

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 9

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c9a2f2cda119

Comment 11

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Aaron's assessment on manual testing needs (see Comment 8) and the fact that this fix has automated coverage.