Fix threading-related crash in Cobalt telemetry. #689
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joeltine
changed the title
joeltine
force-pushed
the
blackbox-and-demo
branch
2 times, most recently
from
8509dff
to
785b5fd
Compare
|
FYI: this PR is based off #645 and makes a few tweaks on top of it. Happy to replace that PR with this one, if you'd prefer. |
The Chromium metrics libraries require any calls to their public APIs to be invoked on the same thread in which they were created. Similarly, H5vcc JS callback need to be invoked on the V8 thread. This was being violated as the H5vcc callbacks were being invoked by a non-V8 thread and H5vccMetrics was trying to call metrics APIs from a V8 thread. The fix is to store the task runners for the CobaltMetricsServicesManager and H5vccMetrics instances and use those to run any tasks that must be run in the targeted thread. This approach required a slight refactoring to allow the top level CobaltMetricsServicesManager to provide its own public API to interact with the underlying metrics client on a targeted thread. I also found this approach allowed me to just pass around a RepeatingCallback for the upload handler and remove the abstract class that was being used before (CobaltH5vccMetricsUploaderCallback). Added demo to verify everything is working. b/288252273 Change-Id: I8243b27a1853c65ac9d4fe11adc95a7f4da9ed51
joeltine
force-pushed
the
blackbox-and-demo
branch
from
785b5fd
to
00993e7
Compare
|
FYI: Just rebased this on top of #645, which was submitted just recently. |
sherryzy
reviewed
Jun 22, 2023
sherryzy
reviewed
Jun 22, 2023
sherryzy
approved these changes
Jun 22, 2023
cobalt-github-releaser-bot
pushed a commit
that referenced
this pull request
Jun 27, 2023
The Chromium metrics libraries require any calls to their public APIs to be invoked on the same thread in which they were created. Similarly, H5vcc JS callback need to be invoked on the V8 thread. This was being violated as the H5vcc callbacks were being invoked by a non-V8 thread and H5vccMetrics was trying to call metrics APIs from a V8 thread. The fix is to store the task runners for the CobaltMetricsServicesManager and H5vccMetrics instances and use those to run any tasks that must be run in the targeted thread. This approach required a slight refactoring to allow the top level CobaltMetricsServicesManager to provide its own public API to interact with the underlying metrics client on a targeted thread. I also found this approach allowed me to just pass around a RepeatingCallback for the upload handler and remove the abstract class that was being used before (CobaltH5vccMetricsUploaderCallback). Added demo to verify everything is working. b/288252273 Change-Id: I8243b27a1853c65ac9d4fe11adc95a7f4da9ed51 (cherry picked from commit a89e9fb)
joeltine
added a commit
that referenced
this pull request
Jun 27, 2023
…#737) The Chromium metrics libraries require any calls to their public APIs to be invoked on the same thread in which they were created. Similarly, H5vcc JS callback need to be invoked on the V8 thread. This was being violated as the H5vcc callbacks were being invoked by a non-V8 thread and H5vccMetrics was trying to call metrics APIs from a V8 thread. The fix is to store the task runners for the CobaltMetricsServicesManager and H5vccMetrics instances and use those to run any tasks that must be run in the targeted thread. This approach required a slight refactoring to allow the top level CobaltMetricsServicesManager to provide its own public API to interact with the underlying metrics client on a targeted thread. I also found this approach allowed me to just pass around a RepeatingCallback for the upload handler and remove the abstract class that was being used before (CobaltH5vccMetricsUploaderCallback). Added demo to verify everything is working. b/288252273 Change-Id: I8243b27a1853c65ac9d4fe11adc95a7f4da9ed51 (cherry picked from commit a89e9fb) Co-authored-by: Joel Martinez <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix threading-related crash in Cobalt telemetry.
The Chromium metrics libraries require any calls to their public APIs to
be invoked on the same thread in which they were created. Similarly,
H5vcc JS callback need to be invoked on the V8 thread.
This was being violated as the H5vcc callbacks were being invoked by a
non-V8 thread and H5vccMetrics was trying to call metrics APIs from a V8
thread.
The fix is to store the task runners for the
CobaltMetricsServicesManager and H5vccMetrics instances and use those to
run any tasks that must be run in the targeted thread.
This approach required a slight refactoring to allow the top level
CobaltMetricsServicesManager to provide its own public API to interact
with the underlying metrics client on a targeted thread.
I also found this approach allowed me to just pass around a
RepeatingCallback for the upload handler and remove the abstract class
that was being used before (CobaltH5vccMetricsUploaderCallback).
Added a demo page to verify all the behavior is correct.
b/288252273
Change-Id: I878285894293f5faae409e477998b2073631f306