bindings: Use Maybe version of v8::String::NewFromUtf8()

bindings: Use Maybe version of v8::String::NewFromUtf8()

Also add v8NormalString() which returns the result of
v8::String::NewFromUtf8(). This function ensures that the
value is not empty. Replace existing v8::String::NewFromUtf8()
calls with this function.

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

[bashi, 2015-03-20 08:16:44]

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

[bashi, 2015-03-20 08:16:45]

PTAL? No hurry, I won't reply review comments until Monday :)

https://codereview.chromium.org/1027593003/diff/1/Source/bindings/core/v8/V8B...
File Source/bindings/core/v8/V8Binding.h (left):

https://codereview.chromium.org/1027593003/diff/1/Source/bindings/core/v8/V8B...
Source/bindings/core/v8/V8Binding.h:350: inline v8::Handle<v8::String>
v8AtomicString(v8::Isolate* isolate, const char* str)
I think we can remove this. When length is -1, v8::String::NewFromUtf8() calls
StringLength(), which should be the same as calling strlen().

https://codereview.chromium.org/1027593003/diff/1/Source/bindings/core/v8/V8B...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1027593003/diff/1/Source/bindings/core/v8/V8B...
Source/bindings/core/v8/V8Binding.h:354: if
(LIKELY(v8::String::NewFromUtf8(isolate, str, v8::NewStringType::kNormal,
length).ToLocal(&value)))
I added LIKELY() because according to the v8.h, NewFromUtf8() only fails when
|length| is too long. But this might be unnecessary.

[haraken, 2015-03-20 09:46:08]

LGTM

https://codereview.chromium.org/1027593003/diff/1/Source/bindings/core/v8/V8B...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1027593003/diff/1/Source/bindings/core/v8/V8B...
Source/bindings/core/v8/V8Binding.h:354: if
(LIKELY(v8::String::NewFromUtf8(isolate, str, v8::NewStringType::kNormal,
length).ToLocal(&value)))
On 2015/03/20 08:16:44, bashi1 wrote:
> I added LIKELY() because according to the v8.h, NewFromUtf8() only fails when
> |length| is too long. But this might be unnecessary.

v8NormalString is performance-sensitive, so LIKELY would make sense. Rather I'm
a bit concerned that this CL might regress performance of some micro benchmarks.
Let's land this and see the perf bots.

[yurys, 2015-03-20 11:00:46]

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

[yurys, 2015-03-20 11:00:47]

Can you please revert changes in the following files in favor of using v8.h APIs
directly?
ScriptDebugServer.cpp
V8InjectedScriptHostCustom.cpp
JavaScriptCallFrame.cpp

The rationale for this is that these files are going to be moved out of Blink
closer to v8, hence these files shouldn't depend on bindings code. See [1] for
more details.

https://docs.google.com/document/d/1fEkZFMH_U5DhIYM95Mhp5ovtBUeHaToSvgQapDzgN...

[yurys, 2015-03-20 11:28:13]

The CQ bit was checked by yurys@chromium.org

[yurys, 2015-03-20 11:28:16]

The CQ bit was unchecked by yurys@chromium.org

[bashi, 2015-03-23 01:26:26]

On 2015/03/20 11:00:47, yurys wrote:
> Can you please revert changes in the following files in favor of using v8.h
APIs
> directly?
> ScriptDebugServer.cpp
> V8InjectedScriptHostCustom.cpp
> JavaScriptCallFrame.cpp
> 
> The rationale for this is that these files are going to be moved out of Blink
> closer to v8, hence these files shouldn't depend on bindings code. See [1] for
> more details.
> 
>
https://docs.google.com/document/d/1fEkZFMH_U5DhIYM95Mhp5ovtBUeHaToSvgQapDzgN...

