bindings: Use Maybe APIs in V8ScriptRunner (part 1)

bindings: Use Maybe APIs in V8ScriptRunner (part 1)

Replacing deprecating Compile() and Run() APIs.

BUG=462402
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192226

[bashi, 2015-03-13 02:55:25]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org

[bashi, 2015-03-13 02:55:25]

PTAL?

[haraken, 2015-03-13 04:31:16]

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/Pri...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/Pri...
Source/bindings/core/v8/PrivateScriptRunner.cpp:65: if (block.HasCaught())

Just to confirm: When a worker termination is signaled and the compileScript
returns false, V8 does not throw any exception, right? Is it an expected
behavior?

I'm asking this because except for the worker termination, it is guaranteed that
the compileScript returns false if and only if V8 throws an exception. In other
words, except for the worker termination, block.HasCaught() is guaranteed to be
true here.

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptStreamerTest.cpp (right):

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptStreamerTest.cpp:165: v8::Handle<v8::Script>
script = V8ScriptRunner::compileScript(sourceCode, isolate()).ToLocalChecked();

Can we use ToLocal? (i.e., we might not want to introduce ToLocalChecked to
bindings/.) The following script.IsEmpty() would be enough.

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/V8S...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/V8S...
Source/bindings/core/v8/V8ScriptRunner.cpp:105: v8::MaybeLocal<v8::Script>
compileWithoutOptions(V8CompileHistogram::Cacheability cacheability,
v8::Isolate* isolate, v8::Handle<v8::String> code, v8::ScriptOrigin origin)

oh... we cannot avoid introducing MaybeLocal in the Blink code base...

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/cus...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/cus...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:411: if
(!tryCatch.CanContinue())

What is this check for?

[haraken, 2015-03-13 04:34:28]

Overall I might want to clarify the following two points before proceeding with
the work:

a) if there is a way to avoid MaybeLocal in the Blink code base.

b) the relation ship between the condition where a V8 API returns false and the
condition where V8 throws an exception.

[Yuki, 2015-03-13 04:39:41]

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptStreamerTest.cpp (right):

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptStreamerTest.cpp:165: v8::Handle<v8::Script>
script = V8ScriptRunner::compileScript(sourceCode, isolate()).ToLocalChecked();
optional:

Reading v8.h:
  V8_INLINE Local<T> ToLocalChecked() {
    // TODO(dcarney): add DCHECK.
    return Local<T>(val_);
  }

ToLocalChecked should be named ToLocalDChecked or something like that.  It seems
not going to be checked in case of release build.

You may want to RELEASE_ASSERT version of ToLocalChecked, too.
You may send feedback to V8 team about this point.

[bashi, 2015-03-13 05:15:20]

> a) if there is a way to avoid MaybeLocal in the Blink code base.              
                              

As commented above, we could but I don't think we should. All blink methods
which just return a result of V8 APIs should use MaybeLocal. Callers need to
check whether the return value is empty anyway. This means that we should       
                           
try not to expose these APIs.

> b) the relation ship between the condition where a V8 API returns false and
the
> condition where V8 throws an exception.

This question is difficult to answer without knowing V8 internals, and could be
changed as the V8 teams change thier design. I don't think we should depend on
that, so I'd like to assume that 'there is no guarantee that there is an
exception
when V8 APIs fails'. This means that if we want to handle exception, we should
check HadException() when V8 APIs fails. (I think ToLocal() never succeed when
there is an exception, so we don't need to check HadException() when ToLocal()
succeed)

> You may want to RELEASE_ASSERT version of ToLocalChecked, too.
> You may send feedback to V8 team about this point.

^^^
@dcarney

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/Pri...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/Pri...
Source/bindings/core/v8/PrivateScriptRunner.cpp:65: if (block.HasCaught())
On 2015/03/13 04:31:16, haraken wrote:
> 
> Just to confirm: When a worker termination is signaled and the compileScript
> returns false, V8 does not throw any exception, right? Is it an expected
> behavior?
> 
> I'm asking this because except for the worker termination, it is guaranteed
that
> the compileScript returns false if and only if V8 throws an exception. In
other
> words, except for the worker termination, block.HasCaught() is guaranteed to
be
> true here.

