Issue 6526 - *ToV8String() return MaybeLocal<> and check Call() return value

Issue 6526 - *ToV8String() return MaybeLocal<> and check Call() return value

[hub, 2018-06-22 00:34:31]

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsEngine.cpp#ne...
src/JsEngine.cpp:242: return JsValue(shared_from_this(),
v8::Undefined(GetIsolate()));
This is where I think we should break APIs to some extent. For now I return
`undefined` if there is an error.

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsValue.cpp#new...
src/JsValue.cpp:189: throw std::runtime_error("Invalid property name");
Since we already can throw, I thought it was OK to throw too. This is defintely
an error if we can make the key.

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsValue.cpp#new...
src/JsValue.cpp:193: return JsValue(jsEngine, v8::Undefined(isolate));
If the property isn't found we return `undefined`.

https://codereview.adblockplus.org/29812649/diff/29812650/src/Utils.cpp
File src/Utils.cpp (right):

https://codereview.adblockplus.org/29812649/diff/29812650/src/Utils.cpp#newco...
src/Utils.cpp:72: Utils::ToV8String(isolate, "Unknown
Exception").ToLocalChecked());
Failure throwing an exception. Not good. So at least we should be fine. Note
that since I pass a plain ASCII string, I expect the Maybe<> to not be empty. I
believe the assumption is correct.

[sergei, 2018-06-22 06:57:47]

I think we need several tests to better understand what is appropriate when a
value is empty.

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsEngine.cpp#ne...
src/JsEngine.cpp:242: return JsValue(shared_from_this(),
v8::Undefined(GetIsolate()));
On 2018/06/22 00:34:31, hub wrote:
> This is where I think we should break APIs to some extent. For now I return
> `undefined` if there is an error. 

Let's just use CHECKED_TO_LOCAL.

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsValue.cpp#new...
src/JsValue.cpp:189: throw std::runtime_error("Invalid property name");
On 2018/06/22 00:34:31, hub wrote:
> Since we already can throw, I thought it was OK to throw too. This is
defintely
> an error if we can make the key.

I think here we should use CHECKED_TO_LOCAL.

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsValue.cpp#new...
src/JsValue.cpp:193: return JsValue(jsEngine, v8::Undefined(isolate));
On 2018/06/22 00:34:31, hub wrote:
> If the property isn't found we return `undefined`.

If the property is not found then the result of obj->Get(property) already
contains v8::Undefined. However, if something is wrong with the engine, then
maybe.IsEmpty() is true. So, IMO we should rather stop our execution as well.
Could you please add a test here?

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsValue.cpp#new...
src/JsValue.cpp:201: auto isolate = jsEngine->GetIsolate();
I think here and below we should use CHECKED_TO_LOCAL.

https://codereview.adblockplus.org/29812649/diff/29812650/src/Utils.cpp
File src/Utils.cpp (right):

https://codereview.adblockplus.org/29812649/diff/29812650/src/Utils.cpp#newco...
src/Utils.cpp:72: Utils::ToV8String(isolate, "Unknown
Exception").ToLocalChecked());
On 2018/06/22 00:34:31, hub wrote:
> Failure throwing an exception. Not good. So at least we should be fine. Note
> that since I pass a plain ASCII string, I expect the Maybe<> to not be empty.
I
> believe the assumption is correct.

I'm not sure how it works. Can we create a couple of tests for that?
The first test should call TerminateExecution and then try to
Utils::ThrowExceptionInJS.
The second one should cause the stack overflow and then try to
Utils::ThrowExceptionInJS.

[hub, 2018-06-22 15:50:56]

Updated patch.

Didn't yet attempt to write tests for ThrowExceptionInJS()

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsEngine.cpp#ne...
src/JsEngine.cpp:242: return JsValue(shared_from_this(),
v8::Undefined(GetIsolate()));
On 2018/06/22 06:57:46, sergei wrote:
> On 2018/06/22 00:34:31, hub wrote:
> > This is where I think we should break APIs to some extent. For now I return
> > `undefined` if there is an error. 
> 
> Let's just use CHECKED_TO_LOCAL.

Done.

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsValue.cpp#new...
src/JsValue.cpp:189: throw std::runtime_error("Invalid property name");
On 2018/06/22 06:57:46, sergei wrote:
> On 2018/06/22 00:34:31, hub wrote:
> > Since we already can throw, I thought it was OK to throw too. This is
> defintely
> > an error if we can make the key.
> 
> I think here we should use CHECKED_TO_LOCAL.

Done.

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsValue.cpp#new...
src/JsValue.cpp:193: return JsValue(jsEngine, v8::Undefined(isolate));
On 2018/06/22 06:57:46, sergei wrote:
> On 2018/06/22 00:34:31, hub wrote:
> > If the property isn't found we return `undefined`.
> 
> If the property is not found then the result of obj->Get(property) already
> contains v8::Undefined. However, if something is wrong with the engine, then
> maybe.IsEmpty() is true. So, IMO we should rather stop our execution as well.
> Could you please add a test here?

Done.

https://codereview.adblockplus.org/29812649/diff/29812650/src/JsValue.cpp#new...
src/JsValue.cpp:201: auto isolate = jsEngine->GetIsolate();
On 2018/06/22 06:57:46, sergei wrote:
> I think here and below we should use CHECKED_TO_LOCAL.

Done.

https://codereview.adblockplus.org/29812649/diff/29812650/src/Utils.cpp
File src/Utils.cpp (right):

https://codereview.adblockplus.org/29812649/diff/29812650/src/Utils.cpp#newco...
src/Utils.cpp:72: Utils::ToV8String(isolate, "Unknown
Exception").ToLocalChecked());
On 2018/06/22 06:57:46, sergei wrote:
> On 2018/06/22 00:34:31, hub wrote:
> > Failure throwing an exception. Not good. So at least we should be fine. Note
> > that since I pass a plain ASCII string, I expect the Maybe<> to not be
empty.
> I
> > believe the assumption is correct.
> 
> I'm not sure how it works. Can we create a couple of tests for that?
> The first test should call TerminateExecution and then try to
> Utils::ThrowExceptionInJS.
> The second one should cause the stack overflow and then try to
> Utils::ThrowExceptionInJS.

Right now, the gist is that if we can't obtain the UTF-8 then we have a default
string and still throw an exception. NewFromUtf8() will only return empty if
length is too large.

[sergei, 2018-07-05 08:39:00]

I think the only stopper here is the absence of tests.

https://codereview.adblockplus.org/29812649/diff/29812650/src/Utils.cpp
File src/Utils.cpp (right):

https://codereview.adblockplus.org/29812649/diff/29812650/src/Utils.cpp#newco...
src/Utils.cpp:72: Utils::ToV8String(isolate, "Unknown
Exception").ToLocalChecked());
On 2018/06/22 15:50:56, hub wrote:
> On 2018/06/22 06:57:46, sergei wrote:
> > On 2018/06/22 00:34:31, hub wrote:
> > > Failure throwing an exception. Not good. So at least we should be fine.
Note
> > > that since I pass a plain ASCII string, I expect the Maybe<> to not be
> empty.
> > I
> > > believe the assumption is correct.
> > 
> > I'm not sure how it works. Can we create a couple of tests for that?
> > The first test should call TerminateExecution and then try to
> > Utils::ThrowExceptionInJS.
> > The second one should cause the stack overflow and then try to
> > Utils::ThrowExceptionInJS.
> 
> Right now, the gist is that if we can't obtain the UTF-8 then we have a
default
> string and still throw an exception. NewFromUtf8() will only return empty if
> length is too large.

But it can be changed later and it can start to return an empty value if someone
called `TerminateExecution` from another thread.

https://codereview.adblockplus.org/29812649/diff/29813585/src/Utils.h
File src/Utils.h (right):

https://codereview.adblockplus.org/29812649/diff/29813585/src/Utils.h#newcode58
src/Utils.h:58: #define CHECKED_TO_LOCAL_NOTHROW(isolate, value) \
What about adding something like NOTRYCATCH instead of _NOTHROW? It's easy to
assume that NOTHROW is related to that macro/function, namely that it does not
throw, but actually it does throw, the difference is that it does not check
v8::TryCatch.

BTW, what do you think about swapping,
CHECKED_TO_LOCAL_NOTHROW->CHECKED_TO_LOCAL and CHECKED_TO_LOCAL_WITH_TRY_CATCH?
If we add something else later, or replace v8::TryCatch then it will be
reflected in the suffix CHECKED_TO_LOCAL_WITH_SOMETHING_ELSE.

[hub, 2018-07-11 02:11:02]

updated patch with tests.

https://codereview.adblockplus.org/29812649/diff/29813585/src/Utils.h
File src/Utils.h (right):

https://codereview.adblockplus.org/29812649/diff/29813585/src/Utils.h#newcode58
src/Utils.h:58: #define CHECKED_TO_LOCAL_NOTHROW(isolate, value) \
On 2018/07/05 08:39:00, sergei wrote:
> What about adding something like NOTRYCATCH instead of _NOTHROW? It's easy to
> assume that NOTHROW is related to that macro/function, namely that it does not
> throw, but actually it does throw, the difference is that it does not check
> v8::TryCatch.
> 
> BTW, what do you think about swapping,
> CHECKED_TO_LOCAL_NOTHROW->CHECKED_TO_LOCAL and
CHECKED_TO_LOCAL_WITH_TRY_CATCH?
> If we add something else later, or replace v8::TryCatch then it will be
> reflected in the suffix CHECKED_TO_LOCAL_WITH_SOMETHING_ELSE.

Done.

[sergei, 2018-08-06 12:18:31]

It seems LGTM, though could you please add to the issue description
- revisit ThrowExceptionInJS because failing throwing of an exception is not a
good thing
- add actual tests simulating the state of the isolate when it starts to return
empty values.

https://codereview.adblockplus.org/29812649/diff/29826601/src/Utils.h
File src/Utils.h (right):

https://codereview.adblockplus.org/29812649/diff/29826601/src/Utils.h#newcode56
src/Utils.h:56: AdblockPlus::Utils::CheckedToLocal(isolate, value, &tryCatch,
__FILE__, __LINE__)
I wonder whether we should use all macro arguments in parentheses ().

[hub, 2018-08-06 13:25:48]

Updated patch

https://codereview.adblockplus.org/29812649/diff/29826601/src/Utils.h
File src/Utils.h (right):

https://codereview.adblockplus.org/29812649/diff/29826601/src/Utils.h#newcode56
src/Utils.h:56: AdblockPlus::Utils::CheckedToLocal(isolate, value, &tryCatch,
__FILE__, __LINE__)
On 2018/08/06 12:18:30, sergei wrote:
> I wonder whether we should use all macro arguments in parentheses ().

I'll do that. It is safer.

[sergei, 2018-08-06 13:53:46]

LGTM