Audit and fix leaks of Mach host and thread ports |
||||
Issue descriptionIn investigating bug 878071, I read https://robert.sesek.com/2012/1/debugging_mach_ports.html about a very early Mach port leak ( bug 105513 ) caused by calls to mach_thread_self(). I did a search for it and found: https://cs.chromium.org/search/?q=mach_thread_self: - https://cs.chromium.org/chromium/src/third_party/webrtc/rtc_base/cpu_time.cc?rcl=f18b35284288ac851b77db88df3e2e8d2273db97&l=86 Robert did a search for related calls and found: https://cs.chromium.org/search/?q=mach_host_self - https://cs.chromium.org/chromium/src/third_party/swiftshader/third_party/llvm-subzero/lib/Support/Host.cpp?rcl=c47cd435bf6f631aedffa820867350c40e602c41&l=956 - https://cs.chromium.org/chromium/src/third_party/swiftshader/third_party/LLVM/lib/Target/PowerPC/PPCSubtarget.cpp?rcl=c47cd435bf6f631aedffa820867350c40e602c41&l=39 (I know, PowerPC, but while we're in there to fix our thing.) Dunno if this is enough to account for the ~50 port per day leak in bug 878071, but this is obvious low-hanging fruit.
,
Aug 30
The one in webrtc looks suspicous to me because it's in GetThreadCpuTimeNanos(), which looks like it may be called every frame(?!).
,
Aug 30
That can't possibly be happening, otherwise we'd be blowing up our port usage, right?
,
Aug 30
,
Aug 31
I just noticed that the swiftshader Host.cpp one is guarded by a defined(__ppc__) - so that's okay (I've already got a CL out to fix it in LLVM though).
,
Aug 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/07d0a85ba6fffb6c527c3473ad2474274327e062 commit 07d0a85ba6fffb6c527c3473ad2474274327e062 Author: Robert Sesek <rsesek@chromium.org> Date: Fri Aug 31 14:24:32 2018 Use ScopedMachSendRight in media::NumberOfPhysicalProcessors(). This is not leaking, but it is easier to audit for leaks when using the scoper. Bug: 879307 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I3d0fa36922feb1a4c1ffef293b97d030c8793e14 Reviewed-on: https://chromium-review.googlesource.com/1196967 Reviewed-by: Olga Sharonova <olka@chromium.org> Commit-Queue: Olga Sharonova <olka@chromium.org> Cr-Commit-Position: refs/heads/master@{#588015} [modify] https://crrev.com/07d0a85ba6fffb6c527c3473ad2474274327e062/media/audio/mac/audio_low_latency_input_mac.cc
,
Sep 1
Re 2: The function is called only in the standalone performance tests and isn't included in the chrome. This actually may explain why we sometimes have random failures for mac performance test bots. Thanks a lot for catching this.
,
Sep 1
,
Sep 1
Re comment 8, that makes sense. That function leaks ports so aggressively that it would have surely been caught had it shipped with Chrome. Which means that bug 878071 is caused by other issues...
,
Sep 4
https://reviews.llvm.org/D51517 for LLVM, which may eventually get rolled into swiftshader.
,
Sep 4
The following revision refers to this bug: https://webrtc.googlesource.com/src.git/+/b0800519b0dce9852ad77176a163cbd16e50a2c1 commit b0800519b0dce9852ad77176a163cbd16e50a2c1 Author: Robert Sesek <rsesek@chromium.org> Date: Tue Sep 04 13:43:24 2018 Do not leak the Mach thread port in GetThreadCpuTimeNanos(). Bug: chromium:879307 Change-Id: Ia6b5b3ea4684354d8a21dc85e43f67166832cc19 Reviewed-on: https://webrtc-review.googlesource.com/96980 Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org> Reviewed-by: Karl Wiberg <kwiberg@webrtc.org> Commit-Queue: Robert Sesek <rsesek@chromium.org> Cr-Commit-Position: refs/heads/master@{#24554} [modify] https://crrev.com/b0800519b0dce9852ad77176a163cbd16e50a2c1/rtc_base/cpu_time.cc
,
Sep 5
The above CL in #11 has rolled into Chromium but Bugdroid is down for maintenance so it hasn't posted. The LLVM CL hasn't been downstreamed into Switfshader yet, but since it's PPC-only, I don't see an urgent need to do so.
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5bce785d4f1bbb8a681499b39accdec5f94e8403 commit 5bce785d4f1bbb8a681499b39accdec5f94e8403 Author: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Date: Tue Sep 04 18:25:27 2018 Roll src/third_party/webrtc 46d65e2c2cc0..b0800519b0dc (1 commits) https://webrtc.googlesource.com/src.git/+log/46d65e2c2cc0..b0800519b0dc git log 46d65e2c2cc0..b0800519b0dc --date=short --no-merges --format='%ad %ae %s' 2018-09-04 rsesek@chromium.org Do not leak the Mach thread port in GetThreadCpuTimeNanos(). Created with: gclient setdep -r src/third_party/webrtc@b0800519b0dc The AutoRoll server is located here: https://autoroll.skia.org/r/webrtc-chromium-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary. CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux_chromium_archive_rel_ng;luci.chromium.try:mac_chromium_archive_rel_ng BUG= chromium:879307 TBR=webrtc-chromium-sheriffs-robots@google.com Change-Id: If9e374fbae31d1b49bf47a89c41e5815ca475afb Reviewed-on: https://chromium-review.googlesource.com/1204610 Reviewed-by: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Commit-Queue: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#588590} [modify] https://crrev.com/5bce785d4f1bbb8a681499b39accdec5f94e8403/DEPS |
||||
►
Sign in to add a comment |
||||
Comment 1 by a...@chromium.org
, Aug 30