The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/01f608bbcf59a818904459fc563e2d6ff5f49395

commit 01f608bbcf59a818904459fc563e2d6ff5f49395
Author: Nico Weber <thakis@chromium.org>
Date: Wed Jun 06 13:07:52 2018

Don't suppress stderr when calling generate_breakpad_symbols.py

It fails on one of my bots, and I can't tell why.

Bug:  chromium:850055 
Change-Id: I895fc0e789b0ff803f1e2e20c426af4fdbc69959
Reviewed-on: https://chromium-review.googlesource.com/1088751
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Nico Weber <thakis@chromium.org>

[modify] https://crrev.com/01f608bbcf59a818904459fc563e2d6ff5f49395/telemetry/telemetry/internal/backends/chrome/desktop_browser_backend.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29a890af6185fcc251d77738334732093c131d96

commit 29a890af6185fcc251d77738334732093c131d96
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Wed Jun 06 14:36:04 2018

Roll src/third_party/catapult ba9d017..01f608b (1 commits)

https://chromium.googlesource.com/catapult.git/+log/ba9d017..01f608b


git log ba9d017..01f608b --date=short --no-merges --format='%ad %ae %s'
2018-06-06 thakis@chromium.org Don't suppress stderr when calling generate_breakpad_symbols.py


Created with:
  gclient setdep -r src/third_party/catapult@01f608b

The AutoRoll server is located here: https://catapult-roll.skia.org

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: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

BUG= chromium:850055 
TBR=sullivan@chromium.org

Change-Id: I416a35459a6c4ca30a81147a35d16d00dd49f7b0
Reviewed-on: https://chromium-review.googlesource.com/1088810
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#564879}
[modify] https://crrev.com/29a890af6185fcc251d77738334732093c131d96/DEPS

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0cd00af09966bebff27af5a84ab67daf699c3eb7

commit 0cd00af09966bebff27af5a84ab67daf699c3eb7
Author: Nico Weber <thakis@chromium.org>
Date: Fri Jun 08 15:27:29 2018

Reland "Make generate_breakpad_symbols.py not silently ignore subprocess errors."

This reverts commit 5a7973799e44d0313bf5a6df2f11f6fb2ef1474a.

Reason for revert: Relanding without making lld failures critical for now (see https://crbug.com/849904).

Original change's description:
> Revert "Make generate_breakpad_symbols.py not silently ignore subprocess errors."
>
> This reverts commit 924c8465fff86ce13f8a4ca4d352807e0616f0b1.
>
> Reason for revert: This appears to break the Android WebView bots.
>
> https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20WebView%20N%20%28dbg%29
> https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20WebView%20O%20(dbg)
>
> Traceback (most recent call last):
>   File "/b/s/w/ir/cache/builder/src/components/crash/content/tools/generate_breakpad_symbols.py", line 327, in <module>
>     sys.exit(main())
>   File "/b/s/w/ir/cache/builder/src/components/crash/content/tools/generate_breakpad_symbols.py", line 316, in main
>     deps = GetSharedLibraryDependencies(options, queue.pop(0), loader_path)
>   File "/b/s/w/ir/cache/builder/src/components/crash/content/tools/generate_breakpad_symbols.py", line 151, in GetSharedLibraryDependencies
>     deps = GetSharedLibraryDependenciesLinux(binary)
>   File "/b/s/w/ir/cache/builder/src/components/crash/content/tools/generate_breakpad_symbols.py", line 67, in GetSharedLibraryDependenciesLinux
>     ldd = subprocess.check_output(['ldd', binary])
>   File "/b/s/w/ir/cipd_bin_packages/lib/python2.7/subprocess.py", line 219, in check_output
>     raise CalledProcessError(retcode, cmd, output=output)
>
> Original change's description:
> > Make generate_breakpad_symbols.py not silently ignore subprocess errors.
> >
> > GetCommandOuput() used to pipe stderr to /dev/null, and it ignored
> > the command's return code. Use check_output() to check the return
> > code, and keep stderr attached to parent's stderr.
> >
> > Also make breakpad_integration_test.py a bit simpler (this part is
> > supposed to be behavior-preserving.)
> >
> > Bug: 813163
> > Change-Id: I8c55d3da9fff3b944111c3e868121ac34bf65c17
> > Reviewed-on: https://chromium-review.googlesource.com/1086981
> > Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> > Commit-Queue: Nico Weber <thakis@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#564531}
>
> TBR=thakis@chromium.org,jochen@chromium.org
>
> Change-Id: Iee5a81e9b7e9db21f1dda9bdc272d3392438f481
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 813163
> Reviewed-on: https://chromium-review.googlesource.com/1087871
> Reviewed-by: Ted Choc <tedchoc@chromium.org>
> Commit-Queue: Ted Choc <tedchoc@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#564707}

TBR=thakis@chromium.org,tedchoc@chromium.org,jochen@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 813163, 850055 
Change-Id: I3b241dffac522af75c46dc1a7554bd954b2a8f57
Reviewed-on: https://chromium-review.googlesource.com/1092671
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565638}
[modify] https://crrev.com/0cd00af09966bebff27af5a84ab67daf699c3eb7/components/crash/content/tools/generate_breakpad_symbols.py
[modify] https://crrev.com/0cd00af09966bebff27af5a84ab67daf699c3eb7/content/shell/tools/breakpad_integration_test.py