Sure. Will do tomorrow (I'm ooo today). I think it would be better to use new
APIs rather than just reverting them. WDYT?

[yurys, 2015-03-23 07:29:06]

On 2015/03/23 01:26:26, bashi1 wrote:
> On 2015/03/20 11:00:47, yurys wrote:
> > Can you please revert changes in the following files in favor of using v8.h
> APIs
> > directly?
> > ScriptDebugServer.cpp
> > V8InjectedScriptHostCustom.cpp
> > JavaScriptCallFrame.cpp
> > 
> > The rationale for this is that these files are going to be moved out of
Blink
> > closer to v8, hence these files shouldn't depend on bindings code. See [1]
for
> > more details.
> > 
> >
>
https://docs.google.com/document/d/1fEkZFMH_U5DhIYM95Mhp5ovtBUeHaToSvgQapDzgN...
> 
> Sure. Will do tomorrow (I'm ooo today). I think it would be better to use new
> APIs rather than just reverting them. WDYT?

[yurys, 2015-03-23 07:30:26]

On 2015/03/23 07:29:06, yurys wrote:
> On 2015/03/23 01:26:26, bashi1 wrote:
> > On 2015/03/20 11:00:47, yurys wrote:
> > > Can you please revert changes in the following files in favor of using
v8.h
> > APIs
> > > directly?
> > > ScriptDebugServer.cpp
> > > V8InjectedScriptHostCustom.cpp
> > > JavaScriptCallFrame.cpp
> > > 
> > > The rationale for this is that these files are going to be moved out of
> Blink
> > > closer to v8, hence these files shouldn't depend on bindings code. See [1]
> for
> > > more details.
> > > 
> > >
> >
>
https://docs.google.com/document/d/1fEkZFMH_U5DhIYM95Mhp5ovtBUeHaToSvgQapDzgN...
> > 
> > Sure. Will do tomorrow (I'm ooo today). I think it would be better to use
new
> > APIs rather than just reverting them. WDYT?

If the old ones are going to be removed soon, then sure, we can switch this code
to Maybe* versions.

[Yuki, 2015-03-23 08:52:51]

It seems a little confusing that we have following three functions.
- v8String()
- v8NormalString()   // NEW
- v8AtomicString()

What v8AtomicString is is crystal clear, however, the difference between
v8String and v8NormalString is not that clear.  v8String is NOT normal???

IIUC, v8String uses a cache to make the conversion as much as fast, while
v8NormalString simply calls v8 APIs.

I'd recommend to rename v8String and v8NormalString to represent what they are
and what they are not.  Without reading the implementation, I cannot tell which
I should use.

Function header comments are welcome, too.

[haraken, 2015-03-23 08:55:14]

On 2015/03/23 08:52:51, Yuki wrote:
> It seems a little confusing that we have following three functions.
> - v8String()
> - v8NormalString()   // NEW
> - v8AtomicString()
> 
> What v8AtomicString is is crystal clear, however, the difference between
> v8String and v8NormalString is not that clear.  v8String is NOT normal???
> 
> IIUC, v8String uses a cache to make the conversion as much as fast, while
> v8NormalString simply calls v8 APIs.
> 
> I'd recommend to rename v8String and v8NormalString to represent what they are
> and what they are not.  Without reading the implementation, I cannot tell
which
> I should use.
> 
> Function header comments are welcome, too.

Good point!

BTW, do we really need v8NormalString? I understand that v8NormalString and
v8String are different, but I'm not sure if the difference matters for
performance. If not, we can just replace v8NormalString with v8String.

[bashi, 2015-03-23 23:42:31]

On 2015/03/23 08:55:14, haraken wrote:
> On 2015/03/23 08:52:51, Yuki wrote:
> > It seems a little confusing that we have following three functions.
> > - v8String()
> > - v8NormalString()   // NEW
> > - v8AtomicString()
> > 
> > What v8AtomicString is is crystal clear, however, the difference between
> > v8String and v8NormalString is not that clear.  v8String is NOT normal???
> > 
> > IIUC, v8String uses a cache to make the conversion as much as fast, while
> > v8NormalString simply calls v8 APIs.
> > 
> > I'd recommend to rename v8String and v8NormalString to represent what they
are
> > and what they are not.  Without reading the implementation, I cannot tell
> which
> > I should use.
> > 
> > Function header comments are welcome, too.
> 
> Good point!
> 
> BTW, do we really need v8NormalString? I understand that v8NormalString and
> v8String are different, but I'm not sure if the difference matters for
> performance. If not, we can just replace v8NormalString with v8String.

Added a comment on v8NormalString(). I think we should have v8NormalString()
because it doesn't require String. Many call sites just passing C++'s string
literals, which means that we create a temporary String instance unnecessary.

As for naming, I'm going to rename v8String -> v8SharedString. This will touch
many files, so I'll do that in a follow-up CL.

[bashi, 2015-03-23 23:54:01]

On 2015/03/23 07:30:26, yurys wrote:
> On 2015/03/23 07:29:06, yurys wrote:
> > On 2015/03/23 01:26:26, bashi1 wrote:
> > > On 2015/03/20 11:00:47, yurys wrote:
> > > > Can you please revert changes in the following files in favor of using
> v8.h
> > > APIs
> > > > directly?
> > > > ScriptDebugServer.cpp
> > > > V8InjectedScriptHostCustom.cpp
> > > > JavaScriptCallFrame.cpp
> > > > 
> > > > The rationale for this is that these files are going to be moved out of
> > Blink
> > > > closer to v8, hence these files shouldn't depend on bindings code. See
[1]
> > for
> > > > more details.
> > > > 
> > > >
> > >
> >
>
https://docs.google.com/document/d/1fEkZFMH_U5DhIYM95Mhp5ovtBUeHaToSvgQapDzgN...
> > > 
> > > Sure. Will do tomorrow (I'm ooo today). I think it would be better to use
> new
> > > APIs rather than just reverting them. WDYT?
> 
> If the old ones are going to be removed soon, then sure, we can switch this
code
> to Maybe* versions.

Just reverted because new APIs returns MaybeLocal, and without our macros, code
will mess up.

BTW, this kind of special treatment makes me very frustrated. I change
V8_DEPRECATE_SOON -> V8_DEPRECATED to verify we don't use old APIs, but now I
can't use this method. Also, I can't semi-automatic grep-and-replace because
some files are placed outside of core/inspector (e.g. ScriptDebugServer).

yurys@, do you have any ideas how to deal with these problem?

[haraken, 2015-03-24 00:51:17]

On 2015/03/23 23:42:31, bashi1 wrote:
> On 2015/03/23 08:55:14, haraken wrote:
> > On 2015/03/23 08:52:51, Yuki wrote:
> > > It seems a little confusing that we have following three functions.
> > > - v8String()
> > > - v8NormalString()   // NEW
> > > - v8AtomicString()
> > > 
> > > What v8AtomicString is is crystal clear, however, the difference between
> > > v8String and v8NormalString is not that clear.  v8String is NOT normal???
> > > 
> > > IIUC, v8String uses a cache to make the conversion as much as fast, while
> > > v8NormalString simply calls v8 APIs.
> > > 
> > > I'd recommend to rename v8String and v8NormalString to represent what they
> are
> > > and what they are not.  Without reading the implementation, I cannot tell
> > which
> > > I should use.
> > > 
> > > Function header comments are welcome, too.
> > 
> > Good point!
> > 
> > BTW, do we really need v8NormalString? I understand that v8NormalString and
> > v8String are different, but I'm not sure if the difference matters for
> > performance. If not, we can just replace v8NormalString with v8String.
> 
> Added a comment on v8NormalString(). I think we should have v8NormalString()
> because it doesn't require String. Many call sites just passing C++'s string
> literals, which means that we create a temporary String instance unnecessary.
> 
> As for naming, I'm going to rename v8String -> v8SharedString. This will touch
> many files, so I'll do that in a follow-up CL.

How about replacing v8NormalString with v8AtomicString?

Conceptually we should have four types of strings:

(a) A V8 string created from blink::String with a cache.
(b) A V8 string created from blink::String without a cache.
(c) A V8 string created from char* with a cache.
(d) A V8 string created from char* without a cache.

Currently (a) is v8String, (b) is not implemented, (c) is v8AtomicString, and
(d) is v8NormalString.

I understand these are different conceptually, but if the performance doesn't
matter, I might want to remove (b) and (d) for simplicity. At least, it looks
inconsistent that we have (a), (c) and (d) but don't have (b).

[bashi, 2015-03-24 01:07:39]

On 2015/03/24 00:51:17, haraken wrote:
> On 2015/03/23 23:42:31, bashi1 wrote:
> > On 2015/03/23 08:55:14, haraken wrote:
> > > On 2015/03/23 08:52:51, Yuki wrote:
> > > > It seems a little confusing that we have following three functions.
> > > > - v8String()
> > > > - v8NormalString()   // NEW
> > > > - v8AtomicString()
> > > > 
> > > > What v8AtomicString is is crystal clear, however, the difference between
> > > > v8String and v8NormalString is not that clear.  v8String is NOT
normal???
> > > > 
> > > > IIUC, v8String uses a cache to make the conversion as much as fast,
while
> > > > v8NormalString simply calls v8 APIs.
> > > > 
> > > > I'd recommend to rename v8String and v8NormalString to represent what
they
> > are
> > > > and what they are not.  Without reading the implementation, I cannot
tell
> > > which
> > > > I should use.
> > > > 
> > > > Function header comments are welcome, too.
> > > 
> > > Good point!
> > > 
> > > BTW, do we really need v8NormalString? I understand that v8NormalString
and
> > > v8String are different, but I'm not sure if the difference matters for
> > > performance. If not, we can just replace v8NormalString with v8String.
> > 
> > Added a comment on v8NormalString(). I think we should have v8NormalString()
> > because it doesn't require String. Many call sites just passing C++'s string
> > literals, which means that we create a temporary String instance
unnecessary.
> > 
> > As for naming, I'm going to rename v8String -> v8SharedString. This will
touch
> > many files, so I'll do that in a follow-up CL.
> 
> How about replacing v8NormalString with v8AtomicString?
> 
> Conceptually we should have four types of strings:
> 
> (a) A V8 string created from blink::String with a cache.
> (b) A V8 string created from blink::String without a cache.
> (c) A V8 string created from char* with a cache.
> (d) A V8 string created from char* without a cache.
> 
> Currently (a) is v8String, (b) is not implemented, (c) is v8AtomicString, and
> (d) is v8NormalString.
> 
> I understand these are different conceptually, but if the performance doesn't
> matter, I might want to remove (b) and (d) for simplicity. At least, it looks
> inconsistent that we have (a), (c) and (d) but don't have (b).

We currently have (a) and (d), which is already inconsistent :)
Anyway, in my understanding, the criteria of having functions in V8Binding.h is
that "add it when it is needed". And I thought we need v8NormalString() to make
sure no semantic change and to avoid unnecessary object creation. If we can use
v8AtomicString(), which sightly changes the semantics of the code, we don't need
v8NormalString(). Let's try it.

[haraken, 2015-03-24 01:25:52]

On 2015/03/24 01:07:39, bashi1 wrote:
> On 2015/03/24 00:51:17, haraken wrote:
> > On 2015/03/23 23:42:31, bashi1 wrote:
> > > On 2015/03/23 08:55:14, haraken wrote:
> > > > On 2015/03/23 08:52:51, Yuki wrote:
> > > > > It seems a little confusing that we have following three functions.
> > > > > - v8String()
> > > > > - v8NormalString()   // NEW
> > > > > - v8AtomicString()
> > > > > 
> > > > > What v8AtomicString is is crystal clear, however, the difference
between
> > > > > v8String and v8NormalString is not that clear.  v8String is NOT
> normal???
> > > > > 
> > > > > IIUC, v8String uses a cache to make the conversion as much as fast,
> while
> > > > > v8NormalString simply calls v8 APIs.
> > > > > 
> > > > > I'd recommend to rename v8String and v8NormalString to represent what
> they
> > > are
> > > > > and what they are not.  Without reading the implementation, I cannot
> tell
> > > > which
> > > > > I should use.
> > > > > 
> > > > > Function header comments are welcome, too.
> > > > 
> > > > Good point!
> > > > 
> > > > BTW, do we really need v8NormalString? I understand that v8NormalString
> and
> > > > v8String are different, but I'm not sure if the difference matters for
> > > > performance. If not, we can just replace v8NormalString with v8String.
> > > 
> > > Added a comment on v8NormalString(). I think we should have
v8NormalString()
> > > because it doesn't require String. Many call sites just passing C++'s
string
> > > literals, which means that we create a temporary String instance
> unnecessary.
> > > 
> > > As for naming, I'm going to rename v8String -> v8SharedString. This will
> touch
> > > many files, so I'll do that in a follow-up CL.
> > 
> > How about replacing v8NormalString with v8AtomicString?
> > 
> > Conceptually we should have four types of strings:
> > 
> > (a) A V8 string created from blink::String with a cache.
> > (b) A V8 string created from blink::String without a cache.
> > (c) A V8 string created from char* with a cache.
> > (d) A V8 string created from char* without a cache.
> > 
> > Currently (a) is v8String, (b) is not implemented, (c) is v8AtomicString,
and
> > (d) is v8NormalString.
> > 
> > I understand these are different conceptually, but if the performance
doesn't
> > matter, I might want to remove (b) and (d) for simplicity. At least, it
looks
> > inconsistent that we have (a), (c) and (d) but don't have (b).
> 
> We currently have (a) and (d), which is already inconsistent :)
> Anyway, in my understanding, the criteria of having functions in V8Binding.h
is
> that "add it when it is needed". And I thought we need v8NormalString() to
make
> sure no semantic change and to avoid unnecessary object creation. If we can
use
> v8AtomicString(), which sightly changes the semantics of the code, we don't
need
> v8NormalString(). Let's try it.

LGTM to land this change, but would be worth trying to cleaning up the String
related methods (My 2 cents are on replacing v8NormalString with
v8AtomicString).

[bashi, 2015-03-24 01:28:49]

On 2015/03/24 01:25:52, haraken wrote:
> On 2015/03/24 01:07:39, bashi1 wrote:
> > On 2015/03/24 00:51:17, haraken wrote:
> > > On 2015/03/23 23:42:31, bashi1 wrote:
> > > > On 2015/03/23 08:55:14, haraken wrote:
> > > > > On 2015/03/23 08:52:51, Yuki wrote:
> > > > > > It seems a little confusing that we have following three functions.
> > > > > > - v8String()
> > > > > > - v8NormalString()   // NEW
> > > > > > - v8AtomicString()
> > > > > > 
> > > > > > What v8AtomicString is is crystal clear, however, the difference
> between
> > > > > > v8String and v8NormalString is not that clear.  v8String is NOT
> > normal???
> > > > > > 
> > > > > > IIUC, v8String uses a cache to make the conversion as much as fast,
> > while
> > > > > > v8NormalString simply calls v8 APIs.
> > > > > > 
> > > > > > I'd recommend to rename v8String and v8NormalString to represent
what
> > they
> > > > are
> > > > > > and what they are not.  Without reading the implementation, I cannot
> > tell
> > > > > which
> > > > > > I should use.
> > > > > > 
> > > > > > Function header comments are welcome, too.
> > > > > 
> > > > > Good point!
> > > > > 
> > > > > BTW, do we really need v8NormalString? I understand that
v8NormalString
> > and
> > > > > v8String are different, but I'm not sure if the difference matters for
> > > > > performance. If not, we can just replace v8NormalString with v8String.
> > > > 
> > > > Added a comment on v8NormalString(). I think we should have
> v8NormalString()
> > > > because it doesn't require String. Many call sites just passing C++'s
> string
> > > > literals, which means that we create a temporary String instance
> > unnecessary.
> > > > 
> > > > As for naming, I'm going to rename v8String -> v8SharedString. This will
> > touch
> > > > many files, so I'll do that in a follow-up CL.
> > > 
> > > How about replacing v8NormalString with v8AtomicString?
> > > 
> > > Conceptually we should have four types of strings:
> > > 
> > > (a) A V8 string created from blink::String with a cache.
> > > (b) A V8 string created from blink::String without a cache.
> > > (c) A V8 string created from char* with a cache.
> > > (d) A V8 string created from char* without a cache.
> > > 
> > > Currently (a) is v8String, (b) is not implemented, (c) is v8AtomicString,
> and
> > > (d) is v8NormalString.
> > > 
> > > I understand these are different conceptually, but if the performance
> doesn't
> > > matter, I might want to remove (b) and (d) for simplicity. At least, it
> looks
> > > inconsistent that we have (a), (c) and (d) but don't have (b).
> > 
> > We currently have (a) and (d), which is already inconsistent :)
> > Anyway, in my understanding, the criteria of having functions in V8Binding.h
> is
> > that "add it when it is needed". And I thought we need v8NormalString() to
> make
> > sure no semantic change and to avoid unnecessary object creation. If we can
> use
> > v8AtomicString(), which sightly changes the semantics of the code, we don't
> need
> > v8NormalString(). Let's try it.
> 
> LGTM to land this change, but would be worth trying to cleaning up the String
> related methods (My 2 cents are on replacing v8NormalString with
> v8AtomicString).

PS#3 removed v8NormalString and replaced NewFromUtf8() with v8AtomicString().
Sorry for confusion. I'll land PS#3 if trybots succeed.

[haraken, 2015-03-24 01:39:42]

On 2015/03/24 01:28:49, bashi1 wrote:
> On 2015/03/24 01:25:52, haraken wrote:
> > On 2015/03/24 01:07:39, bashi1 wrote:
> > > On 2015/03/24 00:51:17, haraken wrote:
> > > > On 2015/03/23 23:42:31, bashi1 wrote:
> > > > > On 2015/03/23 08:55:14, haraken wrote:
> > > > > > On 2015/03/23 08:52:51, Yuki wrote:
> > > > > > > It seems a little confusing that we have following three
functions.
> > > > > > > - v8String()
> > > > > > > - v8NormalString()   // NEW
> > > > > > > - v8AtomicString()
> > > > > > > 
> > > > > > > What v8AtomicString is is crystal clear, however, the difference
> > between
> > > > > > > v8String and v8NormalString is not that clear.  v8String is NOT
> > > normal???
> > > > > > > 
> > > > > > > IIUC, v8String uses a cache to make the conversion as much as
fast,
> > > while
> > > > > > > v8NormalString simply calls v8 APIs.
> > > > > > > 
> > > > > > > I'd recommend to rename v8String and v8NormalString to represent
> what
> > > they
> > > > > are
> > > > > > > and what they are not.  Without reading the implementation, I
cannot
> > > tell
> > > > > > which
> > > > > > > I should use.
> > > > > > > 
> > > > > > > Function header comments are welcome, too.
> > > > > > 
> > > > > > Good point!
> > > > > > 
> > > > > > BTW, do we really need v8NormalString? I understand that
> v8NormalString
> > > and
> > > > > > v8String are different, but I'm not sure if the difference matters
for
> > > > > > performance. If not, we can just replace v8NormalString with
v8String.
> > > > > 
> > > > > Added a comment on v8NormalString(). I think we should have
> > v8NormalString()
> > > > > because it doesn't require String. Many call sites just passing C++'s
> > string
> > > > > literals, which means that we create a temporary String instance
> > > unnecessary.
> > > > > 
> > > > > As for naming, I'm going to rename v8String -> v8SharedString. This
will
> > > touch
> > > > > many files, so I'll do that in a follow-up CL.
> > > > 
> > > > How about replacing v8NormalString with v8AtomicString?
> > > > 
> > > > Conceptually we should have four types of strings:
> > > > 
> > > > (a) A V8 string created from blink::String with a cache.
> > > > (b) A V8 string created from blink::String without a cache.
> > > > (c) A V8 string created from char* with a cache.
> > > > (d) A V8 string created from char* without a cache.
> > > > 
> > > > Currently (a) is v8String, (b) is not implemented, (c) is
v8AtomicString,
> > and
> > > > (d) is v8NormalString.
> > > > 
> > > > I understand these are different conceptually, but if the performance
> > doesn't
> > > > matter, I might want to remove (b) and (d) for simplicity. At least, it
> > looks
> > > > inconsistent that we have (a), (c) and (d) but don't have (b).
> > > 
> > > We currently have (a) and (d), which is already inconsistent :)
> > > Anyway, in my understanding, the criteria of having functions in
V8Binding.h
> > is
> > > that "add it when it is needed". And I thought we need v8NormalString() to
> > make
> > > sure no semantic change and to avoid unnecessary object creation. If we
can
> > use
> > > v8AtomicString(), which sightly changes the semantics of the code, we
don't
> > need
> > > v8NormalString(). Let's try it.
> > 
> > LGTM to land this change, but would be worth trying to cleaning up the
String
> > related methods (My 2 cents are on replacing v8NormalString with
> > v8AtomicString).
> 
> PS#3 removed v8NormalString and replaced NewFromUtf8() with v8AtomicString().
> Sorry for confusion. I'll land PS#3 if trybots succeed.

