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

Parse debugger protocol messages using v8 JSON parser #9322

Merged
merged 3 commits into from May 1, 2017

Conversation

Projects
None yet
2 participants
@kevinsawicki
Contributor

kevinsawicki commented Apr 28, 2017

This pull request switches the debugger to parse protocol messages using v8::JSON::Parse instead of base::JSONReader::Read.

base::JSONReader::Read has strict range parsing of escape characters and is currently failing to parse the following debug protocol message:

{"id":2,"result":{"body":"\uFFFF","base64Encoded":false}}

Fixes #9257

@kevinsawicki kevinsawicki requested a review from deepak1556 Apr 28, 2017

@kevinsawicki kevinsawicki changed the title from Use v8 to parse JSON debugger messages to Parse debugger protocol messages using v8 JSON parser Apr 28, 2017

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Apr 30, 2017

Member

Chrome had a similar crash report with chrome.debugger api. They have base64 encoded non valid utf8 characters https://bugs.chromium.org/p/chromium/issues/detail?id=559772 and ignored the cases where JSONReader decoding fails. We could just follow the same behavior.

Member

deepak1556 commented Apr 30, 2017

Chrome had a similar crash report with chrome.debugger api. They have base64 encoded non valid utf8 characters https://bugs.chromium.org/p/chromium/issues/detail?id=559772 and ignored the cases where JSONReader decoding fails. We could just follow the same behavior.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki May 1, 2017

Contributor

ignored the cases where JSONReader decoding fails. We could just follow the same behavior.

If we ignore the cases where JSONReader fails then I don't think there is a way to get the spec in this pull request to pass, which means the callbacks for certain commands sent will never get called which seems confusing and hard to debug.

Contributor

kevinsawicki commented May 1, 2017

ignored the cases where JSONReader decoding fails. We could just follow the same behavior.

If we ignore the cases where JSONReader fails then I don't think there is a way to get the spec in this pull request to pass, which means the callbacks for certain commands sent will never get called which seems confusing and hard to debug.

@deepak1556

👍

@kevinsawicki kevinsawicki merged commit 98d17d6 into master May 1, 2017

7 of 9 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-linux-arm Build #6440318 succeeded in 71s
Details
electron-linux-ia32 Build #6440319 succeeded in 67s
Details
electron-linux-x64 Build #6440320 succeeded in 155s
Details
electron-mas-x64 Build #4057 succeeded in 9 min 10 sec
Details
electron-osx-x64 Build #4059 succeeded in 8 min 29 sec
Details
electron-win-ia32 Build #3043 succeeded in 9 min 17 sec
Details
electron-win-x64 Build #3017 succeeded in 8 min 14 sec
Details
@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki May 1, 2017

Contributor

Thanks @deepak1556 for reviewing 👀 👍

Contributor

kevinsawicki commented May 1, 2017

Thanks @deepak1556 for reviewing 👀 👍

@kevinsawicki kevinsawicki deleted the debugger-json-crash branch May 1, 2017

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