With stderr not suppressed, we can actually se an error message, woot:

https://chromium-swarm.appspot.com/task?id=3dfe999d88322d10&refresh=10&show_raw=1

ERROR: failed to resolve @rpath/libprotobuf_lite.dylib, exe_path /b/s/w/ir/out/Release, loader_path /b/s/w/ir/out/Release/Chromium.app/Contents/Versions/69.0.3454.0/Chromium Helper.app/Contents/MacOS

$ tools/swarming_client/isolateserver.py download -I https://isolateserver.appspot.com --namespace default-gzip -s afa5a7c8075463cde5cec91c2db0fbe8125f1a96 --target foo

$ ls foo/out/Release/libprotobuf_lite.dylib 
foo/out/Release/libprotobuf_lite.dylib

$  components/crash/content/tools/generate_breakpad_symbols.py --binary foo/out/Release/Chromium.app/Contents/Versions/69.0.3454.0/Chromium\ Helper.app/Contents/MacOS/Chromium\ Helper  --symbols-dir foosym --build-dir foo/out/Release/ 
ERROR: failed to resolve @rpath/libprotobuf_lite.dylib, exe_path foo/out/Release, loader_path foo/out/Release/Chromium.app/Contents/Versions/69.0.3454.0/Chromium Helper.app/Contents/MacOS

cf  components/crash/content/tools/generate_breakpad_symbols.py --build-dir=out/gn --binary=out/gn/Content\ Shell.app/Contents/MacOS/Content\ Shell --symbols-dir=foosym 

This fails in the same way:

components/crash/content/tools/generate_breakpad_symbols.py --build-dir=out/gn --binary=out/gn/Content\ Shell.app/Contents/Frameworks/Content\ Shell\ Helper.app/Contents/MacOS/Content\ Shell\ Helper --symbols-dir=foosym 

I wonder if the telemetry stuff should call generate_breakpad_symbols.py only on the main executable, not on the helper...

These tests:

* core.stacktrace_unittest.TabStackTraceTest.testBadBreakpadFileIgnored
* core.stacktrace_unittest.TabStackTraceTest.testCrashMinimalSymbols
* core.stacktrace_unittest.TabStackTraceTest.testCrashSymbols

$ DYLD_PRINT_RPATHS=1 foo/out/Release/Chromium.app/Contents/Versions/69.0.3454.0/Chromium\ Helper.app/Contents/MacOS/Chromium\ Helper  
RPATH successful expansion of @rpath/libseatbelt.dylib to: /Users/thakis/src/chrome/src/foo/out/Release/Chromium.app/Contents/Versions/69.0.3454.0/Chromium Helper.app/Contents/MacOS/../../../../../../../libseatbelt.dylib

With some debug logging:

diff --git a/components/crash/content/tools/generate_breakpad_symbols.py b/components/crash/content/tools/generate_breakpad_symbols.py
index 23b4725f895a..053e94af98b2 100755
--- a/components/crash/content/tools/generate_breakpad_symbols.py
+++ b/components/crash/content/tools/generate_breakpad_symbols.py
@@ -121,6 +121,7 @@ def GetSharedLibraryDependenciesMac(binary, loader_path):
   for idx, line in enumerate(otool):
     if line.find('cmd LC_RPATH') != -1:
       m = re.match(' *path (.*) \(offset .*\)$', otool[idx+2])