Sure!

[bashi, 2015-03-24 02:48:05]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2015-03-24 02:48:18]

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

[commit-bot: I haz the power, 2015-03-24 02:51:40]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192406

[yurys, 2015-03-24 13:31:09]

On 2015/03/23 23:54:01, bashi1 wrote:
> On 2015/03/23 07:30:26, yurys wrote:
> > On 2015/03/23 07:29:06, yurys wrote:
> > > On 2015/03/23 01:26:26, bashi1 wrote:
> > > > On 2015/03/20 11:00:47, yurys wrote:
> > > > > Can you please revert changes in the following files in favor of using
> > v8.h
> > > > APIs
> > > > > directly?
> > > > > ScriptDebugServer.cpp
> > > > > V8InjectedScriptHostCustom.cpp
> > > > > JavaScriptCallFrame.cpp
> > > > > 
> > > > > The rationale for this is that these files are going to be moved out
of
> > > Blink
> > > > > closer to v8, hence these files shouldn't depend on bindings code. See
> [1]
> > > for
> > > > > more details.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://docs.google.com/document/d/1fEkZFMH_U5DhIYM95Mhp5ovtBUeHaToSvgQapDzgN...
> > > > 
> > > > Sure. Will do tomorrow (I'm ooo today). I think it would be better to
use
> > new
> > > > APIs rather than just reverting them. WDYT?
> > 
> > If the old ones are going to be removed soon, then sure, we can switch this
> code
> > to Maybe* versions.
> 
> Just reverted because new APIs returns MaybeLocal, and without our macros,
code
> will mess up.
> 
> BTW, this kind of special treatment makes me very frustrated. I change
> V8_DEPRECATE_SOON -> V8_DEPRECATED to verify we don't use old APIs, but now I
> can't use this method. Also, I can't semi-automatic grep-and-replace because
> some files are placed outside of core/inspector (e.g. ScriptDebugServer).
> 
> yurys@, do you have any ideas how to deal with these problem?

I'm not sure what the advantage of MaybeLocal version over Local are. Can you
educate me on this?

Before this change if we had failed to create v8::String for "DevToolsHost" the
program would have crashed in WebDevToolsFrontendImpl.cpp on attempt to use
empty handle in the following line:
global->Set(v8::String::NewFromUtf8(isolate, "DevToolsHost"), devtoolsHostObj);
with the new implementation if the conversion fails we will set devtoolsHostObj
into a property with name "" which doesn't make sense to me. It will result in a
failure in some random place in the code far from the place where the actual
problem occurred. The only valid reason I could think of why this allocation
could fail is OOM. In case of OOM we should crash instead of swallowing the
error and returning empty string instead of expected one. Can we revert this
please?

All the reasoning above applies to the change in InspectorOverlayImpl.cpp. I
don't think we should go this way and silently swallow exceptions (I assume that
empty handle means that there was an exception). Returning handle to empty
string instead of actual one just hides the problem and will make things very
hard to debug, whenever possible the error should be reported to the caller and
handled properly otherwise we should probably crash.

