JSRT: Inconsistent Callback behavior

[obastemur]

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]

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]

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]

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

[liminzhu]

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]

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

[jianchun]

@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]

@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]

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]

@liminzhu Should we keep tracking this?