+      print m.groups()
       rpaths.append(m.group(1))
     elif line.find('cmd LC_ID_DYLIB') != -1:
       m = re.match(' *name (.*) \(offset .*\)$', otool[idx+2])
@@ -141,8 +142,8 @@ def GetSharedLibraryDependenciesMac(binary, loader_path):
         deps.append(os.path.normpath(dep))
       else:
         print >>sys.stderr, \
-            'ERROR: failed to resolve %s, exe_path %s, loader_path %s' % (
-            m.group(1), exe_path, loader_path)
+            'ERROR: failed to resolve %s, exe_path %s, loader_path %s, rpaths %s' % (
+            m.group(1), exe_path, loader_path, ', '.join(rpaths))
         sys.exit(1)
   return deps
 
@@ -153,6 +154,7 @@ def GetSharedLibraryDependencies(options, binary, loader_path):
   if sys.platform.startswith('linux'):
     deps = GetSharedLibraryDependenciesLinux(binary)
   elif sys.platform == 'darwin':
+    print 'GetSharedLibraryDependencies', binary
     deps = GetSharedLibraryDependenciesMac(binary, loader_path)
   else:
     print "Platform not supported."


The output:

$  components/crash/content/tools/generate_breakpad_symbols.py --binary foo/out/Release/Chromium.app/Contents/Versions/69.0.3454.0/Chromium\ Helper.app/Contents/MacOS/Chromium\ Helper  --symbols-dir foosym --build-dir foo/out/Release/ 
GetSharedLibraryDependencies foo/out/Release/Chromium.app/Contents/Versions/69.0.3454.0/Chromium Helper.app/Contents/MacOS/Chromium Helper
('@loader_path/../../../../../../..',)
('@loader_path/.',)
('@loader_path/../../..',)
GetSharedLibraryDependencies foo/out/Release/libseatbelt.dylib
('@loader_path/.',)
('@loader_path/../../..',)
ERROR: failed to resolve @rpath/libprotobuf_lite.dylib, exe_path foo/out/Release, loader_path foo/out/Release/Chromium.app/Contents/Versions/69.0.3454.0/Chromium Helper.app/Contents/MacOS, rpaths @loader_path/., @loader_path/../../..


I think the problem is that generate_breakpad_symbols.py only collects LC_RPATHs of the executable itself, but not of the loader exec (so libprotobuf_lite.dylib is required by libseatbelt.dylib, but we only look at the rpath of libprotobuf_lite.dylib, not of Chromium Helper, which pulled in libprotobuf_lite.dylib). From `man dyld`:

       @rpath/
              Dyld maintains a current stack of paths called the run path list.  When @rpath is encountered it is substituted with each path in the run path list until a loadable dylib if found.  The run path stack is built from
              the  LC_RPATH  load commands in the depencency chain that lead to the current dylib load.  You can add an LC_RPATH load command to an image with the -rpath option to ld(1).  You can even add a LC_RPATH load command
              path that starts with @loader_path/, and it will push a path on the run path stack that relative to the image containing the LC_RPATH.  The use of @rpath is most useful when you have a complex  directory  structure
              of  programs  and  dylibs  which  can  be  installed anywhere, but keep their relative positions.  This scenario could be implemented using @loader_path, but every client of a dylib could need a different load path
              because its relative position in the file system is different. The use of @rpath introduces a level of indirection that simplies things.  You pick a location in your directory structure as an  anchor  point.   Each
              dylib  then  gets  an install path that starts with @rpath and is the path to the dylib relative to the anchor point. Each main executable is linked with -rpath @loader_path/zzz, where zzz is the path from the exe-
              cutable to the anchor point.  At runtime dyld sets it run path to be the anchor point, then each dylib is found relative to the anchor point.


So we should implement this.

But we're kind of implementing our own dynamic loader bits here; maybe we could coax dyld to just do this for us?