I'm not fully confident, but I think you are right. Changed to use ASSERT().

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptStreamerTest.cpp (right):

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptStreamerTest.cpp:165: v8::Handle<v8::Script>
script = V8ScriptRunner::compileScript(sourceCode, isolate()).ToLocalChecked();
On 2015/03/13 04:31:16, haraken wrote:
> 
> Can we use ToLocal? (i.e., we might not want to introduce ToLocalChecked to
> bindings/.) The following script.IsEmpty() would be enough.

Done.

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/V8S...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/V8S...
Source/bindings/core/v8/V8ScriptRunner.cpp:105: v8::MaybeLocal<v8::Script>
compileWithoutOptions(V8CompileHistogram::Cacheability cacheability,
v8::Isolate* isolate, v8::Handle<v8::String> code, v8::ScriptOrigin origin)
On 2015/03/13 04:31:16, haraken wrote:
> 
> oh... we cannot avoid introducing MaybeLocal in the Blink code base...

Unfortunately, yeah..

IMHO, these methods should return MaybeLocal. We can use Local, but callers need
to check IsEmpty() anyway. Using MaybeLocal, we can make sure that callers do
empty checks.

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/cus...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/1003043002/diff/1/Source/bindings/core/v8/cus...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:411: if
(!tryCatch.CanContinue())
On 2015/03/13 04:31:16, haraken wrote:
> 
> What is this check for?

I think when this is true, following V8 APIs will fail (e.g. v8::Object::New()),
which may cause mysterious crashes.

[bashi, 2015-03-13 05:38:12]

@dcarney,

> check HadException() when V8 APIs fails. (I think ToLocal() never succeed when
> there is an exception, so we don't need to check HadException() when ToLocal()
> succeed)

I'd like to confirm that my understanding is correct: MaybeLocal is just a
wrapper, but APIs which return MaybeLocal always return an empty MaybeLocal when
there is an exception (normal exception, termination signal, whatever). Can we
assume this?

[Yuki, 2015-03-13 08:06:44]

lgtm

[dcarney, 2015-03-13 08:14:35]

On 2015/03/13 05:38:12, bashi1 wrote:
> @dcarney,
> 
> > check HadException() when V8 APIs fails. (I think ToLocal() never succeed
when
> > there is an exception, so we don't need to check HadException() when
ToLocal()
> > succeed)
> 
> I'd like to confirm that my understanding is correct: MaybeLocal is just a
> wrapper, but APIs which return MaybeLocal always return an empty MaybeLocal
when
> there is an exception (normal exception, termination signal, whatever). Can we
> assume this?

It's almost true everywhere.  There are a few places where it's not, and I'm
currently trying to find a way to make it obvious.

[dcarney, 2015-03-13 08:15:32]

On 2015/03/13 04:34:28, haraken wrote:
> Overall I might want to clarify the following two points before proceeding
with
> the work:
> 
> a) if there is a way to avoid MaybeLocal in the Blink code base.

You definitely want to propagate MaybeLocal backwards.

[dcarney, 2015-03-13 08:15:45]

dcarney@chromium.org changed reviewers:
+ dcarney@chromium.org

[dcarney, 2015-03-13 08:15:46]

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/NPV8Object.cpp (right):

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/NPV8Object.cpp:559: if
(!V8ScriptRunner::compileAndRunInternalScript(source, isolate).ToLocal(&result))
use macros to remove the if(!whatever) block in most places.  if you don't the
code will quickly become unreadable. we had the same problem in the v8 runtime

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptDebugServer.cpp:619: ASSERT_NOT_REACHED();
In cases like this you want ToLocalChecked instead

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:383: if
(tryCatch.HasCaught())
use a macro for this pattern

