Use V8TRYCATCH_FOR_V8STRINGRESOURCE() macro instead of toCoreStringWithUndefinedOrNullCheck()

Use V8TRYCATCH_FOR_V8STRINGRESOURCE() macro instead of toCoreStringWithUndefinedOrNullCheck()

Use V8TRYCATCH_FOR_V8STRINGRESOURCE() macro instead of toCoreStringWithUndefinedOrNullCheck()
when necessary. Namely, when an exception may be thrown, we should use the macro instead to
return early when the call to ToString() throws.

This patch also updates toCoreStringWithUndefinedOrNullCheck(v8::Handle<v8::Value>()) to
stop calling ToString(). This method can never throw and is thus now safe to use if the
caller wants to convert to WTF String only if the v8::Value contains a v8::String. This
utility method is used in the inspector code.

R=haraken, pfeldman
BUG=327338
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163873

[haraken, 2013-12-12 15:41:42]

LGTM but pfeldman might want to take another look at the inspector code.

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

https://codereview.chromium.org/100963003/diff/1/Source/bindings/v8/V8Binding...
Source/bindings/v8/V8Binding.h:192: // please use the
V8TRYCATCH_FOR_V8STRINGRESOURCE_*() macros instead.

You want to add this comment to toCoreString(v8::Handle<v8::Value> value) as
well. Probably we want to put these two methods next to each other.

[Inactive, 2013-12-12 15:44:22]

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

https://codereview.chromium.org/100963003/diff/1/Source/bindings/v8/V8Binding...
Source/bindings/v8/V8Binding.h:192: // please use the
V8TRYCATCH_FOR_V8STRINGRESOURCE_*() macros instead.
On 2013/12/12 15:41:43, haraken wrote:
> 
> You want to add this comment to toCoreString(v8::Handle<v8::Value> value) as
> well. Probably we want to put these two methods next to each other.

Oh my bad, I did not intend to include toCoreString(v8::Handle<v8::Value> value)
in this patch. It is not currently used. I forgot to remove it before uploading.

[pfeldman, 2013-12-12 15:47:07]

This looks exactly like the one I rolled out. Please don't change this code due
to reasoning explained in the bug.

[Inactive, 2013-12-12 15:51:53]

On 2013/12/12 15:47:07, pfeldman wrote:
> This looks exactly like the one I rolled out. Please don't change this code
due
> to reasoning explained in the bug.

The difference is that I kept using
toCoreStringWithUndefinedOrNullCheck(v8::Handle<v8::Value>) in the inspector/
folder. However, this method no longer throws as it no longer calls ToString().
Can you please clarify which code you'd like to keep using
toCoreStringWithUndefinedOrNullCheck()?

Based on your comment I am assuming you are referring to inspector code in the
bindings/ folder? It is not obvious to me which files though since they don't
mention inspector in the name or path (and I am not familiar with inspector).

[Inactive, 2013-12-12 15:59:25]

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8JavaScriptCallFrameCustom.cpp (right):

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8JavaScriptCallFrameCustom.cpp:40:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithUndefinedOrNullCheck>,
expression, info[0]);
For e.g. I understand that this is related to the inspector. However, these are
bindings and this macro/behavior is what would be generated by our bindings
generator script if this wasn't custom code.

[pfeldman, 2013-12-12 16:11:23]

