Fix LSan on swarming.

testing/test_env.py

Index: testing/test_env.py
diff --git a/testing/test_env.py b/testing/test_env.py
index 2c39508c438d2e9074c10aad29f4c845c380464b..ab311c969967749d0b8817805b283fbb2ed705ff 100755
--- a/testing/test_env.py
+++ b/testing/test_env.py
@@ -5,7 +5,6 @@
 
 """Sets environment variables needed to run a chromium unit test."""
 
-import collections
 import os
 import stat
 import subprocess
@@ -39,7 +38,7 @@ def should_enable_sandbox(cmd, sandbox_path):
   return False
 
 
-def enable_sandbox_if_required(cmd, env, verbose=False):
+def enable_sandbox_if_required(cmd, env, extra_env, verbose=False):
   """Checks enables the sandbox if it is required, otherwise it disables it."""
   chrome_sandbox_path = env.get(CHROME_SANDBOX_ENV, CHROME_SANDBOX_PATH)
 
@@ -47,12 +46,21 @@ def enable_sandbox_if_required(cmd, env, verbose=False):
     if verbose:
       print 'Enabling sandbox. Setting environment variable:'
       print '  %s="%s"' % (CHROME_SANDBOX_ENV, chrome_sandbox_path)
-    env[CHROME_SANDBOX_ENV] = chrome_sandbox_path
+    extra_env[CHROME_SANDBOX_ENV] = chrome_sandbox_path
   else:
     if verbose:
       print 'Disabling sandbox.  Setting environment variable:'
       print '  CHROME_DEVEL_SANDBOX=""'
-    env['CHROME_DEVEL_SANDBOX'] = ''
+    extra_env['CHROME_DEVEL_SANDBOX'] = ''

[earthdok, 2014/10/15 14:11:45]

This is not good. Please pass --no-sandbox, like we do in runtest.py.