[bashi, 2015-03-24 23:09:39]

On 2015/03/24 13:31:09, yurys wrote:
> On 2015/03/23 23:54:01, bashi1 wrote:
> > On 2015/03/23 07:30:26, yurys wrote:
> > > On 2015/03/23 07:29:06, yurys wrote:
> > > > On 2015/03/23 01:26:26, bashi1 wrote:
> > > > > On 2015/03/20 11:00:47, yurys wrote:
> > > > > > Can you please revert changes in the following files in favor of
using
> > > v8.h
> > > > > APIs
> > > > > > directly?
> > > > > > ScriptDebugServer.cpp
> > > > > > V8InjectedScriptHostCustom.cpp
> > > > > > JavaScriptCallFrame.cpp
> > > > > > 
> > > > > > The rationale for this is that these files are going to be moved out
> of
> > > > Blink
> > > > > > closer to v8, hence these files shouldn't depend on bindings code.
See
> > [1]
> > > > for
> > > > > > more details.
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://docs.google.com/document/d/1fEkZFMH_U5DhIYM95Mhp5ovtBUeHaToSvgQapDzgN...
> > > > > 
> > > > > Sure. Will do tomorrow (I'm ooo today). I think it would be better to
> use
> > > new
> > > > > APIs rather than just reverting them. WDYT?
> > > 
> > > If the old ones are going to be removed soon, then sure, we can switch
this
> > code
> > > to Maybe* versions.
> > 
> > Just reverted because new APIs returns MaybeLocal, and without our macros,
> code
> > will mess up.
> > 
> > BTW, this kind of special treatment makes me very frustrated. I change
> > V8_DEPRECATE_SOON -> V8_DEPRECATED to verify we don't use old APIs, but now
I
> > can't use this method. Also, I can't semi-automatic grep-and-replace because
> > some files are placed outside of core/inspector (e.g. ScriptDebugServer).
> > 
> > yurys@, do you have any ideas how to deal with these problem?
> 
> I'm not sure what the advantage of MaybeLocal version over Local are. Can you
> educate me on this?

Please refer the motivation [1] and the V8 team's solution [2].

[1]
https://docs.google.com/a/chromium.org/document/d/1komNUseODFLptg4l45zerYRpWB...
[2]
https://groups.google.com/forum/#!searchin/v8-users/maybe/v8-users/gQVpp1Hmbq...

> 
> Before this change if we had failed to create v8::String for "DevToolsHost"
the
> program would have crashed in WebDevToolsFrontendImpl.cpp on attempt to use
> empty handle in the following line:
> global->Set(v8::String::NewFromUtf8(isolate, "DevToolsHost"),
devtoolsHostObj);
> with the new implementation if the conversion fails we will set
devtoolsHostObj
> into a property with name "" which doesn't make sense to me. It will result in
a

I don't think it's a good idea that depending on "touching an empty handle cause
crash". This is the cause of many bugs described jochen's document. You should
always check an empty handle.

> failure in some random place in the code far from the place where the actual
> problem occurred. The only valid reason I could think of why this allocation
> could fail is OOM. In case of OOM we should crash instead of swallowing the
> error and returning empty string instead of expected one. Can we revert this
> please?

So, I don't want to revert this change.

> 
> All the reasoning above applies to the change in InspectorOverlayImpl.cpp. I
> don't think we should go this way and silently swallow exceptions (I assume
that
> empty handle means that there was an exception). Returning handle to empty
> string instead of actual one just hides the problem and will make things very
> hard to debug, whenever possible the error should be reported to the caller
and
> handled properly otherwise we should probably crash.