[bashi, 2015-03-13 08:33:32]

On 2015/03/13 08:14:35, dcarney wrote:
> On 2015/03/13 05:38:12, bashi1 wrote:
> > @dcarney,
> > 
> > > check HadException() when V8 APIs fails. (I think ToLocal() never succeed
> when
> > > there is an exception, so we don't need to check HadException() when
> ToLocal()
> > > succeed)
> > 
> > I'd like to confirm that my understanding is correct: MaybeLocal is just a
> > wrapper, but APIs which return MaybeLocal always return an empty MaybeLocal
> when
> > there is an exception (normal exception, termination signal, whatever). Can
we
> > assume this?
> 
> It's almost true everywhere.  There are a few places where it's not, and I'm
> currently trying to find a way to make it obvious.

Thanks! I'll address your comments later. In summary:
- ToLocal() succeeds -> no HasCaught() check needed. (with very few exceptions,
and you are working on such exceptions)
- ToLocal() fails -> There *may* be an exception. HasCaught() may be true, or
may be false. We need to check HasCaught() when we want to use block.Exception()
or rethrow it.

Is that correct? I assume this can be said for Maybe<T> APIs.

[dcarney, 2015-03-13 08:39:09]

> Thanks! I'll address your comments later. In summary:
> - ToLocal() succeeds -> no HasCaught() check needed. (with very few
exceptions,
> and you are working on such exceptions)
> - ToLocal() fails -> There *may* be an exception. HasCaught() may be true, or
> may be false. We need to check HasCaught() when we want to use
block.Exception()
> or rethrow it.
> 
> Is that correct? I assume this can be said for Maybe<T> APIs.


almost
ToLocal succeeds -> there is never an exception thrown. There are no known
exceptions to this rule currently and none will be created.
ToLocal fails -> there are 5 or 6 functions currently where we don't throw and
i'm fixing those. it's more or less safe to assume HasCaught().  You probably
want to DCHECK that this is true so we can flush out any missing cases.

[bashi, 2015-03-13 08:42:20]

On 2015/03/13 08:39:09, dcarney wrote:
> > Thanks! I'll address your comments later. In summary:
> > - ToLocal() succeeds -> no HasCaught() check needed. (with very few
> exceptions,
> > and you are working on such exceptions)
> > - ToLocal() fails -> There *may* be an exception. HasCaught() may be true,
or
> > may be false. We need to check HasCaught() when we want to use
> block.Exception()
> > or rethrow it.
> > 
> > Is that correct? I assume this can be said for Maybe<T> APIs.
> 
> 
> almost
> ToLocal succeeds -> there is never an exception thrown. There are no known
> exceptions to this rule currently and none will be created.
> ToLocal fails -> there are 5 or 6 functions currently where we don't throw and
> i'm fixing those. it's more or less safe to assume HasCaught().  You probably
> want to DCHECK that this is true so we can flush out any missing cases.

That explains exactly what I should do. Thanks!

[bashi, 2015-03-17 02:01:36]

Could you take another look?

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/NPV8Object.cpp (right):

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/NPV8Object.cpp:559: if
(!V8ScriptRunner::compileAndRunInternalScript(source, isolate).ToLocal(&result))
On 2015/03/13 08:15:45, dcarney wrote:
> use macros to remove the if(!whatever) block in most places.  if you don't the
> code will quickly become unreadable. we had the same problem in the v8 runtime

We discussed how we handle ToLocal failure, and we reached a consensus that we
don't use macros. https://codereview.chromium.org/970403003/

Macros can't improve readability these cases.

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptDebugServer.cpp:619: ASSERT_NOT_REACHED();
On 2015/03/13 08:15:45, dcarney wrote:
> In cases like this you want ToLocalChecked instead

Done. Please add DCHECK() in ToLocalChecked.

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:383: if
(tryCatch.HasCaught())
On 2015/03/13 08:15:46, dcarney wrote:
> use a macro for this pattern