Also, the reason this now fails is because I made the test fail loudly on errors. In practice, we already generate symbols for all the dylibs when we process the browser executable (which also symbolizes the helper), so we're failing when we do duplicate work -- not doing the duplicate work would hide this problem too.

But catapult just calls generate_breakpad_symbols.py for each binary referenced in the minidump and doesn't know what's been done already and what hasn't.

Maybe generate_breakpad_symbols.py could check if the symbols for a binary already exist, and if so and its timestamp is newer than the binary, not do anything. Then things would work as long as the main browser binary is symbolized before the helper.


The Right Fix is probably to fix the rpath behavior and to also do the timestamp bit -- then the order doesn't matter and we still don't do dupe work.

Nicos-MacBook-Pro:src thakis$ otool -L foo/out/Release/Chromium.app/Contents/Versions/69.0.3454.0/Chromium\ Helper.app/Contents/MacOS/Chromium\ Helper 
foo/out/Release/Chromium.app/Contents/Versions/69.0.3454.0/Chromium Helper.app/Contents/MacOS/Chromium Helper:
	@rpath/libseatbelt.dylib (compatibility version 0.0.0, current version 0.0.0)
	@rpath/libprotobuf_lite.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 307.5.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.50.2)
Nicos-MacBook-Pro:src thakis$ otool -L foo/out/Release/libseatbelt.dylib 
foo/out/Release/libseatbelt.dylib:
	@rpath/libseatbelt.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libsandbox.1.dylib (compatibility version 1.0.0, current version 1.0.0)
	@rpath/libprotobuf_lite.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 307.5.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.50.2)



Just doing BFS is probably enough to paper over the issues, at least in this case...

Hm, actually I think the exe_path/loader_path swap in https://chromium-review.googlesource.com/c/chromium/src/+/1085864 was incorrect and that bug needed the rpath stack, not the one here.

https://chromium-review.googlesource.com/c/chromium/src/+/1095834

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/94c597a905a0234a6b04709281919fdee5e07984

commit 94c597a905a0234a6b04709281919fdee5e07984
Author: Nico Weber <thakis@chromium.org>
Date: Mon Jun 11 20:52:58 2018

Let generate_breakpad_symbols.py use a stack of rpaths on mac.

According to `man dyld`, the rpath search list is a stack of all rpaths
found in all binary images on the path resulting in a load, see comment 14
on  bug 850055 .

Also revert the changes exe_path/loader_path swap from
https://chromium-review.googlesource.com/1085864 , it was incorrect
and is no longer needed after the rpath stack fix.

TEST=both of these commands work:
components/crash/content/tools/generate_breakpad_symbols.py --binary out/gn/Content\ Shell.app/Contents/MacOS/Content\ Shell  --symbols-dir foosym --build-dir out/gn
components/crash/content/tools/generate_breakpad_symbols.py --binary out/gn/Chromium.app/Contents/Versions/69.0.3454.0/Chromium\ Helper.app/Contents/MacOS/Chromium\ Helper  --symbols-dir foosym --build-dir out/gn

Bug:  849381 , 850055 
Change-Id: I28502059f444b5ba523763941b1490c1d2ad5d73
Reviewed-on: https://chromium-review.googlesource.com/1095834
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566140}
[modify] https://crrev.com/94c597a905a0234a6b04709281919fdee5e07984/components/crash/content/tools/generate_breakpad_symbols.py

Progress, now it fails with a different error later on:

[23/24] core.stacktrace_unittest.TabStackTraceTest.testBadBreakpadFileIgnored queuedERROR: failed to resolve @rpath/libchrome_dll.dylib, exe_path /b/s/w/ir/out/Release/Chromium.app/Contents/Versions/69.0.3456.0/Chromium Framework.framework/Versions/A, loader_path /b/s/w/ir/out/Release/Chromium.app/Contents/Versions/69.0.3456.0/Chromium Framework.framework/Versions/A, rpaths /b/s/w/ir/out/Release/Chromium.app/Contents/Versions/69.0.3456.0/Chromium Framework.framework/Versions/A/../../../../.., /b/s/w/ir/out/Release/Chromium.app/Contents/Versions/69.0.3456.0/Chromium Framework.framework/Versions/A/., /b/s/w/ir/out/Release/Chromium.app/Contents/Versions/69.0.3456.0/Chromium Framework.framework/Versions/A/../../..