Could you please read the description in v8.h . It explicitly says "Only returns
an empty value when length > kMaxLength."

Devtools may not be related with workers, but it would be confusing to have two
policy (devtool related code and other blink code) for v8 API usage.

[pfeldman, 2015-03-25 05:26:50]

pfeldman@chromium.org changed reviewers:
+ jochen@chromium.org

[pfeldman, 2015-03-25 05:33:24]

I share Yury's sentiment and think this should be reverted. As I read it, it is
a silent catch on the problems with strings. Instead of masking the problem and
letting it slip into the unknown, we should deal with it locally.

Jochen, wdyt?

[bashi, 2015-03-25 05:43:35]

On 2015/03/25 05:33:24, pfeldman wrote:
> I share Yury's sentiment and think this should be reverted. As I read it, it
is
> a silent catch on the problems with strings. Instead of masking the problem
and
> letting it slip into the unknown, we should deal with it locally.
> 
> Jochen, wdyt?

Note that v8AtomicString() doesn't swallow an exception. You can use TryCatch to
catch the exception. What I wanted to do is that to make sure v8AtomicString()
won't return an empty value. Without this guarantee, all call sites must be
careful whether an empty value is fine or not, which doesn't sound a good idea
to me.

[pfeldman, 2015-03-25 05:43:37]

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

