1440564 - StructToStream leaks memory

Status: RESOLVED FIXED

(Core :: IPC: MSCOM, defect)

Description

[James Teh [:Jamie]]

STR:
1. Ensure you have the fix for the bug 1440257 leak applied. Ensure the accessible handler is enabled.
2. Load World War I.
3. Check heap unclassified for that content process in about:memory.
4. Return to the World War I tab and press NVDA+f5 to refresh the buffer.
5. Check heap unclassified for that content process in about:memory.
Expected: No noticeable increase.
Actual: Memory usage goes up by 20 mb or so.

I've tracked this down to RPC memory allocation in StructToStream. If I put printfs in midl_user_allocate and midl_user_free in StructStream.cpp, I only ever see allocations. However, with another printf, I proved that MesHandleFree is getting called, so the StructToStream is being cleaned up correctly.

I notice this little nugget of agony in the documentation for RpcSmFree:

> To improve performance, the RpcSmFree function only marks memory for release. Memory is not actually released until your application calls the RpcSmDisableAllocate function. To release memory immediately, invoke the midl_user_free function.
( https://msdn.microsoft.com/en-us/library/windows/desktop/aa378463(v=vs.85).aspx )

I'm thinking the RPC serialisation actually uses this RpcSm allocation framework and so this applies. However, calling RpcSmDisableAllocate doesn't seem to free the memory. RpcSmDisableAllocate notes that the enable/disable functions need to be called in pairs, so I tried calling RpcSmEnableAllocate before MesEncodeDynBufferHandleCreate and RpcSmDisableAllocate after MesEncodeDynBufferHandleCreate. No joy.

The documentation for these functions indicates that allocation is normally enabled by the server stub. However, in this case, I'm not sure how this works, since this isn't actually an RPC stub. Unfortunately, I haven't worked out how to make this behave the way we need it to.

Comment 1

[James Teh [:Jamie]]

> and RpcSmDisableAllocate after MesEncodeDynBufferHandleCreate

Oops. I meant RpcSmDisableAllocate after MesHandleFree.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

I saw this before bed and couldn't help but comment.

Reviewing the docs for MesEncodeDynBufferHandleCreate, I see "This buffer will be allocated by the current client memory-management mechanism."

Is is possible that we just need to call midl_user_free directly on that buffer?

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

I also found this nugget for IBM DCE that contains slightly different text (obviously, since their DCE implementation is different) at https://www-01.ibm.com/software/network/dce/library/publications/appdev/html/APPDEV26.HTM#HDRWQ506

> With the dynamic buffer encoding style, the IDL encoding services build a single buffer containing all the encoded data and deliver the buffer to application code. The buffer is allocated by whatever client memory management mechanism has been put in place by the application code. The default for this is malloc(). When the application code no longer needs the buffer, it should release the memory resource.

Comment 4

[James Teh [:Jamie]]

I thought of calling midl_user_free too. My concern was that because this is a dynamic buffer, the code might actually perform several allocations and so free wouldn't free everything. However, I just realised that it wouldn't use separate allocations, since it needs the buffer to be consecutive, so at most, it might use realloc, in which case free would still work. My other concern is that even though the buffers aren't freed for a long time, perhaps they might get freed at exit or something, thus causing a crash. And finally, there's a chance that there are other data structures associated with the handle that wouldn't get cleaned up. Still, I guess all I can do is try.

Comment 5

[James Teh [:Jamie]]

And this page suggests the RpcS* memory management functions aren't used by default and are obsolete:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa378480(v=vs.85).aspx
That rather kills my original thinking regarding the enable/disable stuff. That said, this random snippet seems to midl_user_free the buffer if it isn't null, which sort of makes sense and fits with comment 2 and comment 3:
https://gist.github.com/mbikovitsky/d051200b3aa6ff41cd01908943d2983f#file-main-c-L95

Comment 6

[James Teh [:Jamie]]

Attachment: Bug 1440564: mscom::StructToStream: Free the buffer when destroying the instance.

You'd think that MesHandleFree would free the buffer, since it was created by RPC, but it doesn't.
Therefore, we must free it ourselves using midl_user_free.

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

Comment 7

[James Teh [:Jamie]]

I did some brief testing. This definitely fixes the memory leak. Heap unclassified was at around 11 mb after 10 or so refreshes of World War I with NVDA! It didn't crash on exit, so presumably, RPC really never does free the memory. I've asked marco to hammer it a bit harder just in case.

Comment 8

[Marco Zehe (:MarcoZ)]

My results are very very good. No crashes, fast reaction, a Firefox build that hardly puts any strain on my machine at all any more. This truly feels like a Quantum experience even a11y-wise now.

The only problem I hit was that all of a sudden, any open Slack tab froze my Firefox. But after I disabled the TamperMonkey script for Slack, those freezes went away. But I am mentioning this here for completeness. Sorry I didn't have time to investigate if those freezes also would have happened in a regular Nightly build, or if they were related to something that Slack changed on their end and it hit me. After all, the script worked fine at first, but after two hours, suddenly things started freezing.

But I'd say it's a safe bet this build, and this particular patch, does a lot of good. That's along with the others that are in Aaron's review queue from jamie. :)

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8953372 [details]
Bug 1440564: mscom::StructToStream: Free the buffer when destroying the instance.

https://reviewboard.mozilla.org/r/222664/#review228716

Gracias

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d222af1686c
mscom::StructToStream: Free the buffer when destroying the instance. r=aklotz

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8d222af1686c

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8953372 [details]
Bug 1440564: mscom::StructToStream: Free the buffer when destroying the instance.

Approval Request Comment
[Feature/Bug causing the regression]: Windows a11y
[User impact if declined]: memory leaks
[Is this code covered by automated tests?]: No
[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]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Very simple patch
[String changes made/needed]: None

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8953372 [details]
Bug 1440564: mscom::StructToStream: Free the buffer when destroying the instance.

Fixing a memory leak sounds good! Let's uplift this for beta 59.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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