Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2974163002:
Fix the stack script issue when symbolizing tombstones. (Closed)
|
Created:
3 years, 5 months ago by BigBossZhiling Modified:
3 years, 4 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the stack script issue when symbolizing tombstones.
Include the *.so files in lib.unstripped and the *.so files that are in apks so that we have the right symbols when symbolizing tombstones.
BUG=743268
Review-Url: https://codereview.chromium.org/2974163002
Cr-Commit-Position: refs/heads/master@{#489882}
Committed: https://chromium.googlesource.com/chromium/src/+/55fc2af69da5f96e351e5aea26cfe75b95cf1887
Patch Set 1 #
Total comments: 4
Patch Set 2 : add unziped so files as packed-lib #
Total comments: 5
Patch Set 3 : include unstrippped libs #
Total comments: 5
Patch Set 4 : add tombstones_helper into git #
Total comments: 6
Patch Set 5 : created symbolizer.py #
Total comments: 23
Patch Set 6 : address John's comments #
Total comments: 21
Patch Set 7 : address John's comments #
Total comments: 4
Patch Set 8 : address John's comments #Patch Set 9 : remove changes in gcc_toolchain and change GNGen instead #
Total comments: 22
Patch Set 10 : put mb.py code under if android #Patch Set 11 : address andrew's comments, enable intentional crash test case #
Total comments: 2
Patch Set 12 : address Andrew's comments #
Total comments: 4
Patch Set 13 : add todo and crbug; revert changes for intentional crash test case #Messages
Total messages: 62 (20 generated)
hzl@google.com changed reviewers: + agrieve@chromium.org, jbudorick@chromium.org
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
Well, agrieve looks like he is OOO and I think John is too. So I can review this if you want. Is there a bug or issue for this CL btw? https://codereview.chromium.org/2974163002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2974163002/diff/1/build/android/test_runner.p... build/android/test_runner.py:794: shell=True) where is the unzipped APK used? https://codereview.chromium.org/2974163002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/CrashTest.java (right): https://codereview.chromium.org/2974163002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/CrashTest.java:17: // import org.chromium.base.test.util.DisabledTest; Did you mean to include this change?
https://codereview.chromium.org/2974163002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2974163002/diff/1/build/android/test_runner.p... build/android/test_runner.py:794: shell=True) On 2017/07/10 22:51:33, mikecase wrote: > where is the unzipped APK used? hmm I thought the stack file would automatically search for *.so files in <out-dir>/lib. It doesn't seem so. I added the *.so as --packed-lib args for the stack script. https://codereview.chromium.org/2974163002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/CrashTest.java (right): https://codereview.chromium.org/2974163002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/CrashTest.java:17: // import org.chromium.base.test.util.DisabledTest; On 2017/07/10 22:51:33, mikecase wrote: > Did you mean to include this change? Yes. So that the trybot would run into a crashed test case and the tombstone will actually be symbolized, for better view of whether the code is doing what it is supposed to do. However the change in this file would be deleted before I land.
nit: add bug or remove the BUG= (preferably add bug) https://codereview.chromium.org/2974163002/diff/20001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/2974163002/diff/20001/build/android/test_runn... build/android/test_runner.py:794: shell=True) Would it make more sense for the stack/tombstone script to unzip the APK. That way the unzipped APK is used closer to where it is unzipped? idk, suggestion. Or maybe unzip the APK just before running the tombstone script?
https://codereview.chromium.org/2974163002/diff/20001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/2974163002/diff/20001/build/android/test_runn... build/android/test_runner.py:794: shell=True) On 2017/07/11 21:42:01, mikecase wrote: > Would it make more sense for the stack/tombstone script to unzip the APK. That > way the unzipped APK is used closer to where it is unzipped? idk, suggestion. > > Or maybe unzip the APK just before running the tombstone script? We had discussed doing the latter last week, w/ the discussion about lazy unzipping. Please look at doing so. https://codereview.chromium.org/2974163002/diff/20001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2974163002/diff/20001/build/android/tombstone... build/android/tombstones.py:141: cmd.extend(['--packed-lib', os.path.join(root, file_name)]) --packed-lib implies that the library has had its relocations packed. It doesn't look like we know that to be the case here.
Description was changed from ========== Fix the stack script issue when symbolizing tombstones. BUG= ========== to ========== Fix the stack script issue when symbolizing tombstones. BUG=743268 ==========
Description was changed from ========== Fix the stack script issue when symbolizing tombstones. BUG=743268 ========== to ========== Fix the stack script issue when symbolizing tombstones. Include the *.so files in lib.unstripped and the *.so files that are in apks so that we have the right symbols when symbolizing tombstones. BUG=743268 ==========
https://codereview.chromium.org/2974163002/diff/20001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/2974163002/diff/20001/build/android/test_runn... build/android/test_runner.py:794: shell=True) On 2017/07/12 20:57:28, jbudorick wrote: > On 2017/07/11 21:42:01, mikecase wrote: > > Would it make more sense for the stack/tombstone script to unzip the APK. That > > way the unzipped APK is used closer to where it is unzipped? idk, suggestion. > > > > Or maybe unzip the APK just before running the tombstone script? > > We had discussed doing the latter last week, w/ the discussion about lazy > unzipping. Please look at doing so. I just added the unzip logic in tombstones.py, because the unzip logic always will be called at the same time when we want to resolve tombstones. Or do you think I should put it into instrumentation test run? https://codereview.chromium.org/2974163002/diff/20001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2974163002/diff/20001/build/android/tombstone... build/android/tombstones.py:141: cmd.extend(['--packed-lib', os.path.join(root, file_name)]) On 2017/07/12 20:57:28, jbudorick wrote: > --packed-lib implies that the library has had its relocations packed. It doesn't > look like we know that to be the case here. unstripped libs will always be there now in isolated inputs. So I believe stack script will search in unstripped directory first by default?
https://codereview.chromium.org/2974163002/diff/40001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2974163002/diff/40001/build/android/tombstone... build/android/tombstones.py:140: cmd.extend(['--packed-lib', os.path.join(root, file_name)]) For symbols, the script will find the lib.unstripped library first, so that's fine. But this will still *always* indicate to the stack script that packed relocations need to be accounted for, and that's not the case. I think that builds that don't use packed relocations would wind up w/ garbled stacks because of this. https://codereview.chromium.org/2974163002/diff/40001/build/android/tombstone... build/android/tombstones.py:261: 'unzip -o %s -d %s' % (apk_under_test, nit: pass this as a list, i.e. ['unzip', ...] https://codereview.chromium.org/2974163002/diff/40001/build/android/tombstone... build/android/tombstones.py:262: constants.GetOutDirectory()), I don't think we should extract this directly into the output directory. A subdirectory of the output directory would be ok, I suppose. I'd suggest a temporary directory, but down here in ResolveTombstones, that'd mean that we unzip *every time* we resolve a tombstone, which is not what we want to do. https://codereview.chromium.org/2974163002/diff/40001/build/android/tombstone... build/android/tombstones.py:263: shell=True) This shouldn't need shell features.
https://codereview.chromium.org/2974163002/diff/40001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2974163002/diff/40001/build/android/tombstone... build/android/tombstones.py:260: subprocess.check_call( Er, sorry, thought of this after sending the review: we're also going to want to symbolize other things, not just tombstones (e.g. logcat and possibly stdout). As such, we should be doing the unzipping outside of tombstones.
https://codereview.chromium.org/2974163002/diff/60001/build/android/tombstone... File build/android/tombstones_helper.py (right): https://codereview.chromium.org/2974163002/diff/60001/build/android/tombstone... build/android/tombstones_helper.py:1: #!/usr/bin/env python This shouldn't be executable and shouldn't start with the shebang line. https://codereview.chromium.org/2974163002/diff/60001/build/android/tombstone... build/android/tombstones_helper.py:2: # This file should be in pylib/ somewhere -- probably in pylib/utils/. Very little should be added directly to //build/android at this point. https://codereview.chromium.org/2974163002/diff/60001/build/android/tombstone... build/android/tombstones_helper.py:11: class TombstonesHelper(object): I probably wasn't clear enough with my intended design here. The new object should be responsible for symbolization generally, not just for tombstones. (I think we'd like to use it to symbolize stacks in other contexts.) As such, it shouldn't be a wrapper around tombstones. This may require reworking tombstones itself a bit. Let's talk about this a bit more when you get in.
https://codereview.chromium.org/2974163002/diff/60001/build/android/tombstone... File build/android/tombstones_helper.py (right): https://codereview.chromium.org/2974163002/diff/60001/build/android/tombstone... build/android/tombstones_helper.py:1: #!/usr/bin/env python On 2017/07/18 14:11:47, jbudorick wrote: > This shouldn't be executable and shouldn't start with the shebang line. Done. https://codereview.chromium.org/2974163002/diff/60001/build/android/tombstone... build/android/tombstones_helper.py:2: # On 2017/07/18 14:11:47, jbudorick wrote: > This file should be in pylib/ somewhere -- probably in pylib/utils/. Very little > should be added directly to //build/android at this point. Done. https://codereview.chromium.org/2974163002/diff/60001/build/android/tombstone... build/android/tombstones_helper.py:11: class TombstonesHelper(object): On 2017/07/18 14:11:46, jbudorick wrote: > I probably wasn't clear enough with my intended design here. The new object > should be responsible for symbolization generally, not just for tombstones. (I > think we'd like to use it to symbolize stacks in other contexts.) As such, it > shouldn't be a wrapper around tombstones. This may require reworking tombstones > itself a bit. > > Let's talk about this a bit more when you get in. Done.
Looking pretty good. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:633: self._unzipped_files_dir = tempfile.mkdtemp() Hmm. Why should the test instance (or the client generally) be responsible for the symbolizer's unzipped files directory? https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:536: symbolizer = self._test_instance.symbolizer nit: just use this in the call; no need to assign it to a local here. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:546: with open('/tmp/tmp_tombstones.txt', 'w') as f: Leftover from debugging? https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... File build/android/pylib/utils/symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:30: self.packed_libs = [] nit: maybe apk_libs rather than packed_libs, since they may not be packed. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:34: def UnzipAPK(self): I don't think clients should explicitly call this, so it should be _UnzipAPK. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:35: subprocess.check_call( nit: cmd_helper.GetCommandStatusAndOutput, though here you can ignore the output. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:51: include_stack: boolean whether to include stack data in output. Would it make sense for this to have a default? https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:72: stack_tool = os.path.join(os.path.dirname(__file__), '..', '..', '..', '..', nit: This should be a constant at module scope. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:80: proc = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE) Use cmd_helper.GetCommandStatusAndOutput here instead of a direct subprocess call. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:82: for line in output.split('\n'): nit: output.splitlines()? https://codereview.chromium.org/2974163002/diff/80001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2974163002/diff/80001/build/android/tombstone... build/android/tombstones.py:128: data = pool.map( I'm a little concerned about this -- what happens if we have multiple tombstones? Will multiple symbolizers all attempt to unzip the same APK? https://codereview.chromium.org/2974163002/diff/80001/build/android/tombstone... build/android/tombstones.py:201: tombstone_symbolizer=None): Not exactly what I had in mind, but I like this too.
https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:633: self._unzipped_files_dir = tempfile.mkdtemp() On 2017/07/20 15:58:03, jbudorick wrote: > Hmm. Why should the test instance (or the client generally) be responsible for > the symbolizer's unzipped files directory? Done. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:536: symbolizer = self._test_instance.symbolizer On 2017/07/20 15:58:03, jbudorick wrote: > nit: just use this in the call; no need to assign it to a local here. Done. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:546: with open('/tmp/tmp_tombstones.txt', 'w') as f: On 2017/07/20 15:58:03, jbudorick wrote: > Leftover from debugging? Done. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... File build/android/pylib/utils/symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:30: self.packed_libs = [] On 2017/07/20 15:58:04, jbudorick wrote: > nit: maybe apk_libs rather than packed_libs, since they may not be packed. Done. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:34: def UnzipAPK(self): On 2017/07/20 15:58:04, jbudorick wrote: > I don't think clients should explicitly call this, so it should be _UnzipAPK. Let the clients call UnzipAPK, so that the clients can easily call ResolveSymbols on parallel, without worrying about calling UnzipAPK too many times. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:35: subprocess.check_call( On 2017/07/20 15:58:04, jbudorick wrote: > nit: cmd_helper.GetCommandStatusAndOutput, though here you can ignore the > output. Done. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:51: include_stack: boolean whether to include stack data in output. On 2017/07/20 15:58:04, jbudorick wrote: > Would it make sense for this to have a default? Done. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:72: stack_tool = os.path.join(os.path.dirname(__file__), '..', '..', '..', '..', On 2017/07/20 15:58:04, jbudorick wrote: > nit: This should be a constant at module scope. Done. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:80: proc = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE) On 2017/07/20 15:58:04, jbudorick wrote: > Use cmd_helper.GetCommandStatusAndOutput here instead of a direct subprocess > call. Done. https://codereview.chromium.org/2974163002/diff/80001/build/android/pylib/uti... build/android/pylib/utils/symbolizer.py:82: for line in output.split('\n'): On 2017/07/20 15:58:03, jbudorick wrote: > nit: output.splitlines()? Done. https://codereview.chromium.org/2974163002/diff/80001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/2974163002/diff/80001/build/android/tombstone... build/android/tombstones.py:201: tombstone_symbolizer=None): On 2017/07/20 15:58:04, jbudorick wrote: > Not exactly what I had in mind, but I like this too. Acknowledged.
https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:448: self._symbolizer = symbolizer.Symbolizer() This should be None. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... File build/android/pylib/utils/symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:40: if self.libs_dir: nit: extract this into a separate function that __del__ calls. A client should be able to tell Symbolizer when its work is done. instrumentation_test_instance, for example, could call the new function in its TearDown method. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:44: def UnzipAPK(self): nit: add a docstring for this. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:45: cmd_helper.GetCmdOutput( This function should know the conditions under which the apk should be unzipped & check those. To go along with that, it should probably be named "UnzipApkIfNecessary" or something similar. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:67: # Check if the data_to_symbolize has an ABI listed, I think this function should still attempt to unzip the APK. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:70: found_abi = re.search('ABI: \'(.+?)\'', line) nit: compile this regex into a module-scope constant https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:74: if not arch: nit: log a warning in this case. https://codereview.chromium.org/2974163002/diff/100001/build/android/tombston... File build/android/tombstones.py (right): https://codereview.chromium.org/2974163002/diff/100001/build/android/tombston... build/android/tombstones.py:125: if (not tombstone_symbolizer.has_unzipped and As mentioned in symbolizer, this check should be done inside UnzipAPK https://codereview.chromium.org/2974163002/diff/100001/build/android/tombston... build/android/tombstones.py:224: (tombstone_symbolizer or nit: move 'or' down onto the next line
https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... File build/android/pylib/utils/symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:1: # Copyright 2017 The Chromium Authors. All rights reserved. nit: there's already a directory in pylib called "symbols", which has an "elf_symbolizer.py", which is a wrapper around addr2line. The stack script is also a wrapper around addr2line (but with added logic for finding the stack lines from a log, as well as the ability to call objdump, and to adjust addresses for relros & being loaded from .apk). Anyways, the two are cetainly different enough to not want to merge them, but I think it would still make sense to put this under "symbols" and put "stack" in its name to distinguish it from elf_symbolizer.py
https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... File build/android/pylib/utils/symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:1: # Copyright 2017 The Chromium Authors. All rights reserved. On 2017/07/21 14:12:46, agrieve wrote: > nit: there's already a directory in pylib called "symbols", which has an > "elf_symbolizer.py", which is a wrapper around addr2line. > > The stack script is also a wrapper around addr2line (but with added logic for > finding the stack lines from a log, as well as the ability to call objdump, and > to adjust addresses for relros & being loaded from .apk). > > Anyways, the two are cetainly different enough to not want to merge them, but I > think it would still make sense to put this under "symbols" and put "stack" in > its name to distinguish it from elf_symbolizer.py That sounds fine to me. At some point, I want to reconcile the two & the stuff in android_platform -- there's quite a bit of chrome-specific logic in there.
https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:448: self._symbolizer = symbolizer.Symbolizer() On 2017/07/21 13:46:23, jbudorick wrote: > This should be None. Done. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... File build/android/pylib/utils/symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:1: # Copyright 2017 The Chromium Authors. All rights reserved. On 2017/07/21 14:12:46, agrieve wrote: > nit: there's already a directory in pylib called "symbols", which has an > "elf_symbolizer.py", which is a wrapper around addr2line. > > The stack script is also a wrapper around addr2line (but with added logic for > finding the stack lines from a log, as well as the ability to call objdump, and > to adjust addresses for relros & being loaded from .apk). > > Anyways, the two are cetainly different enough to not want to merge them, but I > think it would still make sense to put this under "symbols" and put "stack" in > its name to distinguish it from elf_symbolizer.py Done. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:40: if self.libs_dir: On 2017/07/21 13:46:23, jbudorick wrote: > nit: extract this into a separate function that __del__ calls. A client should > be able to tell Symbolizer when its work is done. instrumentation_test_instance, > for example, could call the new function in its TearDown method. Say this new function is named CleanUp. If the instrumentation_test_instance calls CleanUp in TearDown once, we don't want to call CleanUp again in __del__ right? So I am setting libs_dir to None once we call CleanUp, so the same directory won't be removed twice. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:44: def UnzipAPK(self): On 2017/07/21 13:46:23, jbudorick wrote: > nit: add a docstring for this. Done. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:45: cmd_helper.GetCmdOutput( On 2017/07/21 13:46:23, jbudorick wrote: > This function should know the conditions under which the apk should be unzipped > & check those. To go along with that, it should probably be named > "UnzipApkIfNecessary" or something similar. Done. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:67: # Check if the data_to_symbolize has an ABI listed, On 2017/07/21 13:46:23, jbudorick wrote: > I think this function should still attempt to unzip the APK. Done. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:70: found_abi = re.search('ABI: \'(.+?)\'', line) On 2017/07/21 13:46:23, jbudorick wrote: > nit: compile this regex into a module-scope constant Done. https://codereview.chromium.org/2974163002/diff/100001/build/android/pylib/ut... build/android/pylib/utils/symbolizer.py:74: if not arch: On 2017/07/21 13:46:23, jbudorick wrote: > nit: log a warning in this case. Done. https://codereview.chromium.org/2974163002/diff/100001/build/android/tombston... File build/android/tombstones.py (right): https://codereview.chromium.org/2974163002/diff/100001/build/android/tombston... build/android/tombstones.py:125: if (not tombstone_symbolizer.has_unzipped and On 2017/07/21 13:46:24, jbudorick wrote: > As mentioned in symbolizer, this check should be done inside UnzipAPK Done. https://codereview.chromium.org/2974163002/diff/100001/build/android/tombston... build/android/tombstones.py:224: (tombstone_symbolizer or On 2017/07/21 13:46:23, jbudorick wrote: > nit: move 'or' down onto the next line Done.
lgtm w/ nit https://codereview.chromium.org/2974163002/diff/120001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2974163002/diff/120001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:858: def TearDown(self): nit: call self._symbolizer.CleanUp() here https://codereview.chromium.org/2974163002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/CrashTest.java (right): https://codereview.chromium.org/2974163002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/CrashTest.java:17: // import org.chromium.base.test.util.DisabledTest; Friendly reminder to back this change out before landing.
The CQ bit was checked by hzl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2974163002/diff/120001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2974163002/diff/120001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:858: def TearDown(self): On 2017/07/21 19:50:00, jbudorick wrote: > nit: call self._symbolizer.CleanUp() here Done. https://codereview.chromium.org/2974163002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/CrashTest.java (right): https://codereview.chromium.org/2974163002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/CrashTest.java:17: // import org.chromium.base.test.util.DisabledTest; On 2017/07/21 19:50:00, jbudorick wrote: > Friendly reminder to back this change out before landing. Done.
The CQ bit was unchecked by hzl@google.com
The CQ bit was checked by hzl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2974163002/#ps140001 (title: "address John's comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by hzl@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by hzl@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jbudorick@chromium.org
On 2017/07/24 21:08:11, jbudorick wrote: > The CQ bit was unchecked by mailto:jbudorick@chromium.org This has failed in the same way five separate times. It's likely an issue w/ the CL.
https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... File build/android/pylib/symbols/stack_symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:14: STACK_TOOL = os.path.join(os.path.dirname(__file__), '..', '..', '..', '..', nit: prefix with _ https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:18: nit: 2 blank lines https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:30: """ A helper class to symbolize stack. """ nit: remove spaces before/after """ https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:34: self.apk_under_test = apk_under_test nit: prefix with _ unless you need these to be public https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:42: self.CleanUp() Maybe log a warning if CleanUp() has not already been called a this point? https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:59: cmd_helper.GetCmdOutput( Python's zipfile module makes extracting easy. You should prefer it over calling unzip since it's more portable & easier to get right. Example: https://cs.chromium.org/chromium/src/build/android/gyp/util/build_utils.py?q=... https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:70: def ResolveSymbols(self, data_to_symbolize, device_abi, include_stack=True): This is somewhat misleading. While the stack tool does symbolize, it also extracts stacks (all non-stack output is removed). Maybe call it "ExtractAndResolveNativeStackTraces" https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:85: found_abi = ABI_REG.match(line) The stack script already does this logic: https://cs.chromium.org/chromium/src/third_party/android_platform/development... Is it necessary to do it here as well? https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:103: if not include_stack and 'Stack Data:' in line: Won't this skip over other stack traces that are in the file? Or are tombstones guaranteed to have only one? https://codereview.chromium.org/2974163002/diff/160001/build/android/tombston... File build/android/tombstones.py (right): https://codereview.chromium.org/2974163002/diff/160001/build/android/tombston... build/android/tombstones.py:132: [[tombstone, tombstone_symbolizer] for tombstone in tombstones]) Tombstone symbolizer has a __del__ method that deletes the extracted files upon garbage collection. Here, you're marshalling Symbolizers to sub-processes, which I'd guess all call its __del__ upon completion. Seeing as you're invoking stack script via subprocess anyways, I think it would be much safer to use threads here rather than a process pool. http://lucasb.eyer.be/snips/python-thread-pool.html
Addressed Andrew's comments. I enabled the intentional crash test cases and the tombstone seems to be properly symbolized. lib.unstripped is uploaded correctly. n5x_swarming_rel is not giving us errors anymore. https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... File build/android/pylib/symbols/stack_symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:14: STACK_TOOL = os.path.join(os.path.dirname(__file__), '..', '..', '..', '..', On 2017/07/26 01:26:10, agrieve wrote: > nit: prefix with _ Done. https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:18: On 2017/07/26 01:26:10, agrieve wrote: > nit: 2 blank lines Done. https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:30: """ A helper class to symbolize stack. """ On 2017/07/26 01:26:10, agrieve wrote: > nit: remove spaces before/after """ Done. https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:34: self.apk_under_test = apk_under_test On 2017/07/26 01:26:10, agrieve wrote: > nit: prefix with _ unless you need these to be public Done. https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:42: self.CleanUp() On 2017/07/26 01:26:10, agrieve wrote: > Maybe log a warning if CleanUp() has not already been called a this point? Done. https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:59: cmd_helper.GetCmdOutput( On 2017/07/26 01:26:10, agrieve wrote: > Python's zipfile module makes extracting easy. You should prefer it over calling > unzip since it's more portable & easier to get right. > > Example: > https://cs.chromium.org/chromium/src/build/android/gyp/util/build_utils.py?q=... Done. https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:70: def ResolveSymbols(self, data_to_symbolize, device_abi, include_stack=True): On 2017/07/26 01:26:10, agrieve wrote: > This is somewhat misleading. While the stack tool does symbolize, it also > extracts stacks (all non-stack output is removed). Maybe call it > "ExtractAndResolveNativeStackTraces" Done. https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:85: found_abi = ABI_REG.match(line) On 2017/07/26 01:26:10, agrieve wrote: > The stack script already does this logic: > https://cs.chromium.org/chromium/src/third_party/android_platform/development... > > Is it necessary to do it here as well? Done. https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:103: if not include_stack and 'Stack Data:' in line: On 2017/07/26 01:26:10, agrieve wrote: > Won't this skip over other stack traces that are in the file? Or are tombstones > guaranteed to have only one? My understanding is that stack trace and stack data are two different things. https://luci-logdog.appspot.com/v/?s=chromium%2Fandroid%2Fswarming%2Flogcats%... You will see stack traces in the sample resolved tombstone there. There are multiple of them. We only filter out Stack Data if it exists. https://codereview.chromium.org/2974163002/diff/160001/build/android/tombston... File build/android/tombstones.py (right): https://codereview.chromium.org/2974163002/diff/160001/build/android/tombston... build/android/tombstones.py:132: [[tombstone, tombstone_symbolizer] for tombstone in tombstones]) On 2017/07/26 01:26:10, agrieve wrote: > Tombstone symbolizer has a __del__ method that deletes the extracted files upon > garbage collection. Here, you're marshalling Symbolizers to sub-processes, which > I'd guess all call its __del__ upon completion. > > Seeing as you're invoking stack script via subprocess anyways, I think it would > be much safer to use threads here rather than a process pool. > > http://lucasb.eyer.be/snips/python-thread-pool.html > Done.
lgtm /w a couple more nits https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... File build/android/pylib/symbols/stack_symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:103: if not include_stack and 'Stack Data:' in line: On 2017/07/26 17:36:27, BigBossZhiling wrote: > On 2017/07/26 01:26:10, agrieve wrote: > > Won't this skip over other stack traces that are in the file? Or are > tombstones > > guaranteed to have only one? > > My understanding is that stack trace and stack data are two different things. > https://luci-logdog.appspot.com/v/?s=chromium%2Fandroid%2Fswarming%2Flogcats%... > You will see stack traces in the sample resolved tombstone there. There are > multiple of them. We only filter out Stack Data if it exists. Actually, it looks like these lines are added only when --more-info is passed to stack script. Probably don't need to post-process at all. https://codereview.chromium.org/2974163002/diff/200001/build/android/pylib/sy... File build/android/pylib/symbols/stack_symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/200001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:75: self._apk_libs.append(os.path.join(root, file_name)) nit: shouldn't need to do a separate tree walk here now. Change previous loop to: self._apk_libs.append(z.extract(name, self._libs_dir))
https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... File build/android/pylib/symbols/stack_symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/160001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:103: if not include_stack and 'Stack Data:' in line: On 2017/07/26 17:50:38, agrieve wrote: > On 2017/07/26 17:36:27, BigBossZhiling wrote: > > On 2017/07/26 01:26:10, agrieve wrote: > > > Won't this skip over other stack traces that are in the file? Or are > > tombstones > > > guaranteed to have only one? > > > > My understanding is that stack trace and stack data are two different things. > > > https://luci-logdog.appspot.com/v/?s=chromium%2Fandroid%2Fswarming%2Flogcats%... > > You will see stack traces in the sample resolved tombstone there. There are > > multiple of them. We only filter out Stack Data if it exists. > > Actually, it looks like these lines are added only when --more-info is passed to > stack script. Probably don't need to post-process at all. Acknowledged. I didn't notice there is a more-info tag. Thanks for pointing it out. Instead I am adding the --more-info to the call of stack script and keep the check statement here. Does that sound good? https://codereview.chromium.org/2974163002/diff/200001/build/android/pylib/sy... File build/android/pylib/symbols/stack_symbolizer.py (right): https://codereview.chromium.org/2974163002/diff/200001/build/android/pylib/sy... build/android/pylib/symbols/stack_symbolizer.py:75: self._apk_libs.append(os.path.join(root, file_name)) On 2017/07/26 17:50:38, agrieve wrote: > nit: shouldn't need to do a separate tree walk here now. Change previous loop > to: > > self._apk_libs.append(z.extract(name, self._libs_dir)) Done.
https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py#newcode889 tools/mb/mb.py:889: if android: I'm concerned about hacking this into mb.py rather than having it encoded in gn. Andrew, I'm curious as to why you're in favor of this instead of changes on the gn side.
https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py#newcode889 tools/mb/mb.py:889: if android: On 2017/07/26 18:39:56, jbudorick wrote: > I'm concerned about hacking this into mb.py rather than having it encoded in gn. > Andrew, I'm curious as to why you're in favor of this instead of changes on the > gn side. In GN, "runtime_deps" are the things required to run an executable. I don't think unstripped .so files fall into this definition. This is more of a testing / swarming concern, so I actually think mb.py is a good spot for it.
On 2017/07/26 19:03:57, agrieve wrote: > https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py > File tools/mb/mb.py (right): > > https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py#newcode889 > tools/mb/mb.py:889: if android: > On 2017/07/26 18:39:56, jbudorick wrote: > > I'm concerned about hacking this into mb.py rather than having it encoded in > gn. > > Andrew, I'm curious as to why you're in favor of this instead of changes on > the > > gn side. > > In GN, "runtime_deps" are the things required to run an executable. I don't > think unstripped .so files fall into this definition. This is more of a testing > / swarming concern, so I actually think mb.py is a good spot for it. T
On 2017/07/26 19:03:57, agrieve wrote: > https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py > File tools/mb/mb.py (right): > > https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py#newcode889 > tools/mb/mb.py:889: if android: > On 2017/07/26 18:39:56, jbudorick wrote: > > I'm concerned about hacking this into mb.py rather than having it encoded in > gn. > > Andrew, I'm curious as to why you're in favor of this instead of changes on > the > > gn side. > > In GN, "runtime_deps" are the things required to run an executable. I don't > think unstripped .so files fall into this definition. This is more of a testing > / swarming concern, so I actually think mb.py is a good spot for it. It may not be required to run, but it is required to symbolize any failures while running... anyhow, if we don't want to do that, why not encode it as a runtime dependency of a separate symbolization target? You could also call test data a testing/swarming concern...
On Wed, Jul 26, 2017 at 3:09 PM, <jbudorick@chromium.org> wrote: > On 2017/07/26 19:03:57, agrieve wrote: > > https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py > > File tools/mb/mb.py (right): > > > > > https://codereview.chromium.org/2974163002/diff/220001/ > tools/mb/mb.py#newcode889 > > tools/mb/mb.py:889: if android: > > On 2017/07/26 18:39:56, jbudorick wrote: > > > I'm concerned about hacking this into mb.py rather than having it > encoded in > > gn. > > > Andrew, I'm curious as to why you're in favor of this instead of > changes on > > the > > > gn side. > > > > In GN, "runtime_deps" are the things required to run an executable. I > don't > > think unstripped .so files fall into this definition. This is more of a > testing > > / swarming concern, so I actually think mb.py is a good spot for it. > > It may not be required to run, but it is required to symbolize any failures > while running... > > anyhow, if we don't want to do that, why not encode it as a runtime > dependency > of a separate symbolization target? > Because is_component_build makes it impossible to know all of the .so files you depend on to list them out. > You could also call test data a testing/swarming concern... > You need the test data to run the tests, you don't need the unstripped libs to do so. That said, you generally don't need the stripped .so files either, when they are embedded in an apk, so it's a weak argument. There are three spots that I know of where we use runtime_deps during build: * pushing testdata to device * create_native_executable_dist() * android_apk() testdata already filters out .so files, and we could add filtering logic to the other two, but I think it would be more conservative to be adding these files in where needed rather than filtering them out (and have them accidentally bloat something at some point) I think either way will work, and I don't think there's a super strong argument for either case :/ > > https://codereview.chromium.org/2974163002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/07/26 19:09:18, jbudorick wrote: > On 2017/07/26 19:03:57, agrieve wrote: > > https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py > > File tools/mb/mb.py (right): > > > > > https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py#newcode889 > > tools/mb/mb.py:889: if android: > > On 2017/07/26 18:39:56, jbudorick wrote: > > > I'm concerned about hacking this into mb.py rather than having it encoded in > > gn. > > > Andrew, I'm curious as to why you're in favor of this instead of changes on > > the > > > gn side. > > > > In GN, "runtime_deps" are the things required to run an executable. I don't > > think unstripped .so files fall into this definition. This is more of a > testing > > / swarming concern, so I actually think mb.py is a good spot for it. > > It may not be required to run, but it is required to symbolize any failures > while running... > > anyhow, if we don't want to do that, why not encode it as a runtime dependency > of a separate symbolization target? > > You could also call test data a testing/swarming concern... Is it possible that we land this functional, but not perfect cl first so that people can symbolize stacktraces and let me create a new bug to clean up/rewrite in gn the 9 lines of code in mb.py? I can do that first thing after next week's meeting.
On 2017/07/26 19:24:19, BigBossZhiling wrote: > On 2017/07/26 19:09:18, jbudorick wrote: > > On 2017/07/26 19:03:57, agrieve wrote: > > > https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py > > > File tools/mb/mb.py (right): > > > > > > > > > https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py#newcode889 > > > tools/mb/mb.py:889: if android: > > > On 2017/07/26 18:39:56, jbudorick wrote: > > > > I'm concerned about hacking this into mb.py rather than having it encoded > in > > > gn. > > > > Andrew, I'm curious as to why you're in favor of this instead of changes > on > > > the > > > > gn side. > > > > > > In GN, "runtime_deps" are the things required to run an executable. I don't > > > think unstripped .so files fall into this definition. This is more of a > > testing > > > / swarming concern, so I actually think mb.py is a good spot for it. > > > > It may not be required to run, but it is required to symbolize any failures > > while running... > > > > anyhow, if we don't want to do that, why not encode it as a runtime dependency > > of a separate symbolization target? > > > > You could also call test data a testing/swarming concern... > > Is it possible that we land this functional, but not perfect cl first so that > people can symbolize stacktraces and let me create a new bug to clean up/rewrite > in gn the 9 lines of code in mb.py? I can do that first thing after next week's > meeting. I suppose I'm ok with this for now. Please do file the bug though. lgtm
https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py#newcode890 tools/mb/mb.py:890: unstripped_libs = [] one caveat: please add a TODO here with the bug number.
The CQ bit was checked by hzl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2974163002/#ps240001 (title: "add todo and crbug; revert changes for intentional crash test case")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2974163002/diff/220001/tools/mb/mb.py#newcode890 tools/mb/mb.py:890: unstripped_libs = [] On 2017/07/26 21:55:53, jbudorick wrote: > one caveat: please add a TODO here with the bug number. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by hzl@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1501113169787120,
"parent_rev": "0a68c5935af717b8ad403600433949d2f7903bc0", "commit_rev":
"55fc2af69da5f96e351e5aea26cfe75b95cf1887"}
Message was sent while issue was closed.
Description was changed from ========== Fix the stack script issue when symbolizing tombstones. Include the *.so files in lib.unstripped and the *.so files that are in apks so that we have the right symbols when symbolizing tombstones. BUG=743268 ========== to ========== Fix the stack script issue when symbolizing tombstones. Include the *.so files in lib.unstripped and the *.so files that are in apks so that we have the right symbols when symbolizing tombstones. BUG=743268 Review-Url: https://codereview.chromium.org/2974163002 Cr-Commit-Position: refs/heads/master@{#489882} Committed: https://chromium.googlesource.com/chromium/src/+/55fc2af69da5f96e351e5aea26cf... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/55fc2af69da5f96e351e5aea26cf... |