[pfeldman, 2015-03-25 05:43:37]

https://codereview.chromium.org/1027593003/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1027593003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:356: return v8::String::Empty(isolate);
At this point we know that there is a bug in the code. The problem has already
occurred, we know that the subsequent execution is going to be erroneous. For
example string created from the embedder notification did not make its way into
v8. Since we don't face any security risks such as UAF, we should definitely
crash the renderer on the call site and report it. It is our only option to fix
this bug, what you are doing is masking it.

[pfeldman, 2015-03-25 05:48:55]

> Without this guarantee, all call sites must be
> careful whether an empty value is fine or not, which doesn't sound a good idea
> to me.

I was using 'silent catch' metaphorically - it is when instead of dealing with
the problem you hide it. So I still read it as a "silent catch" pattern. If we
ever want to find the call sites that are feeding v8 with unbounded strings, we
need to deal with it.

[bashi, 2015-03-25 05:50:10]

https://codereview.chromium.org/1027593003/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1027593003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:356: return v8::String::Empty(isolate);
On 2015/03/25 05:43:37, pfeldman wrote:
> At this point we know that there is a bug in the code. The problem has already
> occurred, we know that the subsequent execution is going to be erroneous. For
> example string created from the embedder notification did not make its way
into
> v8. Since we don't face any security risks such as UAF, we should definitely
> crash the renderer on the call site and report it. It is our only option to
fix
> this bug, what you are doing is masking it.