Done.

https://codereview.chromium.org/1003043002/diff/140001/Source/bindings/core/v...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/1003043002/diff/140001/Source/bindings/core/v...
Source/bindings/core/v8/V8ScriptRunner.cpp:412: result =
script->Run(isolate->GetCurrentContext());
It seems that Run() is an instance of APIs which can fail withouth throwing an
exception.

[haraken, 2015-03-17 02:11:01]

Just to confirm: In the near future, tryCatch.HasCaught() is going to be true if
and only if the V8 API returns false. At that point we can remove the
unnecessary HasCaught checks from the binding code base, right?

[bashi, 2015-03-17 02:20:51]

On 2015/03/17 02:11:01, haraken wrote:
> Just to confirm: In the near future, tryCatch.HasCaught() is going to be true
if
> and only if the V8 API returns false. At that point we can remove the
> unnecessary HasCaught checks from the binding code base, right?

That's my understanding, but @dcarney should confirm.

[dcarney, 2015-03-17 06:29:14]

On 2015/03/17 02:01:36, bashi1 wrote:
> Could you take another look?
> 
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> File Source/bindings/core/v8/NPV8Object.cpp (right):
> 
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> Source/bindings/core/v8/NPV8Object.cpp:559: if
> (!V8ScriptRunner::compileAndRunInternalScript(source,
isolate).ToLocal(&result))
> On 2015/03/13 08:15:45, dcarney wrote:
> > use macros to remove the if(!whatever) block in most places.  if you don't
the
> > code will quickly become unreadable. we had the same problem in the v8
runtime
> 
> We discussed how we handle ToLocal failure, and we reached a consensus that we
> don't use macros. https://codereview.chromium.org/970403003/
> 
> Macros can't improve readability these cases.
> 
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> File Source/bindings/core/v8/ScriptDebugServer.cpp (right):
> 
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> Source/bindings/core/v8/ScriptDebugServer.cpp:619: ASSERT_NOT_REACHED();
> On 2015/03/13 08:15:45, dcarney wrote:
> > In cases like this you want ToLocalChecked instead
> 
> Done. Please add DCHECK() in ToLocalChecked.
> 
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):
> 
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:383: if
> (tryCatch.HasCaught())
> On 2015/03/13 08:15:46, dcarney wrote:
> > use a macro for this pattern
> 
> Done.
> 
>
https://codereview.chromium.org/1003043002/diff/140001/Source/bindings/core/v...
> File Source/bindings/core/v8/V8ScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/1003043002/diff/140001/Source/bindings/core/v...
> Source/bindings/core/v8/V8ScriptRunner.cpp:412: result =
> script->Run(isolate->GetCurrentContext());
> It seems that Run() is an instance of APIs which can fail withouth throwing an
> exception.

I don't think run fails without an exception.

[haraken, 2015-03-17 08:17:09]

On 2015/03/17 02:11:01, haraken wrote:
> Just to confirm: In the near future, tryCatch.HasCaught() is going to be true
if
> and only if the V8 API returns false. At that point we can remove the
> unnecessary HasCaught checks from the binding code base, right?

Dan: ^^^ Is my understanding above correct?

[dcarney, 2015-03-17 08:17:39]

On 2015/03/17 08:17:09, haraken wrote:
> On 2015/03/17 02:11:01, haraken wrote:
> > Just to confirm: In the near future, tryCatch.HasCaught() is going to be
true
> if
> > and only if the V8 API returns false. At that point we can remove the
> > unnecessary HasCaught checks from the binding code base, right?
> 
> Dan: ^^^ Is my understanding above correct?

yes

[bashi, 2015-03-18 00:09:00]

