Port to node-webkit #100

Closed
wants to merge 56 commits into
from

Conversation

Projects
None yet
5 participants
@probablycorey
Member

probablycorey commented Dec 5, 2012

An experiment to port Atom's WebKit backend from CEF to node-webkit. If this works we will have node.js as a first-class citizen in WebKit's V8 engine. This will remove the need for much of our native code and allow us to use npm modules.

probablycorey-and-nathan and others added some commits Dec 1, 2012

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Dec 4, 2012

Contributor

Hey Corey, an OnigScanner is supposed to hold an array of regular expressions, not just one.

Contributor

nathansobo commented on 70cf466 Dec 4, 2012

Hey Corey, an OnigScanner is supposed to hold an array of regular expressions, not just one.

This comment has been minimized.

Show comment
Hide comment
@probablycorey

probablycorey Dec 4, 2012

Member
Member

probablycorey replied Dec 4, 2012

@probablycorey

This comment has been minimized.

Show comment
Hide comment
@probablycorey

probablycorey Dec 5, 2012

Member

Not ready to merge... Pull request opened as a place for discussion.

Member

probablycorey commented Dec 5, 2012

Not ready to merge... Pull request opened as a place for discussion.

@probablycorey

This comment has been minimized.

Show comment
Hide comment
@probablycorey

probablycorey Dec 5, 2012

Member

@aroben Could you review the files in src/stdlib/oniguruma/src/

I mostly want to know:

  1. Is any of it wrong.
  2. Could parts be done better or in a more standard C++ way.
Member

probablycorey commented Dec 5, 2012

@aroben Could you review the files in src/stdlib/oniguruma/src/

I mostly want to know:

  1. Is any of it wrong.
  2. Could parts be done better or in a more standard C++ way.
src/stdlib/oniguruma/src/onig-reg-exp.cc
+ int end = searchString.size();
+ OnigRegion* region = onig_region_new();
+ const UChar* searchData = (const UChar*)searchString.data();
+ int status = onig_search(regex_, searchData, searchData + searchString.size(), searchData + position, searchData + end, region, ONIG_OPTION_NONE);

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

I think you have some buffer overflow bugs here.

UChar is a 16-bit type, while char is 8 bits. Let's say the function is called like this:

regExp.Search("abcdef", 5, &result);

Let's say the string characters are stored at memory locations 400–405 (including the null terminator). So searchData is 400. But since searchData is a UChar *, searchData + position == 400 + (5 * sizeof(UChar)) == 400 + (5 * 2) == 410, which is beyond the end of the string.

Does onig_search expect the input string to have the same encoding as was specified to onig_new? If so, you need to cast to UChar * after doing your pointer arithmetic. If it expects some other encoding, you're going to have to transcode the string.

@aroben

aroben Dec 6, 2012

Contributor

I think you have some buffer overflow bugs here.

UChar is a 16-bit type, while char is 8 bits. Let's say the function is called like this:

regExp.Search("abcdef", 5, &result);

Let's say the string characters are stored at memory locations 400–405 (including the null terminator). So searchData is 400. But since searchData is a UChar *, searchData + position == 400 + (5 * sizeof(UChar)) == 400 + (5 * 2) == 410, which is beyond the end of the string.

Does onig_search expect the input string to have the same encoding as was specified to onig_new? If so, you need to cast to UChar * after doing your pointer arithmetic. If it expects some other encoding, you're going to have to transcode the string.

This comment has been minimized.

@probablycorey

probablycorey Dec 7, 2012

Member

Isn't UChar an unsigned char, so it would be the same number of bytes as a char?

@probablycorey

probablycorey Dec 7, 2012

Member

Isn't UChar an unsigned char, so it would be the same number of bytes as a char?

This comment has been minimized.

@aroben

aroben Dec 7, 2012

Contributor

Hm, I could be wrong. UChar is a 16-bit value in ICU, but maybe Oniguruma/V8/whoever is defining the type here has a different definition. If you find the typedef you could paste it here and settle this. :-)

@aroben

aroben Dec 7, 2012

Contributor

Hm, I could be wrong. UChar is a 16-bit value in ICU, but maybe Oniguruma/V8/whoever is defining the type here has a different definition. If you find the typedef you could paste it here and settle this. :-)

This comment has been minimized.

@probablycorey

probablycorey Dec 7, 2012

Member

#define UChar OnigUChar
typedef unsigned char OnigUChar;

They be kooky.

@probablycorey

probablycorey Dec 7, 2012

Member