Unsetting CHROME_DEVEL_SANDBOX is a deprecated (see http://crbug.com/245376)
equivalent of --disable-setuid-sandbox, which is weaker than --no-sandbox.

(Whether --disable-setuid-sandbox would be sufficient for LSan is an interesting
question though.)

[jam, 2014/10/15 15:23:25]

run_test.py also unsets CHROME_DEVEL_SANDBOX. so it doesn't need to do both?

either way, i'm happy to do this but not in this cl. i'd like to keep it focused
on fixing lsan without changing other stuff like this to reduce the risk that it
breaks something else.

[earthdok, 2014/10/15 17:00:37]

Looking at this conditional in runtest.py, there are two scenarios:
https://code.google.com/p/chromium/codesearch#chromium/tools/build/scripts/sl...

- LSan or TSan is enabled, in which case we append --no-sandbox later on, and
unsetting CHROME_DEVEL_SANDBOX here is superfluous,
- neither LSan or TSan is enabled, but _ShouldEnableSandbox returns false. In
this case we don't append --no-sandbox; we rely exclusively on unsetting the env
variable to disable the setuid sandbox. This configuration is not used by
sanitizers. I don't know which builders (if any) use it.
I do not insist that we get rid of this env variable as part of this CL. Rather,
I propose to append "--no-sandbox" in LSan runs. Not doing so is a regression
from runtest.py, and arguably should be fixed as part of fixing lsan.

However, since your test runs weren't significantly broken by this regression,
I'm ok with doing this in a follow-up CL.

[jam, 2014/10/15 17:23:49]

good point. I added it.

+
+
+def trim_cmd(cmd):
+  # Remove these flags from cmd since they're just used to communicate from the
+  # host machine to this script running on the swarm slaves.
+  internal_commands = ['--asan=0', '--asan=1', '--lsan=0', '--lsan=1']

[M-A Ruel, 2014/10/15 13:08:00]

internal_commands = frozenset(['--asan=0', '--asan=1', '--lsan=0', '--lsan=1'])
return [i for i in cmd if i not in internal_commands]

Then update line 149 to be:
cmd = trim_cmd(cmd)

I prefer this to mutating variables in place, it's much harder to follow.

[jam, 2014/10/15 15:23:25]

Done.

+  for i in internal_commands:
+    if i in cmd:
+      cmd.remove(i)

[jam, 2014/10/15 03:20:48]

Marc-Antoine: I added this method to remove the extra flags that are used to
communicate with test_env.py. I figured there's no reason these internal flags
should be exposed to test binaries, and this makes swarmed tests more similar to
running them locally.

 
 
 def fix_python_path(cmd):
@@ -65,6 +73,57 @@ def fix_python_path(cmd):
   return out
 
 
+def setup_asan(cmd, extra_env, lsan):

[M-A Ruel, 2014/10/15 13:07:59]

I also prefer it to create a extra_env locally and return it, then caller can
env.update(setup_asan(cmd, lsan))

But that doesn't work when the code wants to append something to an existing
variable but that's already the flow you use at line 157, so I'd prefer that.
I'm not a fan of in-out parameters in python and prefer to have out arguments to
be returned.

[jam, 2014/10/15 15:23:25]

Done. also enable_sandbox_if_required

+  # Instruct GTK to use malloc while running ASan or LSan tests.

[M-A Ruel, 2014/10/15 13:07:59]

"""Instructs GTK ... "

[jam, 2014/10/15 15:23:25]

that would be for the function comment right? this comment is for the line
below.

I added a function comment

+
+  # TODO(earthdok): enabling G_SLICE gives these leaks, locally and on swarming
+  #0 0x62c01b in __interceptor_malloc (/tmp/run_tha_testXukBDT/out/Release/browser_tests+0x62c01b)
+  #1 0x7fb64ab64a38 in g_malloc /build/buildd/glib2.0-2.32.4/./glib/gmem.c:159
+
+  #extra_env['G_SLICE'] = 'always-malloc'

[jam, 2014/10/15 03:20:48]

Sergey: I don't know why I get these leaks with G_SLICE, even though they were
passed before swarming. Even running locally I get them.

[earthdok, 2014/10/15 14:11:45]

I'll take a look. It's possible that someone introduced a GTK leak in the last
two weeks.

[earthdok, 2014/10/15 19:57:35]

Could you please link to a test run where they reproduced?

+  extra_env['NSS_DISABLE_ARENA_FREE_LIST'] = '1'
+  extra_env['NSS_DISABLE_UNLOAD'] = '1'
+
+  # TODO(glider): remove the symbolizer path once
+  # https://code.google.com/p/address-sanitizer/issues/detail?id=134 is fixed.
+  symbolizer_path = os.path.abspath(os.path.join(ROOT_DIR, 'third_party',
+      'llvm-build', 'Release+Asserts', 'bin', 'llvm-symbolizer'))
+
+  asan_options = []
+  if lsan:
+    asan_options.append('detect_leaks=1')
+    if sys.platform == 'linux2':
+      # Use the debug version of libstdc++ under LSan. If we don't, there will
+      # be a lot of incomplete stack traces in the reports.
+      extra_env['LD_LIBRARY_PATH'] = '/usr/lib/x86_64-linux-gnu/debug:'

[earthdok, 2014/10/15 14:11:45]

So this is probably useless to you. This points to a system-installed debug
version of libstdc++, which requires the libstdc++-dbg package. Swarming slaves
likely won't have it installed, so this does nothing.

Note that this does not point to
third_party/llvm-build/Release+Asserts/lib/libstdc++.so.6 (the added dependency
in base.isolate). It is news to me that we have a copy of libstdc++ under
llvm-build. We could use it here instead of the system-installed one. However,
we're switching ASan builds to just-built libc++, which means no mode dependency
on libstdc++, which means we can drop this variable altogether.

Linux ASan builds have already switched, but Linux ChromeOS ASan still uses
libstdc++. We can probably live with incomplete stacks on the ChromeOS bots for
a short while.

[earthdok, 2014/10/15 14:35:56]

On second thought, let's change this to point to
third_party/llvm-build/Release+Asserts/lib/ for now. I'm not sure how long
fixing the ChromeOS issue will take.

[jam, 2014/10/15 15:23:25]

I just checked and the swarm slaves, based on connecting to a few of them (and
given that they're the same image), have /usr/lib/x86_64-linux-gnu/debug

since this is what run_test.py does, it seems fine to leave as is. then when
run_test.py is updated, this script can be updated as well.

somewhat of a sidetrack: it's been hard to figure out what I need in this script
given how much of the stuff in run_test.py isn't needed. can someone remove all
the unnecessary setting of flags in it?

[earthdok, 2014/10/15 17:00:37]

But does that directory contain libstc++ DSOs?
Yeah, clean-up is long overdue. We also need to move more stuff to default
options. I'll take a look.

[jam, 2014/10/15 17:23:49]

yep

chrome-bot@swarm126-c4:~$ ls /usr/lib/x86_64-linux-gnu/debug/libstdc++*
/usr/lib/x86_64-linux-gnu/debug/libstdc++.a   
/usr/lib/x86_64-linux-gnu/debug/libstdc++.so.6@
/usr/lib/x86_64-linux-gnu/debug/libstdc++.so@ 
/usr/lib/x86_64-linux-gnu/debug/libstdc++.so.6.0.16

+
+    # LSan is not sandbox-compatible, so we can use online symbolization. In
+    # fact, it needs symbolization to be able to apply suppressions.
+    symbolization_options = ['symbolize=1',
+                             'external_symbolizer_path=%s' % symbolizer_path]
+
+    suppressions_file = os.path.join(ROOT_DIR, 'tools', 'lsan',
+        'suppressions.txt')
+    lsan_options = ['suppressions=%s' % suppressions_file,
+                    'print_suppressions=1']
+    extra_env['LSAN_OPTIONS'] = ' '.join(lsan_options)
+  else:
+    # ASan uses a script for offline symbolization.
+    # Important note: when running ASan with leak detection enabled, we must use
+    # the LSan symbolization options above.
+    symbolization_options = ['symbolize=0']
+
+  asan_options.extend(symbolization_options)
+
+  extra_env['ASAN_OPTIONS'] = ' '.join(asan_options)
+
+  if sys.platform == 'darwin':
+    isolate_output_dir = os.path.abspath(os.path.dirname(cmd[0]))
+    # This is needed because the test binary has @executable_path embedded in it
+    # it that the OS tries to resolve to the cache directory and not the mapped
+    #  directory.
+    extra_env['DYLD_LIBRARY_PATH'] = str(isolate_output_dir)
+

[M-A Ruel, 2014/10/15 13:08:00]

two lines

[jam, 2014/10/15 15:23:25]

Done.

 def run_executable(cmd, env):
   """Runs an executable with:
     - environment variable CR_SOURCE_ROOT set to the root directory.
@@ -72,34 +131,33 @@ def run_executable(cmd, env):
     - environment variable CHROME_DEVEL_SANDBOX set if need
     - Reuses sys.executable automatically.
   """
-  env = collections.defaultdict(str, env)
+  extra_env = {}
   # Many tests assume a English interface...
-  env['LANG'] = 'en_US.UTF-8'
+  extra_env['LANG'] = 'en_US.UTF-8'
   # Used by base/base_paths_linux.cc as an override. Just make sure the default
   # logic is used.
   env.pop('CR_SOURCE_ROOT', None)
-  enable_sandbox_if_required(cmd, env)
+  enable_sandbox_if_required(cmd, env, extra_env)
 
   # Copy logic from  tools/build/scripts/slave/runtest.py.
   asan = '--asan=1' in cmd
   lsan = '--lsan=1' in cmd
-  if lsan and sys.platform == 'linux2':
-    # Use the debug version of libstdc++ under LSan. If we don't, there will
-    # be a lot of incomplete stack traces in the reports.
-    env['LD_LIBRARY_PATH'] = '/usr/lib/x86_64-linux-gnu/debug:'
 
-  if asan and sys.platform == 'darwin':
-    isolate_output_dir = os.path.abspath(os.path.dirname(cmd[0]))
-    # This is needed because the test binary has @executable_path embedded in it
-    # that the OS tries to resolve to the cache directory and not the mapped
-    # directory.
-    env['DYLD_LIBRARY_PATH'] = str(isolate_output_dir)
+  if asan:
+    setup_asan(cmd, extra_env, lsan)
+
+  trim_cmd(cmd)
 
   # Ensure paths are correctly separated on windows.
   cmd[0] = cmd[0].replace('/', os.path.sep)
   cmd = fix_python_path(cmd)
+
+  print 'test_env.py running with\n  Additional test environment: %s\n' \
+      '  Command: %s\n' % (str(extra_env), str(cmd))

[jam, 2014/10/15 03:20:48]

Marc-Antoine: Sergey brought up a good point over email, which is that we lost
this information after switching to swarming. i.e. before run_tests would show
the environment variables and the command. So I started printing it here.

This would have helped catch this regression earlier because it would be easy to
see how the test is run.

[M-A Ruel, 2014/10/15 13:08:00]

Ok, I'd prefer something like:

print('test_env.py enforced env vars: %s' %
    '  '.join('%s=%s' % (k, v) for k, v in sorted(extra_env.iteritems())))
print('  Command: %s' % ' '.join(cmd))

[jam, 2014/10/15 15:23:25]

Done.

+  env.update(extra_env or {})
   try:
-    if asan:
+    # See above comment regarding offline symbolization.
+    if asan and not lsan:
       # Need to pipe to the symbolizer script.
       p1 = subprocess.Popen(cmd, env=env, stdout=subprocess.PIPE,
                             stderr=sys.stdout)