On 2015/03/17 06:29:14, dcarney wrote:
> On 2015/03/17 02:01:36, bashi1 wrote:
> > Could you take another look?
> > 
> >
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> > File Source/bindings/core/v8/NPV8Object.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> > Source/bindings/core/v8/NPV8Object.cpp:559: if
> > (!V8ScriptRunner::compileAndRunInternalScript(source,
> isolate).ToLocal(&result))
> > On 2015/03/13 08:15:45, dcarney wrote:
> > > use macros to remove the if(!whatever) block in most places.  if you don't
> the
> > > code will quickly become unreadable. we had the same problem in the v8
> runtime
> > 
> > We discussed how we handle ToLocal failure, and we reached a consensus that
we
> > don't use macros. https://codereview.chromium.org/970403003/
> > 
> > Macros can't improve readability these cases.
> > 
> >
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> > File Source/bindings/core/v8/ScriptDebugServer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> > Source/bindings/core/v8/ScriptDebugServer.cpp:619: ASSERT_NOT_REACHED();
> > On 2015/03/13 08:15:45, dcarney wrote:
> > > In cases like this you want ToLocalChecked instead
> > 
> > Done. Please add DCHECK() in ToLocalChecked.
> > 
> >
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> > File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1003043002/diff/40001/Source/bindings/core/v8...
> > Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:383: if
> > (tryCatch.HasCaught())
> > On 2015/03/13 08:15:46, dcarney wrote:
> > > use a macro for this pattern
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1003043002/diff/140001/Source/bindings/core/v...
> > File Source/bindings/core/v8/V8ScriptRunner.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1003043002/diff/140001/Source/bindings/core/v...
> > Source/bindings/core/v8/V8ScriptRunner.cpp:412: result =
> > script->Run(isolate->GetCurrentContext());
> > It seems that Run() is an instance of APIs which can fail withouth throwing
an
> > exception.
> 
> I don't think run fails without an exception.

You are right. I wrote a function which returns an empty MaybeLocal without
throwing an exception, which was wrong.

[haraken, 2015-03-18 01:58:51]

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptDebugServer.cpp:617: v8::Local<v8::Value> value =
V8ScriptRunner::compileAndRunInternalScript(source, m_isolate).ToLocalChecked();

Wouldn't it be better to use ToLocal and return early if the value is empty?

I'm asking this because in V8InjectedScriptManager.cpp, you don't use
ToLocalChecked but use ToLocal & early return. Given that any
compileAndRunInternalScript can potentially fail (e.g., if a worker termination
is requested), I'd prefer using early-return always (and avoid using
ToLocalChecked).

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptDebugServer.cpp:742: if (!tryCatch.HasCaught())

Can this happen?

In V8BindingMacros.h, you're adding ASSERT(success || tryCatch.HasCaught()),
which means that tryCatch.HasCaught() is guaranteed to be true here.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/V8ScriptRunner.cpp (left):

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/V8ScriptRunner.cpp:396: if (script.IsEmpty())

Why can we change this to ASSERT?

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/V8ScriptRunner.cpp:85: // throwStackOverflowException
and call it.

Not related to your CL, this comment doesn't make much sense... here we're not
creating a v8::Function nor calling it...

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/V8ScriptRunner.cpp:414:
throwScriptForbiddenException(isolate);

Shall we make this change in a separate CL? I'm not sure if it is a right thing
to throw an exception in this case.

If I'm understanding correctly, we must not hit this branch. So this should be
RELEASE_ASSERT_NOT_REACHED(). However, we sometimes hit this branch in reality
(this is a bug to be fixed), so we just return an empty value instead of
crashing. But I'm not sure.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/V8ScriptRunner.cpp:460:
throwScriptForbiddenException(isolate);

Ditto.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:410: if
(!tryCatch.CanContinue())