OK, How about having RELEASE_ASSERT() here then?

[pfeldman, 2015-03-25 05:51:36]

> OK, How about having RELEASE_ASSERT() here then?

That'd be a great solution.

[pfeldman, 2015-03-25 05:53:52]

(we need to make sure there are no call sites that intentionally feed v8 with
large strings and check the returning handle, but i doubt they exist)

[bashi, 2015-03-25 06:01:33]

On 2015/03/25 05:51:36, pfeldman_ooo_mar_31 wrote:
> > OK, How about having RELEASE_ASSERT() here then?
> 
> That'd be a great solution.

Thanks, will try to do it.

[yurys, 2015-03-25 07:07:22]

https://codereview.chromium.org/1027593003/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1027593003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:356: return v8::String::Empty(isolate);
On 2015/03/25 05:50:09, bashi1 wrote:
> On 2015/03/25 05:43:37, pfeldman wrote:
> > At this point we know that there is a bug in the code. The problem has
already
> > occurred, we know that the subsequent execution is going to be erroneous.
For
> > example string created from the embedder notification did not make its way
> into
> > v8. Since we don't face any security risks such as UAF, we should definitely
> > crash the renderer on the call site and report it. It is our only option to
> fix
> > this bug, what you are doing is masking it.
> 
> OK, How about having RELEASE_ASSERT() here then?