(https://chromium-swarm.appspot.com/task?id=3e0c1a57b75e0210&refresh=10&show_raw=1)



Called like so:

  Failed to execute "/b/s/w/ir/.swarming_module_cache/vpython/fe1f6b/bin/python /b/s/w/ir/components/crash/content/tools/generate_breakpad_symbols.py --binary=/b/s/w/ir/out/Release/Chromium.app/Contents/Versions/69.0.3456.0/Chromium Framework.framework/Versions/A/Chromium Framework --symbols-dir=/b/s/w/itb9Ehxr/tmpXetlIy/symbols --build-dir=/b/s/w/ir/out/Release"


Not sure if this _should_ work -- can't load the framework without an executable around it. So maybe catapult / telemetry need to filter out non-executable binaries from the dump_minidump output?

https://chromium-review.googlesource.com/c/chromium/src/+/1093569 might have helped, I'm waiting for the bot to cycle.

Two points:

1. On one hand, it's kind of weird that the rpath change in 18 was required -- it'd be nice if each binary (helper, framework, etc) itself contained the rpaths to find stuff it needs. If that was the case (and after 1093569 it might be) then the change in 18 wouldn't be needed.

2. But if we _do_ assume the rpath stack thingy, then only the executables would need to set rpaths to the build dir, the frameworks shouldn't have to. But that doesn't work with the current symbolication setup where telemetry calls generate_breakpad_symbols.py for each binary in the minidump (including frameworks).


I think I'll just hope that things cycle green and then call it a day.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73e546d203cc194e10979e2faa0ab1b295ada217

commit 73e546d203cc194e10979e2faa0ab1b295ada217
Author: Nico Weber <thakis@chromium.org>
Date: Thu Jun 14 16:00:32 2018

mac: Try harder to get telemetry_perf_unittests/stacktrace_unittest work in component builds.

The test tries to symbolize Chromium Framework independently, so the outer executable's
rpath isn't on the stack. The Framework does have an rpath in component builds, but
it is currently os.path.join(5 * ['..']), which relative to
out/Release/Chromium.app/Contents/Versions/69.0.3456.0/Chromium Framework.framework/Versions/A
gets us to out/Release/Chromium.app/Contents.  That doesn't seem useful for anything
(from what I can tell, it's always been this way since at least the gn switch),
so use 7 * ['..'] instead to get to the build dir.

This possibly fixes a regression from the ancient https://codereview.chromium.org/2700013002

Bug:  850055 
Change-Id: I9e65a7fa76620539b2492fd6096b70533a523a74
Reviewed-on: https://chromium-review.googlesource.com/1101064
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567288}
[modify] https://crrev.com/73e546d203cc194e10979e2faa0ab1b295ada217/chrome/BUILD.gn

https://ci.chromium.org/buildbot/chromium.clang/ToTMac/1759 \o/

Should probably undo the rpath stuff again. It's technically correct, but we need full rpaths for this test anyhow.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39a3a9de6ce94ae4e5ef308bd056e3e7c0d6b956

commit 39a3a9de6ce94ae4e5ef308bd056e3e7c0d6b956
Author: Nico Weber <thakis@chromium.org>
Date: Fri Jun 15 16:35:36 2018

mac: Remove rpath stack handling from generate_breakpad_symbols.py again.

Doing rpath stack handling is the correct thing to do, but since the framework
needs to be able to be symbolized on its own for the telemetry crash tests,
it needs to have all the rpaths it needs directly -- and then this code isn't
needed.  Removing it makes telemetry_perf_unittests's core.stacktrace_unittests
and content_shell_crash_test both fail if a framework has incomplete rpaths.

Reverts the parts of https://chromium-review.googlesource.com/c/chromium/src/+/1095834
that weren't a revert.

Bug:  850055 
Change-Id: I9d0c4bd0d64ffbcfc03184dc761a4a2694b6754d
Reviewed-on: https://chromium-review.googlesource.com/1102056
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567684}
[modify] https://crrev.com/39a3a9de6ce94ae4e5ef308bd056e3e7c0d6b956/components/crash/content/tools/generate_breakpad_symbols.py