Sorry for a repeated question, but when do we need CanContinue and when do we
not? I'm wondering why we have CanContinue here but don't in other places.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:416: } else {

Is it possible to hit this branch?

Given that you added ASSERT(success||tryCatch.HasCaught()) to V8BindingMacros.h,
I think we always hit the above tryCatch.HasCaught() branch.

[bashi, 2015-03-18 02:33:12]

Thank you for review!

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptDebugServer.cpp:617: v8::Local<v8::Value> value =
V8ScriptRunner::compileAndRunInternalScript(source, m_isolate).ToLocalChecked();
On 2015/03/18 01:58:51, haraken wrote:
> 
> Wouldn't it be better to use ToLocal and return early if the value is empty?
> 
> I'm asking this because in V8InjectedScriptManager.cpp, you don't use
> ToLocalChecked but use ToLocal & early return. Given that any
> compileAndRunInternalScript can potentially fail (e.g., if a worker
termination
> is requested), I'd prefer using early-return always (and avoid using
> ToLocalChecked).
> 

Will do.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptDebugServer.cpp:742: if (!tryCatch.HasCaught())
On 2015/03/18 01:58:51, haraken wrote:
> 
> Can this happen?
> 
> In V8BindingMacros.h, you're adding ASSERT(success || tryCatch.HasCaught()),
> which means that tryCatch.HasCaught() is guaranteed to be true here.

Oh, I forgot to remove this. Will remove.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/V8ScriptRunner.cpp (left):

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/V8ScriptRunner.cpp:396: if (script.IsEmpty())
On 2015/03/18 01:58:51, haraken wrote:
> 
> Why can we change this to ASSERT?

I should have mentioned about this. I changed the contract of this method so
that script should always be non-empty. For consistency, I'd like to make all
methods which return Maybe throw an exception on failure. But I didn't come up
with what kind of exception should be return when script is empty. Instead of
throwing exceptions, I think it'd be better not to accept empty script.

Every call site checks IsEmpty() before.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/V8ScriptRunner.cpp:85: // throwStackOverflowException
and call it.
On 2015/03/18 01:58:51, haraken wrote:
> 
> Not related to your CL, this comment doesn't make much sense... here we're not
> creating a v8::Function nor calling it...

I guess this comment explains what throwStackOverflowExceptionIfNeeded() does.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/V8ScriptRunner.cpp:414:
throwScriptForbiddenException(isolate);
On 2015/03/18 01:58:51, haraken wrote:
> 
> Shall we make this change in a separate CL? I'm not sure if it is a right
thing
> to throw an exception in this case.
> 
> If I'm understanding correctly, we must not hit this branch. So this should be
> RELEASE_ASSERT_NOT_REACHED(). However, we sometimes hit this branch in reality
> (this is a bug to be fixed), so we just return an empty value instead of
> crashing. But I'm not sure.

This means that you want to allow methods which return Maybe fail without
throwing exceptions, which you said you didn't want :)

I'm fine with either, but if not throwing exceptions here, we need to have

if (!methodReturnsMaybe(...).ToLocal(&value)) {
  if (tryCatch.HasCaught()) { ... }
}

checks.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:410: if
(!tryCatch.CanContinue())
On 2015/03/18 01:58:51, haraken wrote:
> 
> Sorry for a repeated question, but when do we need CanContinue and when do we
> not? I'm wondering why we have CanContinue here but don't in other places.
> 