Sure, sorry for not being precise.

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/PageS...
File Source/bindings/v8/PageScriptDebugServer.cpp (right):

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/PageS...
Source/bindings/v8/PageScriptDebugServer.cpp:228:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithUndefinedOrNullCheck>,
scriptName, callDebuggerMethod("getScriptName", WTF_ARRAY_LENGTH(argvEventData),
argvEventData));
Inspector code often keeps things under manual control. Almost never, you want
to return from the middle of the beforecompile callback. For example,
callDebuggerMethod never throws. So this code should stay as is.

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/PageS...
Source/bindings/v8/PageScriptDebugServer.cpp:229:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithUndefinedOrNullCheck>,
script, callDebuggerMethod("getScriptSource", WTF_ARRAY_LENGTH(argvEventData),
argvEventData));
ditto

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:308: .setMessage(message)
Same here, we know it is a string. See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:312:
V8StringResource<WithUndefinedOrNullCheck> errorResource(resultTuple->Get(1));
ditto

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:503:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithUndefinedOrNullCheck>,
sourceID, object->Get(v8AtomicString(m_isolate, "id")));
These are of known types:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:511: script.sourceMappingURL =
sourceMappingURL;
Hm. This seems NULL. Adding vsevik for this.

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:368:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithUndefinedOrNullCheck>,
variableName, info[2]);
Same here, it is a string, see the call site.

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:428: if
(!name->IsString() || !v8::Handle<v8::String>::Cast(name)->Length())
Ditto - see this check.

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8JavaScriptCallFrameCustom.cpp (right):

https://codereview.chromium.org/100963003/diff/20001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8JavaScriptCallFrameCustom.cpp:40:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithUndefinedOrNullCheck>,
expression, info[0]);
This is only called from within InjectedScriptSource, should be always string as
well.

[Inactive, 2013-12-12 16:31:58]

Thanks for looking into this in details pfedlman@! I updated the CL accordingly.

[pfeldman, 2013-12-12 16:41:41]

inspector lgtm

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

https://codereview.chromium.org/100963003/diff/40001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:387:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithUndefinedOrNullCheck>,
hitBreakpointNumber, hitBreakpointNumbers->Get(i));
We need to make sure this comes in as string. vsevik@, yurys@, could you follow
up?

[yurys, 2013-12-12 20:04:43]

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

https://codereview.chromium.org/100963003/diff/40001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:387:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithUndefinedOrNullCheck>,
hitBreakpointNumber, hitBreakpointNumbers->Get(i));
On 2013/12/12 16:41:41, pfeldman_ooo wrote:
> We need to make sure this comes in as string. vsevik@, yurys@, could you
follow
> up?

The array should actually contain numbers but we need them to be converted to
Stings to be later passed over the protocol. The conversion is done in
JavaScript at the moment which we can change and ASSERT that
hitBreakpointNumbers contains only numbers. These numbers can be converted to
Strings here using String::number(). This would make the code a way more clear
and let us avoid this scary mix of macros and templates.

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

https://codereview.chromium.org/100963003/diff/40001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.h:188: if (value.IsEmpty() || value->IsNull() ||
value->IsUndefined() || !value->IsString())
Isn't this equal to 
if (value.IsEmpty() || !value->IsString()) ?

[Inactive, 2013-12-12 22:43:38]

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

https://codereview.chromium.org/100963003/diff/40001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:387:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<WithUndefinedOrNullCheck>,
hitBreakpointNumber, hitBreakpointNumbers->Get(i));
On 2013/12/12 20:04:43, yurys wrote:
> On 2013/12/12 16:41:41, pfeldman_ooo wrote:
> > We need to make sure this comes in as string. vsevik@, yurys@, could you
> follow
> > up?
> 
> The array should actually contain numbers but we need them to be converted to
> Stings to be later passed over the protocol. The conversion is done in
> JavaScript at the moment which we can change and ASSERT that
> hitBreakpointNumbers contains only numbers. These numbers can be converted to
> Strings here using String::number(). This would make the code a way more clear
> and let us avoid this scary mix of macros and templates. 

Done.

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

https://codereview.chromium.org/100963003/diff/40001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.h:188: if (value.IsEmpty() || value->IsNull() ||
value->IsUndefined() || !value->IsString())
On 2013/12/12 20:04:43, yurys wrote:
> Isn't this equal to 
> if (value.IsEmpty() || !value->IsString()) ?

Done.

[yurys, 2013-12-13 07:18:30]

DevTools changes lgtm!

[commit-bot: I haz the power, 2013-12-13 14:11:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/100963003/70001

[commit-bot: I haz the power, 2013-12-13 15:13:47]

Change committed as 163873