Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2713843003:
[Chromecast] Don't override RegisterCrashKeys() on Android. (Closed)
Description[Chromecast] Don't override RegisterCrashKeys() on Android.
We don't override this method in CastCrashReporterClient, and
every time someone upstream adds keys to the registry, we have
to copy-paste those keys to cast_crash_keys just so the render
process doesn't fail a DCHECK on initialization. This is
annoying and no progress has been made on sanitizing the crash
key registration recently, so why even bother keeping this if we
don't do it on Chromecast?
When we figure out how to register crash keys stably on
Chromecast, we can do so in a way that works for all platforms.
BUG=598854
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove RegisterCastCrashKeys() #
Messages
Total messages: 10 (2 generated)
sanfin@chromium.org changed reviewers: + halliwell@chromium.org, slan@chromium.org
Hi, guys, Casting to Android builds off our master branch currently fails because some crash keys upstream have changed yet again. I'm getting sick of updating cast_crash_keys.cc every time this happens, and I don't see any evidence that we currently make use of the crash key functionality (as I note in the commit message, CastCrashReporterClient doesn't override RegisterCrashKeys). It seems to me that the best path forward is to break the Android dependency on RegisterCastCrashKeys() until we have a plan to use crash keys on Chromecast, whereupon we can make sure we use the same logic on all platforms. Right now, keeping this here seems to be providing negative value.
https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... File chromecast/crash/cast_crash_keys.cc (right): https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... chromecast/crash/cast_crash_keys.cc:42: { kPreviousApp, kSmallSize }, I don't understand. It seems that RegisterCastCrashKeys isn't called from _anywhere_ now? So why not just delete the function? I also thought that we needed to register keys, because of the DCHECK that they're registered before being set (see https://codereview.chromium.org/25572002 )? Does this CL cause that DCHECK to fail? Also, why don't we need to call it on linux? Is it because we use our own crash handling code - slan?
https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... File chromecast/crash/cast_crash_keys.cc (right): https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... chromecast/crash/cast_crash_keys.cc:42: { kPreviousApp, kSmallSize }, On 2017/02/24 14:42:40, halliwell wrote: > I don't understand. It seems that RegisterCastCrashKeys isn't called from > _anywhere_ now? So why not just delete the function? > > I also thought that we needed to register keys, because of the DCHECK that > they're registered before being set (see > https://codereview.chromium.org/25572002 )? Does this CL cause that DCHECK to > fail? > > Also, why don't we need to call it on linux? Is it because we use our own crash > handling code - slan? I could just delete the function and leave a TODO. But as long as we don't call base::debug::InitCrashKeys(), any subsequent call to base::debug::SetCrashKeyValue() will no-op. That's why Android, which called RegisterCastCrashKeys(), would crash when new keys were added upstream, while Linux, which did not call RegisterCastCrashKeys(), would not crash.
On 2017/02/24 19:10:50, Simeon wrote: > https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... > File chromecast/crash/cast_crash_keys.cc (right): > > https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... > chromecast/crash/cast_crash_keys.cc:42: { kPreviousApp, kSmallSize }, > On 2017/02/24 14:42:40, halliwell wrote: > > I don't understand. It seems that RegisterCastCrashKeys isn't called from > > _anywhere_ now? So why not just delete the function? > > > > I also thought that we needed to register keys, because of the DCHECK that > > they're registered before being set (see > > https://codereview.chromium.org/25572002 )? Does this CL cause that DCHECK to > > fail? > > > > Also, why don't we need to call it on linux? Is it because we use our own > crash > > handling code - slan? > > I could just delete the function and leave a TODO. But as long as we don't call > base::debug::InitCrashKeys(), any subsequent call to > base::debug::SetCrashKeyValue() will no-op. That's why Android, which called > RegisterCastCrashKeys(), would crash when new keys were added upstream, while > Linux, which did not call RegisterCastCrashKeys(), would not crash. slan: you've spent more time than most in the crash code, any thoughts?
On 2017/02/25 01:04:13, halliwell wrote: > On 2017/02/24 19:10:50, Simeon wrote: > > > https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... > > File chromecast/crash/cast_crash_keys.cc (right): > > > > > https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... > > chromecast/crash/cast_crash_keys.cc:42: { kPreviousApp, kSmallSize }, > > On 2017/02/24 14:42:40, halliwell wrote: > > > I don't understand. It seems that RegisterCastCrashKeys isn't called from > > > _anywhere_ now? So why not just delete the function? > > > > > > I also thought that we needed to register keys, because of the DCHECK that > > > they're registered before being set (see > > > https://codereview.chromium.org/25572002 )? Does this CL cause that DCHECK > to > > > fail? > > > > > > Also, why don't we need to call it on linux? Is it because we use our own > > crash > > > handling code - slan? > > > > I could just delete the function and leave a TODO. But as long as we don't > call > > base::debug::InitCrashKeys(), any subsequent call to > > base::debug::SetCrashKeyValue() will no-op. That's why Android, which called > > RegisterCastCrashKeys(), would crash when new keys were added upstream, while > > Linux, which did not call RegisterCastCrashKeys(), would not crash. > > slan: you've spent more time than most in the crash code, any thoughts? My gut tells me that removing these isn't necessarily the best way to go. Simeon, where is the failing DCHECK? Could we capture that behavior in a unittest that runs on cast_shell_android? That way, the Chromium developer changing the chrome crash keys is compelled to update ours also?
On 2017/02/25 02:02:50, slan wrote: > On 2017/02/25 01:04:13, halliwell wrote: > > On 2017/02/24 19:10:50, Simeon wrote: > > > > > > https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... > > > File chromecast/crash/cast_crash_keys.cc (right): > > > > > > > > > https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... > > > chromecast/crash/cast_crash_keys.cc:42: { kPreviousApp, kSmallSize }, > > > On 2017/02/24 14:42:40, halliwell wrote: > > > > I don't understand. It seems that RegisterCastCrashKeys isn't called from > > > > _anywhere_ now? So why not just delete the function? > > > > > > > > I also thought that we needed to register keys, because of the DCHECK that > > > > they're registered before being set (see > > > > https://codereview.chromium.org/25572002 )? Does this CL cause that > DCHECK > > to > > > > fail? > > > > > > > > Also, why don't we need to call it on linux? Is it because we use our own > > > crash > > > > handling code - slan? > > > > > > I could just delete the function and leave a TODO. But as long as we don't > > call > > > base::debug::InitCrashKeys(), any subsequent call to > > > base::debug::SetCrashKeyValue() will no-op. That's why Android, which called > > > RegisterCastCrashKeys(), would crash when new keys were added upstream, > while > > > Linux, which did not call RegisterCastCrashKeys(), would not crash. > > > > slan: you've spent more time than most in the crash code, any thoughts? > > My gut tells me that removing these isn't necessarily the best way to go. > Simeon, where is the failing DCHECK? Could we capture that behavior in a > unittest that runs on cast_shell_android? That way, the Chromium developer > changing the chrome crash keys is compelled to update ours also? The DCHECK failure is in https://cs.chromium.org/chromium/src/base/debug/crash_logging.cc?type=cs&l=59. Notice that if g_crash_keys_ is not set, then the function returns early as a no-op. The problem is really that the whole crash key architecture is fundamentally flawed, with registration happening in //chrome and clients in everywhere from //content to //media to //ui. This essentially creates a hard dependency of the client packages on what should be a leaf package with no antidependencies. It would be hard to test that everything registered in //chrome/common/crash_keys.cc also appears in //chromecast/crash/cast_crash_keys.cc. Such a unittest would have to declare a dependency on //chrome:chrome_initial and //chromecast/crash. The most viable alternative would be to factor out //chrome/common/crash_keys.cc into //content or something, but even that wouldn't work perfectly because there's no way to test that all parts of the code base use valid keys without getting 100% unittest coverage in a test case that makes sure to register the keys. Really, the only solution that seems viable to me is to implement http://crbug.com/598854 and have each module be responsible for its own keys, similar to how we register JNI functions, and each module that uses crash keys must first call a RegisterCrashKeys() function associated with that module. Then we wouldn't have to worry about upstream code's crash keys unless we ourselves violate ownership rules and set values for crash keys that other modules own.
On 2017/02/25 03:29:14, Simeon wrote: > On 2017/02/25 02:02:50, slan wrote: > > On 2017/02/25 01:04:13, halliwell wrote: > > > On 2017/02/24 19:10:50, Simeon wrote: > > > > > > > > > > https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... > > > > File chromecast/crash/cast_crash_keys.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2713843003/diff/1/chromecast/crash/cast_crash... > > > > chromecast/crash/cast_crash_keys.cc:42: { kPreviousApp, kSmallSize }, > > > > On 2017/02/24 14:42:40, halliwell wrote: > > > > > I don't understand. It seems that RegisterCastCrashKeys isn't called > from > > > > > _anywhere_ now? So why not just delete the function? > > > > > > > > > > I also thought that we needed to register keys, because of the DCHECK > that > > > > > they're registered before being set (see > > > > > https://codereview.chromium.org/25572002 )? Does this CL cause that > > DCHECK > > > to > > > > > fail? > > > > > > > > > > Also, why don't we need to call it on linux? Is it because we use our > own > > > > crash > > > > > handling code - slan? > > > > > > > > I could just delete the function and leave a TODO. But as long as we don't > > > call > > > > base::debug::InitCrashKeys(), any subsequent call to > > > > base::debug::SetCrashKeyValue() will no-op. That's why Android, which > called > > > > RegisterCastCrashKeys(), would crash when new keys were added upstream, > > while > > > > Linux, which did not call RegisterCastCrashKeys(), would not crash. > > > > > > slan: you've spent more time than most in the crash code, any thoughts? > > > > My gut tells me that removing these isn't necessarily the best way to go. > > Simeon, where is the failing DCHECK? Could we capture that behavior in a > > unittest that runs on cast_shell_android? That way, the Chromium developer > > changing the chrome crash keys is compelled to update ours also? > > The DCHECK failure is in > https://cs.chromium.org/chromium/src/base/debug/crash_logging.cc?type=cs&l=59. > Notice that if g_crash_keys_ is not set, then the function returns early as a > no-op. > > The problem is really that the whole crash key architecture is fundamentally > flawed, with registration happening in //chrome and clients in everywhere from > //content to //media to //ui. This essentially creates a hard dependency of the > client packages on what should be a leaf package with no antidependencies. > > It would be hard to test that everything registered in > //chrome/common/crash_keys.cc also appears in > //chromecast/crash/cast_crash_keys.cc. Such a unittest would have to declare a > dependency on //chrome:chrome_initial and //chromecast/crash. The most viable > alternative would be to factor out //chrome/common/crash_keys.cc into //content > or something, but even that wouldn't work perfectly because there's no way to > test that all parts of the code base use valid keys without getting 100% > unittest coverage in a test case that makes sure to register the keys. > > Really, the only solution that seems viable to me is to implement > http://crbug.com/598854 and have each module be responsible for its own keys, > similar to how we register JNI functions, and each module that uses crash keys > must first call a RegisterCrashKeys() function associated with that module. Then > we wouldn't have to worry about upstream code's crash keys unless we ourselves > violate ownership rules and set values for crash keys that other modules own. Ok, I had a dig into this code ... to summarize conversation with Simeon for Stephen's benefit: * We don't use crash keys system on linux at all. Instead, we track the few keys we care about for our code (last app, previous app) in AppStateTracker and read them directly from there when uploading minidumps. This works because we have our own minidump upload code on linux. * We don't have our own codepath on ATV, so this change will lose our crash keys on reports for ATV. We verified that we currently get last, current app and other Chromium keys in our crash reports. Therefore, we shouldn't land this change. I think the real solution is to: 1. Add the specific keys that are causing this latest failure, as a short term fix to get casting working again on master. 2. We should try to resurrect and land something along the lines of this CL: https://codereview.chromium.org/1849413003/
Yes, I agree with this approach. This fits the convention used by Chrome for allowing customization from various components or clients. SGTM, thanks for the research, Luke and Simeon. On Mon, Feb 27, 2017 at 11:10 AM, <halliwell@chromium.org> wrote: > On 2017/02/25 03:29:14, Simeon wrote: > > On 2017/02/25 02:02:50, slan wrote: > > > On 2017/02/25 01:04:13, halliwell wrote: > > > > On 2017/02/24 19:10:50, Simeon wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2713843003/diff/1/ > chromecast/crash/cast_crash_keys.cc > > > > > File chromecast/crash/cast_crash_keys.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2713843003/diff/1/ > chromecast/crash/cast_crash_keys.cc#newcode42 > > > > > chromecast/crash/cast_crash_keys.cc:42: { kPreviousApp, > kSmallSize }, > > > > > On 2017/02/24 14:42:40, halliwell wrote: > > > > > > I don't understand. It seems that RegisterCastCrashKeys isn't > called > > from > > > > > > _anywhere_ now? So why not just delete the function? > > > > > > > > > > > > I also thought that we needed to register keys, because of the > DCHECK > > that > > > > > > they're registered before being set (see > > > > > > https://codereview.chromium.org/25572002 )? Does this CL cause > that > > > DCHECK > > > > to > > > > > > fail? > > > > > > > > > > > > Also, why don't we need to call it on linux? Is it because we > use our > > own > > > > > crash > > > > > > handling code - slan? > > > > > > > > > > I could just delete the function and leave a TODO. But as long as > we > don't > > > > call > > > > > base::debug::InitCrashKeys(), any subsequent call to > > > > > base::debug::SetCrashKeyValue() will no-op. That's why Android, > which > > called > > > > > RegisterCastCrashKeys(), would crash when new keys were added > upstream, > > > while > > > > > Linux, which did not call RegisterCastCrashKeys(), would not crash. > > > > > > > > slan: you've spent more time than most in the crash code, any > thoughts? > > > > > > My gut tells me that removing these isn't necessarily the best way to > go. > > > Simeon, where is the failing DCHECK? Could we capture that behavior in > a > > > unittest that runs on cast_shell_android? That way, the Chromium > developer > > > changing the chrome crash keys is compelled to update ours also? > > > > The DCHECK failure is in > > https://cs.chromium.org/chromium/src/base/debug/crash_ > logging.cc?type=cs&l=59. > > Notice that if g_crash_keys_ is not set, then the function returns early > as a > > no-op. > > > > The problem is really that the whole crash key architecture is > fundamentally > > flawed, with registration happening in //chrome and clients in > everywhere from > > //content to //media to //ui. This essentially creates a hard dependency > of > the > > client packages on what should be a leaf package with no > antidependencies. > > > > It would be hard to test that everything registered in > > //chrome/common/crash_keys.cc also appears in > > //chromecast/crash/cast_crash_keys.cc. Such a unittest would have to > declare a > > dependency on //chrome:chrome_initial and //chromecast/crash. The most > viable > > alternative would be to factor out //chrome/common/crash_keys.cc into > //content > > or something, but even that wouldn't work perfectly because there's no > way to > > test that all parts of the code base use valid keys without getting 100% > > unittest coverage in a test case that makes sure to register the keys. > > > > Really, the only solution that seems viable to me is to implement > > http://crbug.com/598854 and have each module be responsible for its own > keys, > > similar to how we register JNI functions, and each module that uses > crash keys > > must first call a RegisterCrashKeys() function associated with that > module. > Then > > we wouldn't have to worry about upstream code's crash keys unless we > ourselves > > violate ownership rules and set values for crash keys that other modules > own. > > Ok, I had a dig into this code ... to summarize conversation with Simeon > for > Stephen's benefit: > * We don't use crash keys system on linux at all. Instead, we track the few > keys we care about for our code (last app, previous app) in > AppStateTracker and > read them directly from there when uploading minidumps. This works because > we > have our own minidump upload code on linux. > * We don't have our own codepath on ATV, so this change will lose our > crash keys > on reports for ATV. We verified that we currently get last, current app and > other Chromium keys in our crash reports. Therefore, we shouldn't land this > change. > > I think the real solution is to: > 1. Add the specific keys that are causing this latest failure, as a short > term > fix to get casting working again on master. > 2. We should try to resurrect and land something along the lines of this > CL: > https://codereview.chromium.org/1849413003/ > > https://codereview.chromium.org/2713843003/ > -- 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.
Description was changed from ========== [Chromecast] Don't override RegisterCrashKeys() on Android. We don't override this method in CastCrashReporterClient, and every time someone upstream adds keys to the registry, we have to copy-paste those keys to cast_crash_keys just so the render process doesn't fail a DCHECK on initialization. This is annoying and no progress has been made on sanitizing the crash key registration recently, so why even bother keeping this if we don't do it on Chromecast? When we figure out how to register crash keys stably on Chromecast, we can do so in a way that works for all platforms. BUG=598854 ========== to ========== [Chromecast] Don't override RegisterCrashKeys() on Android. We don't override this method in CastCrashReporterClient, and every time someone upstream adds keys to the registry, we have to copy-paste those keys to cast_crash_keys just so the render process doesn't fail a DCHECK on initialization. This is annoying and no progress has been made on sanitizing the crash key registration recently, so why even bother keeping this if we don't do it on Chromecast? When we figure out how to register crash keys stably on Chromecast, we can do so in a way that works for all platforms. BUG=598854 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
