Try and handle NSData UTF8 encoding failures in subliminal-instruments #231
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
justinseanmartin
changed the title
|
Credit for the fix goes to @wearhere. He cleaned up my quick and dirty fix and instead made the right fix. The new branch commit no longer should exhibit the issues that I called out as the potential issues. |
| NSUInteger lineStart = 0; | ||
| for (; charOffset < [charBuffer length] / sizeof(char); charOffset++) { | ||
| if (chars[charOffset] == '\n') { | ||
| NSData *lineData = [charBuffer subdataWithRange:NSMakeRange(lineStart, (charOffset - 1) * sizeof(char))]; |
There was a problem hiding this comment.
I messed this up per Travis. I think it should be charOffset - 1 - lineStart?
There was a problem hiding this comment.
Fixed up, running locally and on CI now, so far so good.
|
Thanks @justinseanmartin for exploring the edge cases here--and, I must note, suggesting the idea that I implemented, and then fixing my bug in that implementation. ;) Cheers. |
wearhere
added a commit
that referenced
this pull request
Jul 10, 2014
…etter-utf8-decoding Try and handle NSData UTF8 encoding failures in subliminal-instruments
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.
The issue with the current logic is that it assumes that each individual data buffer will successfully convert into a UTF8 string. However, this is not the case if instruments flushes the pipe in the middle of a UTF8 multi-byte character, and will lead to an exception when we try to appendString:nil to buffer.
The easy solution to this crash is that if the data buffer doesn't convert, then wait for the next data buffer. The assumption is that before the pipe is closed, we'll receive a buffer that can be successfully converted.
FYI - One potential issue with this solution is that if the Instruments output has a lot of non-ASCII text in it, the data buffer will be more likely to fail conversion (and grow the buffer in memory while doing so). This is could possibly happen with a non-English localized app-under-test (e.g. Japanese). This could be even more problematic if the OSX system language were set to a different language.