Other places return immediately on failure, but this isn't the case, and calls
V8 APIs, which may fail. No?

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:416: } else {
On 2015/03/18 01:58:51, haraken wrote:
> 
> Is it possible to hit this branch?
> 
> Given that you added ASSERT(success||tryCatch.HasCaught()) to
V8BindingMacros.h,
> I think we always hit the above tryCatch.HasCaught() branch.

If v8ScriptRunner::runCompiledScript() succeeds, we reach this branch.
If fails, we reach L414.

[haraken, 2015-03-18 08:27:37]

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:410: if
(!tryCatch.CanContinue())
On 2015/03/18 02:33:12, bashi1 wrote:
> On 2015/03/18 01:58:51, haraken wrote:
> > 
> > Sorry for a repeated question, but when do we need CanContinue and when do
we
> > not? I'm wondering why we have CanContinue here but don't in other places.
> > 
> 
> Other places return immediately on failure, but this isn't the case, and calls
> V8 APIs, which may fail. No?

Hmm, I'm not sure of how helpful the CanContinue check is. Even if we have the
CanContinue check, we'll crash somewhere unexpected because the binding code is
not written in a can-continue-aware manner. If we don't have the check, we'll
crash in the next V8 API. I'm not sure if the former is improving the situation.

I think this is a separate issue we need to address in the future.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:413: if
(tryCatch.HasCaught()) {

Also this HasCaught check is a bit too late. The Object::New might cause a GC,
which may set another exception. Basically HasCaught must be called immediately
after the V8 API you called.

[bashi, 2015-03-18 08:43:47]

Patchset #11 (id:200001) has been deleted

[bashi, 2015-03-18 08:46:48]

Patchset #11 (id:220001) has been deleted

[bashi, 2015-03-18 09:04:10]

Patchset #11 (id:240001) has been deleted

[bashi, 2015-03-20 01:16:07]

Revised the CL. Could you take another look?

As chatted offline, I'd like to make following rules for a blink method which
returns V8 Maybe API's result.
- It returns Maybe{Local}.
- If it fails, an exception is always thrown.

This matches V8 API's behavior. This way, call sites don't need to have
tryCatch.HasCought() checks.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:410: if
(!tryCatch.CanContinue())
On 2015/03/18 08:27:37, haraken wrote:
> On 2015/03/18 02:33:12, bashi1 wrote:
> > On 2015/03/18 01:58:51, haraken wrote:
> > > 
> > > Sorry for a repeated question, but when do we need CanContinue and when do
> we
> > > not? I'm wondering why we have CanContinue here but don't in other places.
> > > 
> > 
> > Other places return immediately on failure, but this isn't the case, and
calls
> > V8 APIs, which may fail. No?
> 
> Hmm, I'm not sure of how helpful the CanContinue check is. Even if we have the
> CanContinue check, we'll crash somewhere unexpected because the binding code
is
> not written in a can-continue-aware manner. If we don't have the check, we'll
> crash in the next V8 API. I'm not sure if the former is improving the
situation.
> 
> I think this is a separate issue we need to address in the future.

OK, removed.

https://codereview.chromium.org/1003043002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:413: if
(tryCatch.HasCaught()) {
On 2015/03/18 08:27:37, haraken wrote:
> 
> Also this HasCaught check is a bit too late. The Object::New might cause a GC,
> which may set another exception. Basically HasCaught must be called
immediately
> after the V8 API you called.

Changed the current implementation so that we don't continue when APIs fail.

[haraken, 2015-03-20 01:38:56]

LGTM

https://codereview.chromium.org/1003043002/diff/280001/Source/bindings/core/v...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/1003043002/diff/280001/Source/bindings/core/v...
Source/bindings/core/v8/V8ScriptRunner.cpp:429:
throwScriptForbiddenException(isolate);

Just to confirm: If we forget to throw this exception, the error will be caught
by the ASSERT in the v8Call macro written in the caller side of
V8ScriptRunner::runCompiledScript. Right?

[bashi, 2015-03-20 02:29:32]

On 2015/03/20 01:38:56, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/1003043002/diff/280001/Source/bindings/core/v...
> File Source/bindings/core/v8/V8ScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/1003043002/diff/280001/Source/bindings/core/v...
> Source/bindings/core/v8/V8ScriptRunner.cpp:429:
> throwScriptForbiddenException(isolate);
> 
> Just to confirm: If we forget to throw this exception, the error will be
caught
> by the ASSERT in the v8Call macro written in the caller side of
> V8ScriptRunner::runCompiledScript. Right?

Yes. That's why I need to throw an exception here.

[bashi, 2015-03-20 02:30:05]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-03-20 02:30:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/1003043002/#ps280001
(title: " ")

[commit-bot: I haz the power, 2015-03-20 02:30:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1003043002/280001

[commit-bot: I haz the power, 2015-03-20 05:26:29]

Committed patchset #12 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192226