search

chakra-core / ChakraCore

ChakraCore is an open source Javascript engine with a C API.
MIT License
9.07k stars 1.19k forks source link

JSRT: Inconsistent Callback behavior #1917

Open obastemur opened 7 years ago

obastemur commented 7 years ago

Only few of our callbacks can execute JSRT API calls. i.e. new JsSerializedLoadScriptCallback is one of them. https://github.com/Microsoft/ChakraCore/blob/master/lib/Jsrt/JsrtSourceHolder.cpp#L246-L252

Is there any particular callback we shouldn't allow JSRT API calls ? or best if we make them all capable of doing this?

/cc @agarwal-sandeep @liminzhu

liminzhu commented 7 years ago

What's the current behavior? What JSRT APIs do we allow to call in JsSerializedLoadScriptCallback and other callbacks? Everything including JsRun? I can see that as a problem...

jianchun commented 7 years ago

If I understand correctly, the issue sample code originally had a problem pointed out by @agarwal-sandeep , that when calling user provided callback we should LEAVE_SCRIPT. I think we should do this for every user provided native-callback. (It is a different problem what JSRT API that callback can call.)

agarwal-sandeep commented 7 years ago

The only problem I see is that this opens up new reentrancy paths which are untested.

liminzhu commented 7 years ago

IIUC @jianchun @agarwal-sandeep both of you are talking about script re-entrancy? We should disallow that in callbacks. Any other calls are fine.

obastemur commented 6 years ago

@jianchun @agarwal-sandeep what we should do here?

jianchun commented 6 years ago

@obastemur This issue itself sounds unclear to me. By "inconsistent", could you enumerate the callbacks that behave inconsistently -- assuming you mean some allowed and some disallowed to call JSRT? My understanding is that I don't recall we have special code to allow/disallow, thus mostly the callbacks should by default all allowed to call JSRT. Only specific ones may be special due to specific reasons.

Or, @agarwal-sandeep, was it the background of this issue that you discovered we forgot to LEAVE_SCRIPT when calling callbacks? If so we should just examine all JSRT callbacks and ensure if we entered script we should LEAVE_SCRIPT?

obastemur commented 6 years ago

@jianchun Your previous comment was pointing out to right direction. Right, it's about LEAVE_SCRIPT. Hence the question I asked above, Is there any particular callback we shouldn't allow JSRT API calls ?

agarwal-sandeep commented 6 years ago

My concern was related to thorough reentrancy test coverage. I feel we don't have good test coverage which can test this. +1 for allowing callbacks to call JsRT API.

kfarnung commented 6 years ago

@liminzhu Should we keep tracking this?

liminzhu commented 6 years ago

Yeah let's have this one open. It's a good discussion to have since it taps into some not well-defined behavior.