#define UChar OnigUChar
typedef unsigned char OnigUChar;

They be kooky.

This comment has been minimized.

@aroben

aroben Dec 7, 2012

Contributor

Awesome. But very confusing for it to be different from ICU. :-)

@aroben

aroben Dec 7, 2012

Contributor

Awesome. But very confusing for it to be different from ICU. :-)

src/stdlib/oniguruma/src/onig-reg-exp.cc
+ int status = onig_search(regex_, searchData, searchData + searchString.size(), searchData + position, searchData + end, region, ONIG_OPTION_NONE);
+
+ if (status != ONIG_MISMATCH) {
+ *result = new OnigResult(region, searchString);

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

Is it valid for the caller to pass a null pointer for result? If so, you should check that condition. If not, you should decide if crashing is what you want to have happen.

@aroben

aroben Dec 6, 2012

Contributor

Is it valid for the caller to pass a null pointer for result? If so, you should check that condition. If not, you should decide if crashing is what you want to have happen.

src/stdlib/oniguruma/src/onig-reg-exp.h
+
+ private:
+ std::string source_;
+ regex_t *regex_;

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

If OnigRegExp is copied, two instances will have the same regex_ pointer and will both try to free it when they're destroyed. You should either disallow copying by adding this to the class:

private:
  OnigRegExp(const OnigRegExp &);
  OnigRegExp &operator=(const OnigRegExp &);

(Just declare those, don't implement them.)

Or you should make the class copyable by making the above declarations public and implementing them.

@aroben

aroben Dec 6, 2012

Contributor

If OnigRegExp is copied, two instances will have the same regex_ pointer and will both try to free it when they're destroyed. You should either disallow copying by adding this to the class:

private:
  OnigRegExp(const OnigRegExp &);
  OnigRegExp &operator=(const OnigRegExp &);

(Just declare those, don't implement them.)

Or you should make the class copyable by making the above declarations public and implementing them.

src/stdlib/oniguruma/src/onig-result.cc
+#include "onig-reg-exp.h"
+#include "onig-result.h"
+
+OnigResult::OnigResult(OnigRegion* region, std::string &searchString) : searchString_(searchString) {

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

const std::string &searchString.

@aroben

aroben Dec 6, 2012

Contributor

const std::string &searchString.

src/stdlib/oniguruma/src/onig-result.h
+ int LengthAt(int index);
+
+ std::string searchString_;
+ OnigRegion *region_;

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

Same comments here about copying.

@aroben

aroben Dec 6, 2012

Contributor

Same comments here about copying.

src/stdlib/oniguruma/src/onig-reg-exp.h
+
+class OnigRegExp {
+ public:
+ OnigRegExp(std::string source);

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

It's usually best to mark single-parameter constructors with the explicit keyword. That will prevent someone accidentally converting a std::string to an OnigRegExp when they didn't mean to.

explicit OnigRegExp(const std::string &source);
@aroben

aroben Dec 6, 2012

Contributor

It's usually best to mark single-parameter constructors with the explicit keyword. That will prevent someone accidentally converting a std::string to an OnigRegExp when they didn't mean to.

explicit OnigRegExp(const std::string &source);
+ tpl->InstanceTemplate()->SetInternalFieldCount(1);
+ tpl->PrototypeTemplate()->Set(v8::String::NewSymbol("findNextMatch"), FunctionTemplate::New(OnigScanner::FindNextMatch)->GetFunction());
+
+ Persistent<Function> constructor = Persistent<Function>::New(tpl->GetFunction());

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

All the work above this point kind of seems like it should only be done once per process. But I don't know the V8 API, so I could well be wrong.

@aroben

aroben Dec 6, 2012

Contributor

All the work above this point kind of seems like it should only be done once per process. But I don't know the V8 API, so I could well be wrong.

This comment has been minimized.

@probablycorey

probablycorey Dec 7, 2012

Member

I believe this runs once per V8 Context.

@probablycorey

probablycorey Dec 7, 2012

Member

I believe this runs once per V8 Context.

+
+Handle<Value> OnigScanner::New(const Arguments& args) {
+ HandleScope scope;
+ OnigScanner* scanner = new OnigScanner(Local<Array>::Cast(args[0]));

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

Who is responsible for deleting this object?

@aroben

aroben Dec 6, 2012

Contributor

Who is responsible for deleting this object?

This comment has been minimized.

@probablycorey

probablycorey Dec 7, 2012

Member

I had this same question. I'm assuming it is V8 though because it is being deallocated when the V8 context is destroyed.

@probablycorey

probablycorey Dec 7, 2012

Member

I had this same question. I'm assuming it is V8 though because it is being deallocated when the V8 context is destroyed.

This comment has been minimized.

@aroben

aroben Dec 7, 2012

Contributor

👍 Might be worth adding a comment here about that.

@aroben

aroben Dec 7, 2012

Contributor

👍 Might be worth adding a comment here about that.

src/stdlib/oniguruma/src/onig-scanner.cc
+
+OnigScanner::~OnigScanner() {
+ for (std::vector<OnigRegExp *>::iterator iter = regExps.begin(); iter < regExps.end(); iter++) {
+ delete *iter;

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

I don't know if you're compiling this code as C++11. But if you are, you can make this a lot more concise:

for (auto regExp : regExps) {
  delete regExp;
}
@aroben

aroben Dec 6, 2012

Contributor

I don't know if you're compiling this code as C++11. But if you are, you can make this a lot more concise:

for (auto regExp : regExps) {
  delete regExp;
}

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

Another option would be to make regExps use std::vector<std::unique_ptr<OnigRegExp>> as its type. That way the lifetime of the OnigRegExp options will be managed for you.

@aroben

aroben Dec 6, 2012

Contributor

Another option would be to make regExps use std::vector<std::unique_ptr<OnigRegExp>> as its type. That way the lifetime of the OnigRegExp options will be managed for you.

src/stdlib/oniguruma/src/onig-scanner.cc
+ delete *iter;
+ }
+ for (std::vector<OnigResult *>::iterator iter = cachedResults.begin(); iter < cachedResults.end(); iter++) {
+ delete *iter;

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

Ditto the above.

@aroben

aroben Dec 6, 2012

Contributor

Ditto the above.

src/stdlib/oniguruma/src/onig-scanner.cc
+ OnigRegExp *regExp = *iter;
+
+ bool useCachedResult = false;
+ OnigResult *result = NULL;

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

If you're using C++11, you can (and should) use nullptr instead of NULL.

@aroben

aroben Dec 6, 2012

Contributor

If you're using C++11, you can (and should) use nullptr instead of NULL.

src/stdlib/oniguruma/src/onig-scanner.cc
+ while (iter != cachedResults.end()) {
+ delete *iter;
+ iter = cachedResults.erase(iter);
+ }

This comment has been minimized.

@aroben

aroben Dec 6, 2012

Contributor

If you were to use std::vector<std::unique_ptr<OnigResult>> this would just become cachedResults.clear().

@aroben

aroben Dec 6, 2012

Contributor

If you were to use std::vector<std::unique_ptr<OnigResult>> this would just become cachedResults.clear().

@aroben

This comment has been minimized.

Show comment
Hide comment
@aroben

aroben Dec 6, 2012

Contributor

OK, I made it through the C++ code.

Contributor

aroben commented Dec 6, 2012

OK, I made it through the C++ code.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Dec 6, 2012

Contributor

Holy shit balls Adam. That was amazing.

Contributor

nathansobo commented Dec 6, 2012

Holy shit balls Adam. That was amazing.

@aroben

This comment has been minimized.

Show comment
Hide comment
@aroben

aroben Dec 7, 2012

Contributor

😊 🙇

Contributor

aroben commented Dec 7, 2012

😊 🙇

@nuclearsandwich

This comment has been minimized.

Show comment
Hide comment
@nuclearsandwich

nuclearsandwich Jan 19, 2013

Quick question: It looks like node-webkit uses the Content Shell API instead of CEF. Doesn't that hamstring it by removing things like context menu support? Is that something that is easy to roll ourselves?

Quick question: It looks like node-webkit uses the Content Shell API instead of CEF. Doesn't that hamstring it by removing things like context menu support? Is that something that is easy to roll ourselves?

@probablycorey

This comment has been minimized.

Show comment
Hide comment
@probablycorey

probablycorey Jan 21, 2013

Member

You can just layer context menu support on top of it. CEF3 uses the Content API as well, it just adds a bit of native code on top of it.

Member

probablycorey commented Jan 21, 2013

You can just layer context menu support on top of it. CEF3 uses the Content API as well, it just adds a bit of native code on top of it.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Mar 20, 2013

Contributor

We are not going this route for Node integration. We've hired the maintainer on contract.

Contributor

nathansobo commented Mar 20, 2013

We are not going this route for Node integration. We've hired the maintainer on contract.

@nathansobo nathansobo closed this Mar 20, 2013

maxbrunsfeld pushed a commit that referenced this pull request Jul 26, 2016

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