tondrej / chakracore-delphi

Delphi and Free Pascal bindings and classes for Microsoft's ChakraCore library
https://tondrej.blogspot.com/search/label/chakracore
MIT License
137 stars 34 forks source link

TMethod(NativeMethod).Data is nil #10

Closed pony5551 closed 5 years ago

pony5551 commented 5 years ago
TMethod(NativeMethod).Code := CallbackState;
TMethod(NativeMethod).Data := JsGetExternalData(Args^[0]);

//Can we add a judgment here?
if TMethod(NativeMethod).Data=nil then
  raise Exception.Create('Data is null.');

//If you execute the wrong js code, Data is nil
if Args^[0] <> TNativeObject(TMethod(NativeMethod).Data).Instance then
  raise Exception.Create('thisarg not the registered instance');
tondrej commented 5 years ago

Sure we can. Defensive programming style has a good purpose. Just out of curiosity, do you have any particular concern? Could you provide an example of "executing the wrong js code"? Then we could add a test case for it. You're welcome to write one. :-)

pony5551 commented 5 years ago

Sorry, this problem is very complicated, I will look for the reason, but it is really TMethod(NativeMethod).data =nil

tondrej commented 5 years ago

Guessing from the code you posted (it's unclear which lines exactly you mean), if JsGetExternalData in Native_PropSetCallback returns nil it probably means that your instance of the TNativeObject descendant has already been freed when the script is trying to set a property on it.

The constructor sets Self as the external data here or here, the destructor clears it again.

So I guess in your script you're trying to set a property on an instance which has already been freed.

In this case I agree that an exception with a clear error message would be helpful (instead of the access violation I guess you're getting now).

Again, an MCVE would be helpful... at least, what error you get and where you get it.

Anyway, I'll probably review the callbacks and put in some more defensive checking.

pony5551 commented 5 years ago

Thank you. The problem is found. You reminded me that I released TObject in Delphi code in advance, so JsGetExternalData gets nil

Thank you very much!

tondrej commented 5 years ago

You're welcome. I'll close this issue later when I have the changes ready.

I'm glad to have feedback which can lead to improvement of the code. Thanks again for your help.