Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(inspector): ensure socket message is copied and stored #155

Merged
merged 1 commit into from
Mar 18, 2022

Conversation

edusperoni
Copy link
Collaborator

@edusperoni edusperoni commented Mar 17, 2022

also adds a nice semaphore so we convert a busy wait into a more CPU friendly one

@cla-bot cla-bot bot added the cla: yes label Mar 17, 2022
@edusperoni edusperoni marked this pull request as draft March 17, 2022 20:00
@edusperoni edusperoni marked this pull request as ready for review March 18, 2022 11:41
@edusperoni edusperoni requested a review from NathanWalker March 18, 2022 11:41
@edusperoni
Copy link
Collaborator Author

@NathanWalker it seems this does solve the iOS crash when debugging with the devtools. Still not sure why this is even needed as it's just adding some "inefficient" code to ensure we're copying strings and retaining them.

To expand further in the issue, the message is received correctly, gets into the onFrontendMessageReceived correctly and with the correct string but somewhere between there and the pop_back() the string has invalid data, so you either get garbage in it and it fails a conversion or pop_back() crashes saying the internal std::string storage (cstring) isn't even allocated to begin with. My suspiscion is that somehow the std::string message was being deleted (maybe due to the fact that it comes from dispatch_data_t) before the block executed, so garbage data was being pushed back.

Not confident in my explanation. The issue was too rare for a proper debugging/memory inspection, but this change seems to fix it.

@NathanWalker NathanWalker merged commit 3098976 into main Mar 18, 2022
@NathanWalker NathanWalker deleted the fix/inspector-string-crash branch March 18, 2022 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants