Issue metadata
Sign in to add a comment
|
macOS: Leak of swapped memory. |
||||||||||||||||||||
Issue descriptionFiling this bug on behalf of rohitrao@. We did some debugging together. Symptoms: * Chrome is using ~8GB of swapped memory. * And there are black square boxes appearing on the UI: https://bugs.chromium.org/p/chromium/issues/detail?id=870317#c37 Observations: * Rohit took a vmmap of every chrome process [see Archive.zip]. * First, I asked him to kill the GPU process [which restarts itself]. This caused no significant change to swapped memory. * Then, I asked him to kill process 62644 [gmail]. This has [615MB virtual pages resident, 425MB dirty, 175MB swap]. This caused swapped memory to drop by 1.2GB! * I asked Rohit to kill all non-browser processes. Some of these [extensions, gpu, etc.] restart themselves. See before_quit.zip for the before-vmmap for all of these processes. Then I asked Rohit to kill the browser process. According to vmmap, total swap across all processes [basically all in the browser] is 125MB. Killing the browser [and all associated processes] released ~2.1 GB swap!!. This suggests that something is holding memory on behalf of a process [kernel/drivers], which is why the memory is not showing up in vmmap. Given the high correlation with black UI boxes, I'm going to guess that we're somehow misusing/leaking texture-related memory. There are fixes landing for this: https://bugs.chromium.org/p/chromium/issues/detail?id=870317 The other possibility would be something analogous to wired memory leak: https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=292295 But that shouldn't affect the browser process.
,
Aug 14
Tentatively tagging as M69/M70 stable blocker for tracking purpose. Pls remove labels after investigation if needed.
,
Aug 15
I suspect this is fixed by: https://chromium-review.googlesource.com/c/chromium/src/+/1175918
,
Aug 15
To clarify: we were leaking unmapped shared memory handles. The contents would therefore be leaked, but not present in the vmmap.
,
Aug 15
,
Aug 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6544b8a5f6cd7a060f1293d9683b0d8f88ba1cd6 commit 6544b8a5f6cd7a060f1293d9683b0d8f88ba1cd6 Author: Alexandr Ilin <alexilin@chromium.org> Date: Wed Aug 15 23:08:54 2018 base: Fix shared memory handle leak in IPC serialization PlatformSharedMemoryRegion leaked handles after serializing into the IPC message on Windows, Mac and Fuchsia. Serialization code incorrectly assumed that HandleWin, MachPortMac and HandleFuchsia classes take ownership over the handle. What really happens is that HandleFoo is passed to WriteParam(), which causes ParamTraits<HandleFoo>::Write() to be invoked, before it returns, creating the HandleAttachmentFoo, which will duplicate the handle. By the time we return from WriteParam() the handle should have been duplicated into the serialized message, so we can safely let the PSMR close the handle when it goes out of scope. This CL makes the PlatformSharedMemoryRegion serialization repeat the logic of the base::SharedMemoryHandle serialization as if the OwnershipPassesToIPC() flag is always set to true. Bug: 873313 , 873668 Change-Id: I4e98ff9483a48970ebcb2fcf6b150f447af82794 Reviewed-on: https://chromium-review.googlesource.com/1175918 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Wez <wez@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#583434} [modify] https://crrev.com/6544b8a5f6cd7a060f1293d9683b0d8f88ba1cd6/ipc/ipc_message_utils.cc
,
Aug 16
Pls update bug with canary result tomorrow and request a merge to M69 if necessary. Thank you.
,
Aug 16
The NextAction date has arrived: 2018-08-16
,
Aug 16
There won't be a canary result because we don't have any kind of reliable repro for this or any way to detect it inside Chrome :( Untagging RBS.
,
Aug 16
Thank you Elly. Per comment #9, this is not "RBS". So this doesn't need a merge to M69, correct?
,
Aug 16
This absolutely needs a merge to M-69. The magnitude of the leak is terrifying.
,
Aug 16
Thank you erikchen@. Cl listed at #6 didn't make it to canary yet. Per comment #9, there won't be a canary result but would it be ok to wait for canary baking just to make sure no other regression?
,
Aug 16
Yup -- we should get this merged before M-69 hits stable, but there's no rush right now.
,
Aug 16
Ok, lets wait until Monday morning.
,
Aug 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9ba9f73180a591eba65b70fa708bdac3786cead9 commit 9ba9f73180a591eba65b70fa708bdac3786cead9 Author: Alexandr Ilin <alexilin@chromium.org> Date: Thu Aug 16 17:35:10 2018 base: Fix shared memory handle leak in IPC serialization PlatformSharedMemoryRegion leaked handles after serializing into the IPC message on Windows, Mac and Fuchsia. Serialization code incorrectly assumed that HandleWin, MachPortMac and HandleFuchsia classes take ownership over the handle. What really happens is that HandleFoo is passed to WriteParam(), which causes ParamTraits<HandleFoo>::Write() to be invoked, before it returns, creating the HandleAttachmentFoo, which will duplicate the handle. By the time we return from WriteParam() the handle should have been duplicated into the serialized message, so we can safely let the PSMR close the handle when it goes out of scope. This CL makes the PlatformSharedMemoryRegion serialization repeat the logic of the base::SharedMemoryHandle serialization as if the OwnershipPassesToIPC() flag is always set to true. TBR: abdulsyed Bug: 873313 , 873668 Change-Id: I4e98ff9483a48970ebcb2fcf6b150f447af82794 Reviewed-on: https://chromium-review.googlesource.com/1175918 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Wez <wez@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#583434}(cherry picked from commit 6544b8a5f6cd7a060f1293d9683b0d8f88ba1cd6) Reviewed-on: https://chromium-review.googlesource.com/1178261 Cr-Commit-Position: refs/branch-heads/3524@{#5} Cr-Branched-From: 4c984bc7664755d701c75b49b795678e164551e3-refs/heads/master@{#583420} [modify] https://crrev.com/9ba9f73180a591eba65b70fa708bdac3786cead9/ipc/ipc_message_utils.cc
,
Aug 16
+ abdulsyed@ retriggered Windows and Mac canary #70.0.3524.3 which includes merge listed at #15. Per offline chat with wez@, wait until Monday for canary baking. If change looks good and approved, merge by Monday 4:00 PM PT so we can take it in for next week M69 beta release on Wednesday.
,
Aug 16
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 20
The NextAction date has arrived: 2018-08-20
,
Aug 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/579a09af56a7648c5380bc126e0c42c45cfa6cfd commit 579a09af56a7648c5380bc126e0c42c45cfa6cfd Author: Alexandr Ilin <alexilin@chromium.org> Date: Mon Aug 20 17:53:48 2018 base: Fix shared memory handle leak in IPC serialization PlatformSharedMemoryRegion leaked handles after serializing into the IPC message on Windows, Mac and Fuchsia. Serialization code incorrectly assumed that HandleWin, MachPortMac and HandleFuchsia classes take ownership over the handle. What really happens is that HandleFoo is passed to WriteParam(), which causes ParamTraits<HandleFoo>::Write() to be invoked, before it returns, creating the HandleAttachmentFoo, which will duplicate the handle. By the time we return from WriteParam() the handle should have been duplicated into the serialized message, so we can safely let the PSMR close the handle when it goes out of scope. This CL makes the PlatformSharedMemoryRegion serialization repeat the logic of the base::SharedMemoryHandle serialization as if the OwnershipPassesToIPC() flag is always set to true. TBR: govind Bug: 873313 , 873668 Change-Id: I4e98ff9483a48970ebcb2fcf6b150f447af82794 Reviewed-on: https://chromium-review.googlesource.com/1175918 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Wez <wez@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#583434}(cherry picked from commit 6544b8a5f6cd7a060f1293d9683b0d8f88ba1cd6) Reviewed-on: https://chromium-review.googlesource.com/1181701 Cr-Commit-Position: refs/branch-heads/3497@{#717} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/579a09af56a7648c5380bc126e0c42c45cfa6cfd/ipc/ipc_message_utils.cc
,
Aug 20
This M69 merge was approved here - https://bugs.chromium.org/p/chromium/issues/detail?id=873313#c41. Pls mark bug as fixed if nothing else is pending.
,
Aug 20
alexilin@'s patch definitely resolved a major leak of shared-memory handles, especially GPU (which had recently been migrated to the new containers), so closing this out as Fixed. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by ellyjo...@chromium.org
, Aug 13Owner: erikc...@chromium.org
Status: Assigned (was: Untriaged)