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

Handle invalid utf8 characters for debugger #11774

Merged
merged 1 commit into from Feb 5, 2018

Conversation

Projects
None yet
3 participants
@nitsakh
Contributor

nitsakh commented Jan 30, 2018

This fixes disabled test.
The chrome json parser does not handle invalid UTF8 input well. So, we switch to using the v8 json parser, which handles invalid UTF8 characters. However, the parsed json cannot be converted to the dictionary, so we replace invalid UTF8 data, before converting to the dictionary.

@nitsakh nitsakh requested a review from electron/reviewers as a code owner Jan 30, 2018

@codebytere

This comment has been minimized.

Member

codebytere commented Jan 31, 2018

i'm re-kicking CI, the appveyor failure had a pretty strange log

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Feb 1, 2018

I would prefer the current version of ignoring invalid responses similar to what chrome debugger api does https://bugs.chromium.org/p/chromium/issues/detail?id=559772 rather than reformatting it as invalid data.

@nitsakh

This comment has been minimized.

Contributor

nitsakh commented Feb 1, 2018

In that case the test does not make sense, right? Because we do not handle invalid characters.
So maybe the test can be removed then.

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Feb 1, 2018

The test can be restructured to verify that invalid characters don't cause a crash and at the same time ensure valid characters are processed as expected.

@deepak1556

deepak1556 approved these changes Feb 5, 2018 edited

LGTM, Thanks!

@codebytere codebytere merged commit e6a5990 into electron:master Feb 5, 2018

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@nitsakh nitsakh deleted the nitsakh:fix-invalid-utf branch Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment