Memory leak in PPBNaClPrivate::LaunchSelLdr |
||||||||||||||
Issue descriptionThe NoOpListener instance passed to IPC::SyncChannel::Create is leaked. Found by LSan in https://chromium-swarm.appspot.com/task?id=3a1549e5ee84bf10&refresh=10&show_raw=1: Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x97f432 in operator new(unsigned long) /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cc:92:3 #1 0x22213f34 in nacl::PPBNaClPrivate::LaunchSelLdr(int, PP_Bool, char const*, PP_NaClFileInfo const*, PP_Bool, PP_NaClAppProcessType, std::__1::unique_ptr<IPC::SyncChannel, std::__1::default_delete<IPC::SyncChannel> >*, PP_CompletionCallback) components/nacl/renderer/ppb_nacl_private_impl.cc:527:11 #2 0x1fdb0919 in plugin::ServiceRuntime::StartSelLdr(plugin::SelLdrStartParams const&, pp::CompletionCallback) components/nacl/renderer/plugin/service_runtime.cc:34:3 #3 0x1fda0755 in plugin::Plugin::LoadHelperNaClModule(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, PP_NaClFileInfo, plugin::NaClSubprocess*, pp::CompletionCallback) components/nacl/renderer/plugin/plugin.cc:76:20 #4 0x1fda7035 in plugin::PnaclCoordinator::LoadLinker(int) components/nacl/renderer/plugin/pnacl_coordinator.cc:421:12 ...
,
May 8 2018
,
May 8 2018
,
May 11 2018
Component extension dmboannefpncccogfdikhmhpmdnddgoe: the 'ZipArchiver', is the Files.app component extension that is loading Nacl during browser tests. Next, exactly 0 of the Gallery/AudioPlayer/VideoPlayer/FileManager/BrowserTests use dmboannefpncccogfdikhmhpmdnddgoe during Files.app browser tests. So, if we can prevent that extension loading during Files.app browser tests, we can skirt around this bug and also make the tests a little-moar-faster-like, as a nice side-effect.
,
May 11 2018
+sammc: leak is in components/nacl/renderer/ppb_nacl_private_impl.cc function void PPBNaClPrivate::LaunchSelLdr() https://cs.chromium.org/chromium/src/components/nacl/renderer/ppb_nacl_private_impl.cc?type=cs&q=ppb_nacl_private_impl.cc+%22new+NoOpListener%22+&sq=package:chromium&l=528 The 'new NoOpListener' gets stashed into an IPC::SyncChannel, using its Create call, and passed on thru layer-cakes of IPC code, to an IPC::ChannelProxy listener_ member. IPC::ChannelProxy does not own the listener_, it only NULL's it on Clear() https://cs.chromium.org/chromium/src/ipc/ipc_channel_proxy.cc?type=cs&q=IPC::ChannelProxy+Clear&sq=package:chromium&l=219 And ipc_channel_proxy.h has no comment about who owns the listener_, but it looks like it sure does not, per this leak report. https://cs.chromium.org/chromium/src/ipc/ipc_channel_proxy.h?type=cs&sq=package:chromium&l=337
,
May 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2108740c0ae74d1d22fbe74df3393217a1dced9f commit 2108740c0ae74d1d22fbe74df3393217a1dced9f Author: Noel Gordon <noel@chromium.org> Date: Fri May 11 09:59:42 2018 FileManageBrowserTestBase: disable NaCl loading component extensions These extensions are not used by the Files.app browser tests and cause ASAN flakes due to an unrelated NaCl bug 788671 . Skirt around that bug here by not loading these NaCl-using extensions. Side-effect: these browser tests hopefully run a little faster. Bug: 788671 ,836254 Change-Id: Ie528cad328d76f2ff0ee97c994f2bc539de86a98 Reviewed-on: https://chromium-review.googlesource.com/1055351 Commit-Queue: Noel Gordon <noel@chromium.org> Reviewed-by: Naoki Fukino <fukino@chromium.org> Cr-Commit-Position: refs/heads/master@{#557831} [modify] https://crrev.com/2108740c0ae74d1d22fbe74df3393217a1dced9f/chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc
,
May 11 2018
"Side-effect: these browser tests hopefully run a little faster". Initial results for the Chromium browser_test DEBUG bots, crrev.com/557831 shaved 10 to 15 seconds off each test. That's like 33% faster.
,
May 14 2018
,
May 14 2018
,
May 14 2018
Re #8, pictures of FileManagerBrowserTest flakiness disappearing to the right now there is more speed in the tests due to #7.
,
May 14 2018
Great work Noel!
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e0c0304e045a5012a35e2af925c8dd11d0e6a8ba commit e0c0304e045a5012a35e2af925c8dd11d0e6a8ba Author: Sam McNally <sammc@chromium.org> Date: Tue May 15 00:53:01 2018 Remove the NoOpListener created in PPBNaClPrivate::LaunchSelLdr(). IPC::SyncChannel is happy without a Listener so pass nullptr instead of leaking a NoOpListener. Bug: 788671 Change-Id: I492b52ed6b1a7a1c19b31df1fa1754c841d09e92 Reviewed-on: https://chromium-review.googlesource.com/1056751 Reviewed-by: Bill Budge <bbudge@chromium.org> Commit-Queue: Sam McNally <sammc@chromium.org> Cr-Commit-Position: refs/heads/master@{#558559} [modify] https://crrev.com/e0c0304e045a5012a35e2af925c8dd11d0e6a8ba/components/nacl/renderer/ppb_nacl_private_impl.cc
,
May 15 2018
,
Jul 4
Nice stuff Sam. |
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by noel@chromium.org
, May 8 2018