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

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

Merged
merged 1 commit into from Aug 17, 2016

Conversation

bridiver
Copy link
Contributor

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

@zcbenz
Copy link
Member

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
Copy link
Contributor Author

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

@bridiver
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

zcbenz commented Aug 17, 2016

👍

@zcbenz zcbenz merged commit 47c37d6 into electron:master Aug 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants