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

don't convert a string to an array buffer #6814

Merged
merged 1 commit into from Aug 17, 2016

Conversation

Projects
None yet
2 participants
@bridiver
Contributor

bridiver commented Aug 11, 2016

see brave/muon@d072e61#diff-7452c2b1cabfbce577fef50b254bd08dL33
encoded_data is a base64 string so there is no reason to convert it anyway
cc @deepak1556

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Aug 12, 2016

Can you also change the docs of this field?

I'm cool with this change since it is compatible with most existing code, but you should notice that it doesn't not actually fix the crash you saw, it only works around it.

@bridiver

This comment has been minimized.

Contributor

bridiver commented Aug 13, 2016

Sure, I'll update the docs, but why do you say that it only works around the crash and doesn't fix it?

@bridiver

This comment has been minimized.

Contributor

bridiver commented Aug 13, 2016

with the buffer we get a crash, but with a string we get the cert and everything works. I've had problems with node::Buffer every time I've tried to use it and the only way I've ever successfully sent unmunged binary data is with something like this

const uint8_t* data = reinterpret_cast<const uint8_t*>(body.c_str());
  v8::String::NewFromOneByte(isolate(),
      data, v8::NewStringType::kNormal, body.length()).ToLocalChecked();
@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Aug 17, 2016

Sure, I'll update the docs, but why do you say that it only works around the crash and doesn't fix it?

This crash is essentially caused by some weird thing when calling node::Buffer::Copy, since this API is widely used in both Electron and node, you will probably see similar crashes in future.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Aug 17, 2016

👍

@zcbenz zcbenz merged commit 47c37d6 into electron:master Aug 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment