Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
401003003:
Stack trace support for libraries loaded directly from APKs (Closed)
|
Created:
6 years, 5 months ago by Anton Modified:
6 years, 5 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionIf the chrome library is loaded directly from inside the APK the android stack trace will give the on device name of the APK in the stack trace. Try to find the symbols in this case by mapping to the appropriate library name.
BUG=390618
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285009
Patch Set 1 #Patch Set 2 : Simplify GetCrazyLib #Patch Set 3 : Whitespace changes #
Total comments: 16
Patch Set 4 : Updates for Ross' review. #Patch Set 5 : Remove incidental whitespace change. #
Total comments: 2
Patch Set 6 : Refactor based on Ross' comments. #Patch Set 7 : Remove unnecessary lambda. #
Total comments: 4
Patch Set 8 : Fix typo in comment. #
Messages
Total messages: 15 (0 generated)
https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... File third_party/android_platform/development/scripts/symbol.py (right): https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:163: return None return apk_path here for allowing false positives and avoid the "if not aapt:" on L223 https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:168: m = package_name_re.match(line) nit - more descriptive variable names than "m" and "z" please (here and below). https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:192: return candidate_apks Could you put MapDeviceApkToLibrary after L274 and reuse the candidate_dirs? Also maybe invert the process, doing the filtering of sorted candidate_apks in TranslateLibPath and passing this candidate set to MapDeviceApkToLibrary? If so, pull out both the candidate_libraries and candidate_apks filtering to a helper function. https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:216: a list of APK filenames which could contain the desired library. /s/a/A https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:229: package_name == GetPackageName(aapt, candidate_apk), indent (if you can) https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:243: crazy = GetCrazyLib(matching_apk) nit - crazy_lib https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:256: if re.search('-[0-9]+[.]apk$', library_name): Add a comment stating what you are doing here
https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... File third_party/android_platform/development/scripts/symbol.py (right): https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:163: return None On 2014/07/21 16:05:31, rmcilroy wrote: > return apk_path here for allowing false positives and avoid the "if not aapt:" > on L223 apk_path is not the same as the package_name so that does not work. https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:168: m = package_name_re.match(line) On 2014/07/21 16:05:30, rmcilroy wrote: > nit - more descriptive variable names than "m" and "z" please (here and below). Done. https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:192: return candidate_apks On 2014/07/21 16:05:30, rmcilroy wrote: > Could you put MapDeviceApkToLibrary after L274 and reuse the candidate_dirs? The candidate_dirs at L274 are for "libs" at 184 they are for "apks" > Also maybe invert the process, doing the filtering of sorted candidate_apks in > TranslateLibPath and passing this candidate set to MapDeviceApkToLibrary? If > so, pull out both the candidate_libraries and candidate_apks filtering to a > helper function. I don't quite follow what you are suggesting here, but I think you want to try and factor something out, but there isn't much in common. It looks possible to factor filter and sort, but seems contrived and is already succinct. Another possible factor is lists of os.path.join. The other possibility is to change the existing code to use glob, then I think it factors, but it may change the behavior a bit and I would rather existing behavior was preserved. https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:216: a list of APK filenames which could contain the desired library. On 2014/07/21 16:05:32, rmcilroy wrote: > /s/a/A Done. https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:229: package_name == GetPackageName(aapt, candidate_apk), On 2014/07/21 16:05:30, rmcilroy wrote: > indent (if you can) Done. https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:243: crazy = GetCrazyLib(matching_apk) On 2014/07/21 16:05:32, rmcilroy wrote: > nit - crazy_lib Done. https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:256: if re.search('-[0-9]+[.]apk$', library_name): On 2014/07/21 16:05:32, rmcilroy wrote: > Add a comment stating what you are doing here Done.
https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... File third_party/android_platform/development/scripts/symbol.py (right): https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:163: return None On 2014/07/22 13:54:03, anton1 wrote: > On 2014/07/21 16:05:31, rmcilroy wrote: > > return apk_path here for allowing false positives and avoid the "if not aapt:" > > on L223 > > apk_path is not the same as the package_name so that does not work. Ahh right. How about instead of GetPackageName you have a function "ApkMatchesPackageName(aapt, candidate_apk, package_name)", which just returns true if "aapt" is None - I think this would simplify the code in GetMatchingApks. https://codereview.chromium.org/401003003/diff/40001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:192: return candidate_apks On 2014/07/22 13:54:03, anton1 wrote: > On 2014/07/21 16:05:30, rmcilroy wrote: > > Could you put MapDeviceApkToLibrary after L274 and reuse the candidate_dirs? > > The candidate_dirs at L274 are for "libs" at 184 they are for "apks" > > > Also maybe invert the process, doing the filtering of sorted candidate_apks in > > TranslateLibPath and passing this candidate set to MapDeviceApkToLibrary? If > > so, pull out both the candidate_libraries and candidate_apks filtering to a > > helper function. > > I don't quite follow what you are suggesting here, but I think you want to try > and factor something out, but there isn't much in common. It looks possible to > factor filter and sort, but seems contrived and is already succinct. Another > possible factor is lists of os.path.join. > > The other possibility is to change the existing code to use glob, then I think > it factors, but it may change the behavior a bit and I would rather existing > behavior was preserved. As discussed offline, you could factor this out to a helper function which takes a list of subdirectories to search (e.g., ['apks'] here and ['lib', 'lib.target'] below, and a lambda to do the filtering. https://codereview.chromium.org/401003003/diff/80001/third_party/android_plat... File third_party/android_platform/development/scripts/symbol.py (right): https://codereview.chromium.org/401003003/diff/80001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:203: zipf = zipfile.ZipFile(apk_filename, 'r') nit - zip_file
PTAL https://codereview.chromium.org/401003003/diff/80001/third_party/android_plat... File third_party/android_platform/development/scripts/symbol.py (right): https://codereview.chromium.org/401003003/diff/80001/third_party/android_plat... third_party/android_platform/development/scripts/symbol.py:203: zipf = zipfile.ZipFile(apk_filename, 'r') On 2014/07/22 14:22:31, rmcilroy wrote: > nit - zip_file Done.
lgtm assuming CHROME_SYMBOLS_DIR and '.' changes are intended. https://codereview.chromium.org/401003003/diff/120001/third_party/android_pla... File third_party/android_platform/development/scripts/symbol.py (right): https://codereview.chromium.org/401003003/diff/120001/third_party/android_pla... third_party/android_platform/development/scripts/symbol.py:153: """Returns true the APK's package name is matches package_name. drop "is" https://codereview.chromium.org/401003003/diff/120001/third_party/android_pla... third_party/android_platform/development/scripts/symbol.py:208: candidates = PathListJoin([out_dir], buildtype_list) + [CHROME_SYMBOLS_DIR] Is the [CHROME_SYMBOLS_DIR] meant to be here? The original candidate selection also includes '.', which doesn't seem to be retained here.
https://codereview.chromium.org/401003003/diff/120001/third_party/android_pla... File third_party/android_platform/development/scripts/symbol.py (right): https://codereview.chromium.org/401003003/diff/120001/third_party/android_pla... third_party/android_platform/development/scripts/symbol.py:153: """Returns true the APK's package name is matches package_name. On 2014/07/23 11:38:06, rmcilroy wrote: > drop "is" Done. https://codereview.chromium.org/401003003/diff/120001/third_party/android_pla... third_party/android_platform/development/scripts/symbol.py:208: candidates = PathListJoin([out_dir], buildtype_list) + [CHROME_SYMBOLS_DIR] On 2014/07/23 11:38:06, rmcilroy wrote: > Is the [CHROME_SYMBOLS_DIR] meant to be here? The original candidate selection > also includes '.', which doesn't seem to be retained here. The '.' was in a string of the form: '<CHROME_SYMBOLS_DIR>/./<suffix>'. I removed the unnecessary '.' to give: '<CHROME_SYMBOLS_DIR>/<suffix>' which is a less contrived way of referring to the same thing.
The CQ bit was checked by anton@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anton@chromium.org/401003003/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by anton@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anton@chromium.org/401003003/140001
Message was sent while issue was closed.
Change committed as 285009 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