Having RELEASE_ASSERT_NOT_REACHED() here would be great. You just need to make
sure that there is no code that checks for empty handle returned from
v8AtomicString and recovers from the problem. Adding RELEASE_ASSERT here would
regress such code.

[bashi, 2015-03-25 07:16:39]

On 2015/03/25 07:07:22, yurys wrote:
>
https://codereview.chromium.org/1027593003/diff/40001/Source/bindings/core/v8...
> File Source/bindings/core/v8/V8Binding.h (right):
> 
>
https://codereview.chromium.org/1027593003/diff/40001/Source/bindings/core/v8...
> Source/bindings/core/v8/V8Binding.h:356: return v8::String::Empty(isolate);
> On 2015/03/25 05:50:09, bashi1 wrote:
> > On 2015/03/25 05:43:37, pfeldman wrote:
> > > At this point we know that there is a bug in the code. The problem has
> already
> > > occurred, we know that the subsequent execution is going to be erroneous.
> For
> > > example string created from the embedder notification did not make its way
> > into
> > > v8. Since we don't face any security risks such as UAF, we should
definitely
> > > crash the renderer on the call site and report it. It is our only option
to
> > fix
> > > this bug, what you are doing is masking it.
> > 
> > OK, How about having RELEASE_ASSERT() here then?
> 
> Having RELEASE_ASSERT_NOT_REACHED() here would be great. You just need to make
> sure that there is no code that checks for empty handle returned from
> v8AtomicString and recovers from the problem. Adding RELEASE_ASSERT here would
> regress such code.

Yeah, RELEASE_ASSERT_NOT_REACHED() is much better.

[jochen (gone - plz use gerrit), 2015-03-25 07:19:46]

Methods that invoke V8 APIs that can throw should either

- return a Maybe value so ask call sites are forced to deal with exceptions
- use ToLocalChecked so we crash if an exception was thrown
- or deal with the exception

Manually converting a Maybe value to an empty handle defeats the purpose of this
api change

[dcarney, 2015-03-25 07:56:25]

for the string::New* functions, no exception will be thrown because I haven't
yet decided how to throw without contexts, so an empty handle will be returned
if and only if the v8 string will be > String::kMaxLength, otherwise it will
succeed

you can optimize for this case as in most cases the incoming string will be less
than 256MB and either way you need exception handling

[yurys, 2015-03-25 08:02:01]

On 2015/03/25 07:56:25, dcarney wrote:
> for the string::New* functions, no exception will be thrown because I haven't
> yet decided how to throw without contexts, so an empty handle will be returned
> if and only if the v8 string will be > String::kMaxLength, otherwise it will
> succeed
> 
If the string is < String::kMaxLength but v8 fails to allocate some internal
structures this will crash in OOM handler before returning from string::New* ,
right?

[haraken, 2015-03-25 08:43:07]

On 2015/03/25 07:56:25, dcarney wrote:
> for the string::New* functions, no exception will be thrown because I haven't
> yet decided how to throw without contexts, so an empty handle will be returned
> if and only if the v8 string will be > String::kMaxLength, otherwise it will
> succeed
> 
> you can optimize for this case as in most cases the incoming string will be
less
> than 256MB and either way you need exception handling

I'm a bit confused but aren't we planning to guarantee that Maybe is an empty
handle if and only if a V8 exception is thrown?

[bashi, 2015-03-25 10:55:59]

For the record, added RELEASE_ASSERT_NOT_REACHED().
https://codereview.chromium.org/1032053003/