Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2014693003:
Includes GMS version in crash report. (Closed)
|
Created:
4 years, 7 months ago by BigBossZhiling Modified:
4 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, sadrul, kalyank Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIncludes GMS version in crash report.
We saw crashes happen that vary between GMS, but GMS info is not
recorded by chrome crash yet. In this cl, we will add GMS version code
to crash report.
BUG=613618
Committed: https://crrev.com/e36a130a8f123cec0b941f71284436c86bbfe659
Cr-Commit-Position: refs/heads/master@{#399224}
Patch Set 1 #
Total comments: 9
Patch Set 2 : changed variable name; remove empty message #
Total comments: 5
Patch Set 3 : fixes #
Messages
Total messages: 34 (13 generated)
hzl@google.com changed reviewers: + dgn@chromium.org
hzl@google.com changed reviewers: + mark@chromium.org
https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/BuildInfo.java (right): https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/BuildInfo.java:73: PackageInfo pi = pm.getPackageInfo("com.google.android.gms", 0); use GoogleApiAvailability#GOOGLE_PLAY_SERVICES_PACKAGE for the name: https://developers.google.com/android/reference/com/google/android/gms/common... https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/BuildInfo.java:73: PackageInfo pi = pm.getPackageInfo("com.google.android.gms", 0); nit: more descriptive variable names: packageManager/ packageInfo instead of pm/pi https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/BuildInfo.java:74: msg = ""; Either remove this (the string at line 70 would be used), or use another error message, instead of an empty string.
https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/BuildInfo.java (right): https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/BuildInfo.java:73: PackageInfo pi = pm.getPackageInfo("com.google.android.gms", 0); On 2016/05/26 09:29:10, dgn wrote: > use GoogleApiAvailability#GOOGLE_PLAY_SERVICES_PACKAGE for the name: > > https://developers.google.com/android/reference/com/google/android/gms/common... I have trouble importing GoogleApiAvailability in this file. I tried to do "import com.google.android.gms.common.GoogleApiAvailability;" https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/BuildInfo.java:73: PackageInfo pi = pm.getPackageInfo("com.google.android.gms", 0); On 2016/05/26 09:29:10, dgn wrote: > nit: more descriptive variable names: packageManager/ packageInfo instead of > pm/pi Done. https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/BuildInfo.java:74: msg = ""; On 2016/05/26 09:29:10, dgn wrote: > Either remove this (the string at line 70 would be used), or use another error > message, instead of an empty string. Done.
https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/BuildInfo.java (right): https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/BuildInfo.java:73: PackageInfo pi = pm.getPackageInfo("com.google.android.gms", 0); On 2016/05/26 18:35:37, BigBossZhiling wrote: > On 2016/05/26 09:29:10, dgn wrote: > > use GoogleApiAvailability#GOOGLE_PLAY_SERVICES_PACKAGE for the name: > > > > > https://developers.google.com/android/reference/com/google/android/gms/common... > > I have trouble importing GoogleApiAvailability in this file. I tried to do > "import com.google.android.gms.common.GoogleApiAvailability;" It should be because base does not depend on play services currently. base/BUILD.gn (https://code.google.com/p/chromium/codesearch#chromium/src/base/BUILD.gn) has some references to things in third_party/android_tools already. You would need to add google_play_services_libraries (https://code.google.com/p/chromium/codesearch#chromium/src/build/config/andro...) as dependency of your target
Description was changed from ========== Includes GMS version in crash report. We saw crashes happen that vary between GMS, but GMS info is not recorded by chrome crash yet. In this cl, we will add GMS version code to crash report. BUG=613618 ========== to ========== Includes GMS version in crash report. We saw crashes happen that vary between GMS, but GMS info is not recorded by chrome crash yet. In this cl, we will add GMS version code to crash report. BUG=613618 ==========
hzl@google.com changed reviewers: + yfriedman@chromium.org
https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/BuildInfo.java (right): https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/BuildInfo.java:73: PackageInfo pi = pm.getPackageInfo("com.google.android.gms", 0); On 2016/05/26 19:20:09, dgn wrote: > On 2016/05/26 18:35:37, BigBossZhiling wrote: > > On 2016/05/26 09:29:10, dgn wrote: > > > use GoogleApiAvailability#GOOGLE_PLAY_SERVICES_PACKAGE for the name: > > > > > > > > > https://developers.google.com/android/reference/com/google/android/gms/common... > > > > I have trouble importing GoogleApiAvailability in this file. I tried to do > > "import com.google.android.gms.common.GoogleApiAvailability;" > > > It should be because base does not depend on play services currently. > base/BUILD.gn > (https://code.google.com/p/chromium/codesearch#chromium/src/base/BUILD.gn) has > some references to things in third_party/android_tools already. You would need > to add google_play_services_libraries > (https://code.google.com/p/chromium/codesearch#chromium/src/build/config/andro...) > as dependency of your target I don't think this is sufficient reason to make base depend on play services. TBH, it's highly unlikely and improbable for gms core to change its package name at this point. Since this is for reporting only, I think we can risk getting it wrong (of course coding defensively and I think that's covered), and hardcode it. base gets pulled into lots of things (including any test target) so we should keep it minimal
On 2016/05/26 20:50:08, Yaron (OOO until June 27) wrote: > https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... > File base/android/java/src/org/chromium/base/BuildInfo.java (right): > > https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... > base/android/java/src/org/chromium/base/BuildInfo.java:73: PackageInfo pi = > pm.getPackageInfo("com.google.android.gms", 0); > On 2016/05/26 19:20:09, dgn wrote: > > On 2016/05/26 18:35:37, BigBossZhiling wrote: > > > On 2016/05/26 09:29:10, dgn wrote: > > > > use GoogleApiAvailability#GOOGLE_PLAY_SERVICES_PACKAGE for the name: > > > > > > > > > > > > > > https://developers.google.com/android/reference/com/google/android/gms/common... > > > > > > I have trouble importing GoogleApiAvailability in this file. I tried to do > > > "import com.google.android.gms.common.GoogleApiAvailability;" > > > > > > It should be because base does not depend on play services currently. > > base/BUILD.gn > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/BUILD.gn) has > > some references to things in third_party/android_tools already. You would need > > to add google_play_services_libraries > > > (https://code.google.com/p/chromium/codesearch#chromium/src/build/config/andro...) > > as dependency of your target > > I don't think this is sufficient reason to make base depend on play services. > TBH, it's highly unlikely and improbable for gms core to change its package name > at this point. Since this is for reporting only, I think we can risk getting it > wrong (of course coding defensively and I think that's covered), and hardcode > it. > > base gets pulled into lots of things (including any test target) so we should > keep it minimal Another hacky option would be to just add play services to the classpath for base_java but not as a dep. The idea I'm going for is that when base is compiled, the symbol will be present but the code won't be packaged into whatever base includes. It's kinda hacky, I'm not sure how to do it in GN currently but it's possible (agrieve would know,) and you'd need to make sure we don't crash if play services wasn't present at runtime. I don't think it's a great solution but mention it for completeness
On 2016/05/26 21:01:21, Yaron (OOO until June 27) wrote: > On 2016/05/26 20:50:08, Yaron (OOO until June 27) wrote: > > > https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... > > File base/android/java/src/org/chromium/base/BuildInfo.java (right): > > > > > https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... > > base/android/java/src/org/chromium/base/BuildInfo.java:73: PackageInfo pi = > > pm.getPackageInfo("com.google.android.gms", 0); > > On 2016/05/26 19:20:09, dgn wrote: > > > On 2016/05/26 18:35:37, BigBossZhiling wrote: > > > > On 2016/05/26 09:29:10, dgn wrote: > > > > > use GoogleApiAvailability#GOOGLE_PLAY_SERVICES_PACKAGE for the name: > > > > > > > > > > > > > > > > > > > > https://developers.google.com/android/reference/com/google/android/gms/common... > > > > > > > > I have trouble importing GoogleApiAvailability in this file. I tried to do > > > > "import com.google.android.gms.common.GoogleApiAvailability;" > > > > > > > > > It should be because base does not depend on play services currently. > > > base/BUILD.gn > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/BUILD.gn) > has > > > some references to things in third_party/android_tools already. You would > need > > > to add google_play_services_libraries > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/build/config/andro...) > > > as dependency of your target > > > > I don't think this is sufficient reason to make base depend on play services. > > TBH, it's highly unlikely and improbable for gms core to change its package > name > > at this point. Since this is for reporting only, I think we can risk getting > it > > wrong (of course coding defensively and I think that's covered), and hardcode > > it. > > > > base gets pulled into lots of things (including any test target) so we should > > keep it minimal > > Another hacky option would be to just add play services to the classpath for > base_java but not as a dep. The idea I'm going for is that when base is > compiled, the symbol will be present but the code won't be packaged into > whatever base includes. It's kinda hacky, I'm not sure how to do it in GN > currently but it's possible (agrieve would know,) and you'd need to make sure we > don't crash if play services wasn't present at runtime. I don't think it's a > great solution but mention it for completeness Oh actually with GN this is just the default behaviour so it could work just by adding a base_java dep but I'm still not sure this is warranted
https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/BuildInfo.java (right): https://codereview.chromium.org/2014693003/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/BuildInfo.java:73: PackageInfo pi = pm.getPackageInfo("com.google.android.gms", 0); On 2016/05/26 20:50:08, Yaron (OOO until June 27) wrote: > On 2016/05/26 19:20:09, dgn wrote: > > On 2016/05/26 18:35:37, BigBossZhiling wrote: > > > On 2016/05/26 09:29:10, dgn wrote: > > > > use GoogleApiAvailability#GOOGLE_PLAY_SERVICES_PACKAGE for the name: > > > > > > > > > > > > > > https://developers.google.com/android/reference/com/google/android/gms/common... > > > > > > I have trouble importing GoogleApiAvailability in this file. I tried to do > > > "import com.google.android.gms.common.GoogleApiAvailability;" > > > > > > It should be because base does not depend on play services currently. > > base/BUILD.gn > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/BUILD.gn) has > > some references to things in third_party/android_tools already. You would need > > to add google_play_services_libraries > > > (https://code.google.com/p/chromium/codesearch#chromium/src/build/config/andro...) > > as dependency of your target > > I don't think this is sufficient reason to make base depend on play services. > TBH, it's highly unlikely and improbable for gms core to change its package name > at this point. Since this is for reporting only, I think we can risk getting it > wrong (of course coding defensively and I think that's covered), and hardcode > it. > > base gets pulled into lots of things (including any test target) so we should > keep it minimal Ok sounds good. I agree I don't think it's going to change anytime soon anyway. https://codereview.chromium.org/2014693003/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2014693003/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:606: __android_log_write(ANDROID_LOG_WARN, kGoogleBreakpad, The output is going to be something like: Chrome build fingerprint: com.google.chrome 52.2542.0.42 845000 How about having a new line for "Google Play services app version:"?
hzl@google.com changed reviewers: + torne@chromium.org - mark@chromium.org, yfriedman@chromium.org
hzl@google.com changed reviewers: + rsesek@chromium.org
https://codereview.chromium.org/2014693003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/BuildInfo.java (right): https://codereview.chromium.org/2014693003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/BuildInfo.java:74: if (packageInfo.versionCode > 0) { Seems like there isn't much point in doing this; we may as well just log whatever value the field has? https://codereview.chromium.org/2014693003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/BuildInfo.java:78: Log.d(TAG, "Error when trying to get gms version code: %s", e); I'd probably just have this explicitly set msg to something that says the package wasn't found - if you did that and also just logged the version code unconditionally there'd be no need for a generic "version code not available" message any more. This will only happen on devices that lack gmscore (where chrome was presumably sideloaded and probably can't run anyway), or during the middle of a gmscore update.
https://codereview.chromium.org/2014693003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/BuildInfo.java (right): https://codereview.chromium.org/2014693003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/BuildInfo.java:74: if (packageInfo.versionCode > 0) { On 2016/06/03 14:10:10, Torne wrote: > Seems like there isn't much point in doing this; we may as well just log > whatever value the field has? Done. https://codereview.chromium.org/2014693003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/BuildInfo.java:78: Log.d(TAG, "Error when trying to get gms version code: %s", e); On 2016/06/03 14:10:10, Torne wrote: > I'd probably just have this explicitly set msg to something that says the > package wasn't found - if you did that and also just logged the version code > unconditionally there'd be no need for a generic "version code not available" > message any more. This will only happen on devices that lack gmscore (where > chrome was presumably sideloaded and probably can't run anyway), or during the > middle of a gmscore update. Done.
lgtm
The CQ bit was checked by hzl@google.com
The CQ bit was unchecked by hzl@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2014693003/40001
lgtm
The CQ bit was checked by hzl@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2014693003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by hzl@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2014693003/40001
Message was sent while issue was closed.
Description was changed from ========== Includes GMS version in crash report. We saw crashes happen that vary between GMS, but GMS info is not recorded by chrome crash yet. In this cl, we will add GMS version code to crash report. BUG=613618 ========== to ========== Includes GMS version in crash report. We saw crashes happen that vary between GMS, but GMS info is not recorded by chrome crash yet. In this cl, we will add GMS version code to crash report. BUG=613618 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Includes GMS version in crash report. We saw crashes happen that vary between GMS, but GMS info is not recorded by chrome crash yet. In this cl, we will add GMS version code to crash report. BUG=613618 ========== to ========== Includes GMS version in crash report. We saw crashes happen that vary between GMS, but GMS info is not recorded by chrome crash yet. In this cl, we will add GMS version code to crash report. BUG=613618 Committed: https://crrev.com/e36a130a8f123cec0b941f71284436c86bbfe659 Cr-Commit-Position: refs/heads/master@{#399224} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e36a130a8f123cec0b941f71284436c86bbfe659 Cr-Commit-Position: refs/heads/master@{#399224} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
