Remove unused core dump handler code

[hlwarriner]

b/289214111

Change-Id: If822f5879010dd4235c49c01f01a43b287652d68

[hlwarriner]

@jellefoks @niranjanyardi, can you please take a look at https://b.corp.google.com/issues/289214111#comment2 and help me evaluate whether any of this core dump data is actually needed? If so, I think we'd want to add it back by 1) possibly extending the CrashHandler Starboard extension to take key/value pairs with Int or Float values, and 2) using the Starboard extension where we previously used SbCoreDumpLog{String, Integer, Float}.

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (7153cc9) 58.53% compared to head (f79d5ce) 58.57%.
Report is 23 commits behind head on main.

❗ Current head f79d5ce differs from pull request most recent head 591e1da. Consider uploading reports for the commit 591e1da to get more accurate results

Files	Patch %	Lines
cobalt/h5vcc/h5vcc_crash_log.cc	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2192      +/-   ##
==========================================
+ Coverage   58.53%   58.57%   +0.03%     
==========================================
  Files        1908     1909       +1     
  Lines       94585    94565      -20     
==========================================
+ Hits        55369    55393      +24     
+ Misses      39216    39172      -44     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jellefoks]

I agree that is appears to be dead code now.

[niranjanyardi]

@jellefoks @niranjanyardi, can you please take a look at b.corp.google.com/issues/289214111#comment2 and help me evaluate whether any of this core dump data is actually needed? If so, I think we'd want to add it back by 1) possibly extending the CrashHandler Starboard extension to take key/value pairs with Int or Float values, and 2) using the Starboard extension where we previously used SbCoreDumpLog{String, Integer, Float}.

thanks for the comment. lgtm , did you run tests on ps5 and ensure that passes ?

[joeltine]

Change LGTM. It is a tad scary removing this stuff because if we inadvertently break crash context logging, a lot of downstream stuff breaks horribly.

Can you just verify you've tested everything adequately and are confident PlayStation 5 still works and other platforms still work as expected? Thanks!

[hlwarriner]

@jellefoks @niranjanyardi, can you please take a look at b.corp.google.com/issues/289214111#comment2 and help me evaluate whether any of this core dump data is actually needed? If so, I think we'd want to add it back by 1) possibly extending the CrashHandler Starboard extension to take key/value pairs with Int or Float values, and 2) using the Starboard extension where we previously used SbCoreDumpLog{String, Integer, Float}.

thanks for the comment. lgtm , did you run tests on ps5 and ensure that passes ?

You're welcome and not yet - I think I'll need to use the corresponding Gerrit change to run try builds.

[hlwarriner]

Change LGTM. It is a tad scary removing this stuff because if we inadvertently break crash context logging, a lot of downstream stuff breaks horribly.

Can you just verify you've tested everything adequately and are confident PlayStation 5 still works and other platforms still work as expected? Thanks!

You're mostly worried about preserving the client context set by the web app via the setString web API, right? I was planning to test locally on a couple Evergreen platforms, like x64 and AOSP, just to be sure there's no regression. But ps5 is the platform that would be more likely to have a regression, I guess. @niranjanyardi, do we have a way to force a crash locally on ps5 and inspect the crash report?

[joeltine]

Change LGTM. It is a tad scary removing this stuff because if we inadvertently break crash context logging, a lot of downstream stuff breaks horribly.
Can you just verify you've tested everything adequately and are confident PlayStation 5 still works and other platforms still work as expected? Thanks!

You're mostly worried about preserving the client context set by the web app via the setString web API, right? I was planning to test locally on a couple Evergreen platforms, like x64 and AOSP, just to be sure there's no regression. But ps5 is the platform that would be more likely to have a regression, I guess. @niranjanyardi, do we have a way to force a crash locally on ps5 and inspect the crash report?

Ya. Ideally, we could test all of Apple, PS5, EG, and Android. I believe the YTS tests that Wei wrote recently should cover some of this, right?

[hlwarriner]

Change LGTM. It is a tad scary removing this stuff because if we inadvertently break crash context logging, a lot of downstream stuff breaks horribly.
Can you just verify you've tested everything adequately and are confident PlayStation 5 still works and other platforms still work as expected? Thanks!

You're mostly worried about preserving the client context set by the web app via the setString web API, right? I was planning to test locally on a couple Evergreen platforms, like x64 and AOSP, just to be sure there's no regression. But ps5 is the platform that would be more likely to have a regression, I guess. @niranjanyardi, do we have a way to force a crash locally on ps5 and inspect the crash report?

Ya. Ideally, we could test all of Apple, PS5, EG, and Android. I believe the YTS tests that Wei wrote recently should cover some of this, right?

Wei's cl/586080954 modified the existing verify the crash handler functionality test so that it now exercises the setString web API. The test is still somewhat incomplete in this respect though because we haven't added the verification of the data in the crash report (https://b.corp.google.com/issues/310979503#comment9). This behavior is easy enough to test end-to-end manually on Evergreen devices, so I'll probably just do that for x64 and AOSP, anyway. Also, note this YTS test is Evergreen specific, so can't help with other platforms.

For ps5, @niranjanyardi offered to help test this locally. Based on the discussion in b/249761235, I think we should be able to verify that the ps5 crash has the expected client context after we force a crash.

For Apple and Android, I can talk to platform owners and see how much effort it would be to validate that setString still works correctly. It would be very surprising if it didn't, since these changes are mostly scoped to platforms that define CORE_DUMP_HANDLER_SUPPORT, which I think used to be just Playstation and now is no platform, but we can at least look into testing it.

Longer term we should think about whether we can write end-to-end tests for setString for non-Evergreen platforms.

[hlwarriner]

Change LGTM. It is a tad scary removing this stuff because if we inadvertently break crash context logging, a lot of downstream stuff breaks horribly.
Can you just verify you've tested everything adequately and are confident PlayStation 5 still works and other platforms still work as expected? Thanks!

You're mostly worried about preserving the client context set by the web app via the setString web API, right? I was planning to test locally on a couple Evergreen platforms, like x64 and AOSP, just to be sure there's no regression. But ps5 is the platform that would be more likely to have a regression, I guess. @niranjanyardi, do we have a way to force a crash locally on ps5 and inspect the crash report?

Ya. Ideally, we could test all of Apple, PS5, EG, and Android. I believe the YTS tests that Wei wrote recently should cover some of this, right?

Wei's cl/586080954 modified the existing verify the crash handler functionality test so that it now exercises the setString web API. The test is still somewhat incomplete in this respect though because we haven't added the verification of the data in the crash report (https://b.corp.google.com/issues/310979503#comment9). This behavior is easy enough to test end-to-end manually on Evergreen devices, so I'll probably just do that for x64 and AOSP, anyway. Also, note this YTS test is Evergreen specific, so can't help with other platforms.

For ps5, @niranjanyardi offered to help test this locally. Based on the discussion in b/249761235, I think we should be able to verify that the ps5 crash has the expected client context after we force a crash.

For Apple and Android, I can talk to platform owners and see how much effort it would be to validate that setString still works correctly. It would be very surprising if it didn't, since these changes are mostly scoped to platforms that define CORE_DUMP_HANDLER_SUPPORT, which I think used to be just Playstation and now is no platform, but we can at least look into testing it.

Longer term we should think about whether we can write end-to-end tests for setString for non-Evergreen platforms.

Joel and I discussed offline and are ok with just testing on Evergreen platforms and ps5; as long as the CrashHandler Starboard extension is invoked by the setString web API's implementation, we can be confident there isn't a regression for other platforms.

I already tested on Evergreen (on x64x11 and ADT-4), so we're just waiting on ps5.

[hlwarriner]

on

@niranjanyardi spent some time looking into this but it sounds like we currently don't have a way to verify that this change doesn't cause a regression w.r.t. the setString implementation on ps5. From what I understand, the problem is that 1) there's no way to generate/upload a "reported crash" from a devkit or testkit, and 2) feature request b/181902049 is still open.

@joeltine, are you ok with merging this?

[joeltine]

there's no way to generate/upload a "reported crash" from a devkit or testkit,

This seems problematic in general. Did we verify this with Sony?

[hlwarriner]

there's no way to generate/upload a "reported crash" from a devkit or testkit,

This seems problematic in general. Did we verify this with Sony?

@niranjanyardi or @madhurajayaraman can you please comment?

I agree we should have a way to manually test this end-to-end - maybe we can increase the priority of b/181902049? But if we don't have a way to end-to-end test this currently - and it sounds like we don't - I'd like to not block this cleanup.

[joeltine]

there's no way to generate/upload a "reported crash" from a devkit or testkit,

This seems problematic in general. Did we verify this with Sony?

@niranjanyardi or @madhurajayaraman can you please comment?

I agree we should have a way to manually test this end-to-end - maybe we can increase the priority of b/181902049? But if we don't have a way to end-to-end test this currently - and it sounds like we don't - I'd like to not block this cleanup.

Yes, please proceed if there's no expedient way to test this. I'd just be a bit surprised if Sony didn't have a way to trigger crash reports in a dev environment.

[niranjanyardi]

there's no way to generate/upload a "reported crash" from a devkit or testkit,

This seems problematic in general. Did we verify this with Sony?

@niranjanyardi or @madhurajayaraman can you please comment?
I agree we should have a way to manually test this end-to-end - maybe we can increase the priority of b/181902049? But if we don't have a way to end-to-end test this currently - and it sounds like we don't - I'd like to not block this cleanup.

Yes, please proceed if there's no expedient way to test this. I'd just be a bit surprised if Sony didn't have a way to trigger crash reports in a dev environment.

I asked sony, subscribed you to the support request.

[hlwarriner]

there's no way to generate/upload a "reported crash" from a devkit or testkit,

This seems problematic in general. Did we verify this with Sony?

@niranjanyardi or @madhurajayaraman can you please comment?
I agree we should have a way to manually test this end-to-end - maybe we can increase the priority of b/181902049? But if we don't have a way to end-to-end test this currently - and it sounds like we don't - I'd like to not block this cleanup.

Yes, please proceed if there's no expedient way to test this. I'd just be a bit surprised if Sony didn't have a way to trigger crash reports in a dev environment.

I asked sony, subscribed you to the support request.

Thanks, Niranjan. And sorry, I just have one more question on this: even if the crash report isn't "reported," is one still generated on the device that can be inspected locally somehow? Maybe we can ask Sony this, too, if we're not sure.

[niranjanyardi]

there's no way to generate/upload a "reported crash" from a devkit or testkit,

This seems problematic in general. Did we verify this with Sony?

@niranjanyardi or @madhurajayaraman can you please comment?
I agree we should have a way to manually test this end-to-end - maybe we can increase the priority of b/181902049? But if we don't have a way to end-to-end test this currently - and it sounds like we don't - I'd like to not block this cleanup.

Yes, please proceed if there's no expedient way to test this. I'd just be a bit surprised if Sony didn't have a way to trigger crash reports in a dev environment.

I asked sony, subscribed you to the support request.

Thanks, Niranjan. And sorry, I just have one more question on this: even if the crash report isn't "reported," is one still generated on the device that can be inspected locally somehow? Maybe we can ask Sony this, too, if we're not sure.

Good point Joel. I generated a coredump on my ps5 devkit and it does have the user info , see: https://screenshot.googleplex.com/9FckKQ4bS7jdsdH . So we have successfully verified this change doesnt break ps5