DevTools: added injectedScript.evaluateWithDetails, that return exception details if it occured

Added V8InjectedScriptHost::evaluateWithDetails. This method returns {result: evaluation result, exceptionDetails: information about the exception, if it occurred}.
InjectedScriptSource.evaluate and InjectedScriptSource.callFunctionOn uses it. There functions returns additional field with exceptionDetails, if exception occurred.

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

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

[vsevik, 2014-05-20 08:23:01]

We should return the resulting exception through protocol, not through general
Devtools exception handler.

[vsevik, 2014-05-20 08:23:09]

https://chromiumcodereview.appspot.com/289423002/diff/1/Source/bindings/v8/cu...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (left):

https://chromiumcodereview.appspot.com/289423002/diff/1/Source/bindings/v8/cu...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:339:
v8SetReturnValue(info, tryCatch.ReThrow());
Let's move on with SetVerbose instead.

[kozyatinskiy1, 2014-06-11 16:34:41]

On 2014/05/26 12:13:35, kozyatinskiy wrote:

All dependency have already committed. Please, review.

[vsevik, 2014-06-27 12:23:57]

Let's split this into 2 patches:
1) Moving try/catch blocks around
2) Plumbing exceptionDetails

[kozyatinskiy1, 2014-07-03 16:00:59]

On 2014/06/27 12:23:57, vsevik wrote:
> 1) Moving try/catch blocks around
Done. Moved completely.

[vsevik, 2014-07-04 06:54:59]

https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
File Source/core/inspector/InjectedScriptSource.js (right):

https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
Source/core/inspector/InjectedScriptSource.js:613: if (func.exceptionDetails)
Everything except func.apply could me moved out of the try/catch block now.

https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
Source/core/inspector/InjectedScriptSource.js:668: _evaluateAndWrap:
function(evalFunction, object, expression, objectGroup, isEvalOnCallFrame,
injectCommandLineAPI, returnByValue, generatePreview, scopeChain)
Now that this one is only called for evaluation on callframe it makes sense to
name it evaluateOnCallFrameAndWrap and make callframe its first argument.

https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
Source/core/inspector/InjectedScriptSource.js:691: _evaluateAndWrapWithDetails:
function(evalFunction, object, expression, objectGroup, isEvalOnCallFrame,
injectCommandLineAPI, returnByValue, generatePreview, scopeChain)
This one could be changed accordingly, making it _evaluateAndWrap that doesn't
accept evalFunction, but takes it directly from InjectedScriptHost instead.

https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
Source/core/inspector/InjectedScriptSource.js:808: return
this._evaluateAndWrap(callFrame.evaluate, callFrame, expression, objectGroup,
true, injectCommandLineAPI, returnByValue, generatePreview);
So we are still missing exceptionDetails in case of evaluation on call frame.
This is something worth investigation, but might probably require complicated v8
changes. We could do this separately though.

https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
Source/core/inspector/InjectedScriptSource.js:847:
_injectedScriptHostEvaluateThrowable: function(expression) {
We could probably name this one _evaluateOrThrow

https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
Source/core/inspector/InjectedScriptSource.js:849: return wrappedResult.result;
Return early? :)

[kozyatinskiy1, 2014-07-04 14:10:26]

On 2014/07/04 06:54:59, vsevik wrote:
>
https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
> File Source/core/inspector/InjectedScriptSource.js (right):
> 
>
https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
> Source/core/inspector/InjectedScriptSource.js:613: if (func.exceptionDetails)
> Everything except func.apply could me moved out of the try/catch block now.
Restored wrapper, that emulates old behavior. We can't wrap func.apply, and I
suppose, that it is not good - keep only one case of the two.

> 
>
https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
> Source/core/inspector/InjectedScriptSource.js:668: _evaluateAndWrap:
> function(evalFunction, object, expression, objectGroup, isEvalOnCallFrame,
> injectCommandLineAPI, returnByValue, generatePreview, scopeChain)
> Now that this one is only called for evaluation on callframe it makes sense to
> name it evaluateOnCallFrameAndWrap and make callframe its first argument.
This method removed.

> 
>
https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
> Source/core/inspector/InjectedScriptSource.js:691:
_evaluateAndWrapWithDetails:
> function(evalFunction, object, expression, objectGroup, isEvalOnCallFrame,
> injectCommandLineAPI, returnByValue, generatePreview, scopeChain)
> This one could be changed accordingly, making it _evaluateAndWrap that doesn't
> accept evalFunction, but takes it directly from InjectedScriptHost instead.
Done.

> 
>
https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
> Source/core/inspector/InjectedScriptSource.js:808: return
> this._evaluateAndWrap(callFrame.evaluate, callFrame, expression, objectGroup,
> true, injectCommandLineAPI, returnByValue, generatePreview);
> So we are still missing exceptionDetails in case of evaluation on call frame.
> This is something worth investigation, but might probably require complicated
v8
> changes. We could do this separately though.
Done.

> 
>
https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
> Source/core/inspector/InjectedScriptSource.js:847:
> _injectedScriptHostEvaluateThrowable: function(expression) {
> We could probably name this one _evaluateOrThrow
Done.

>
https://chromiumcodereview.appspot.com/289423002/diff/130001/Source/core/insp...
> Source/core/inspector/InjectedScriptSource.js:849: return
wrappedResult.result;
> Return early? :)
Fixed.

[vsevik, 2014-07-04 14:59:30]

This looks good to me now.
Pavel, Yury, could you please take a look?

https://codereview.chromium.org/289423002/diff/190001/Source/bindings/core/v8...
File Source/bindings/core/v8/EvaluateExceptionDetailsFactory.cpp (right):

https://codereview.chromium.org/289423002/diff/190001/Source/bindings/core/v8...
Source/bindings/core/v8/EvaluateExceptionDetailsFactory.cpp:7: *
ditto

https://codereview.chromium.org/289423002/diff/190001/Source/bindings/core/v8...
File Source/bindings/core/v8/EvaluateExceptionDetailsFactory.h (right):

https://codereview.chromium.org/289423002/diff/190001/Source/bindings/core/v8...
Source/bindings/core/v8/EvaluateExceptionDetailsFactory.h:16: * this software
without specific prior written permission.
Please use a short version of license header in new files, see
Source/devtools/front_end/toolbox/ResponsiveDesignView.js for an example.

[kozyatinskiy1, 2014-07-04 15:24:43]

>
https://codereview.chromium.org/289423002/diff/190001/Source/bindings/core/v8...
> File Source/bindings/core/v8/EvaluateExceptionDetailsFactory.cpp (right):
> 
>
https://codereview.chromium.org/289423002/diff/190001/Source/bindings/core/v8...
> Source/bindings/core/v8/EvaluateExceptionDetailsFactory.cpp:7: *
> ditto
> 
>
https://codereview.chromium.org/289423002/diff/190001/Source/bindings/core/v8...
> File Source/bindings/core/v8/EvaluateExceptionDetailsFactory.h (right):
> 
>
https://codereview.chromium.org/289423002/diff/190001/Source/bindings/core/v8...
> Source/bindings/core/v8/EvaluateExceptionDetailsFactory.h:16: * this software
> without specific prior written permission.
> Please use a short version of license header in new files, see
> Source/devtools/front_end/toolbox/ResponsiveDesignView.js for an example.
Done.

[aandrey, 2014-07-04 17:15:07]

https://codereview.chromium.org/289423002/diff/280001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/289423002/diff/280001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:668: if
(!wrappedResult.exceptionDetails)
wrappedResult can be undefined here

https://codereview.chromium.org/289423002/diff/280001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:669: return { wasThrown: false,
nit: wrap multiple lines into { ... }

https://codereview.chromium.org/289423002/diff/280001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:678: * @return {?}
{?} ->{*}

https://codereview.chromium.org/289423002/diff/280001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:691: * @param {Object=}
exceptionDetails
{!DebuggerAgent.ExceptionDetails=}

[aandrey, 2014-07-04 17:17:20]

BTW, how come this patch was not CC-ed to blink-reviews@ ?

[kozyatinskiy1, 2014-07-04 17:53:52]

On 2014/07/04 17:15:07, aandrey wrote:
>
https://codereview.chromium.org/289423002/diff/280001/Source/core/inspector/I...
> File Source/core/inspector/InjectedScriptSource.js (right):
> 
>
https://codereview.chromium.org/289423002/diff/280001/Source/core/inspector/I...
> Source/core/inspector/InjectedScriptSource.js:668: if
> (!wrappedResult.exceptionDetails)
> wrappedResult can be undefined here
Added check. However, there are two different calls of _evaluateAndWrap:
this._evaluateAndWrap(InjectedScriptHost.evaluate, InjectedScriptHost, ...)
this._evaluateAndWrap(callFrame.evaluate, callFrame, ...)
InjectedScriptHost.evaluate and callFrame.evaluate always return {result: ...,
exceptionDetails: ...|undefined }.

>
https://codereview.chromium.org/289423002/diff/280001/Source/core/inspector/I...
> Source/core/inspector/InjectedScriptSource.js:669: return { wasThrown: false,
> nit: wrap multiple lines into { ... }
Done.
 
>
https://codereview.chromium.org/289423002/diff/280001/Source/core/inspector/I...
> Source/core/inspector/InjectedScriptSource.js:678: * @return {?}
> {?} ->{*}
The original version of the function - InjectedScriptHost.evaluate was native
and not checked by the compiler.
 
>
https://codereview.chromium.org/289423002/diff/280001/Source/core/inspector/I...
> Source/core/inspector/InjectedScriptSource.js:691: * @param {Object=}
> exceptionDetails
> {!DebuggerAgent.ExceptionDetails=}
Done.

On 2014/07/04 17:17:20, aandrey wrote:
> BTW, how come this patch was not CC-ed to blink-reviews@ ?
I don't know =/ I didn't do anything special for this.

[aandrey, 2014-07-04 19:01:22]

https://codereview.chromium.org/289423002/diff/320001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/289423002/diff/320001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:668: if (!wrappedResult ||
!wrappedResult.exceptionDetails) {
wrong condition. you should not throw iff wrappedResult==undefiend, but rather
return a thrownValue

https://codereview.chromium.org/289423002/diff/320001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:672: } else
we also use brackets for one-liner else- block, if it were used for if- block

[kozyatinskiy1, 2014-07-05 10:52:46]

https://codereview.chromium.org/289423002/diff/320001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/289423002/diff/320001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:668: if (!wrappedResult ||
!wrappedResult.exceptionDetails) {
On 2014/07/04 19:01:22, aandrey wrote:
> wrong condition. you should not throw iff wrappedResult==undefiend, but rather
> return a thrownValue

Done.

https://codereview.chromium.org/289423002/diff/320001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:672: } else
On 2014/07/04 19:01:22, aandrey wrote:
> we also use brackets for one-liner else- block, if it were used for if- block

Done.

[aandrey, 2014-07-05 11:15:12]

https://codereview.chromium.org/289423002/diff/360001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/289423002/diff/360001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:668: if (wrappedResult !=
undefined || !wrappedResult.exceptionDetails) {
in fact, seems like I was wrong that wrappedResult can be undefined here, sorry
for that! I thought we catch and ignore exceptions in _evaluateOn and return
undefined, but we don't...

[kozyatinskiy1, 2014-07-05 11:24:32]

https://codereview.chromium.org/289423002/diff/360001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/289423002/diff/360001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:668: if (wrappedResult !=
undefined || !wrappedResult.exceptionDetails) {
On 2014/07/05 11:15:12, aandrey wrote:
> in fact, seems like I was wrong that wrappedResult can be undefined here,
sorry
> for that! I thought we catch and ignore exceptions in _evaluateOn and return
> undefined, but we don't...

Now, there are no way, when wrappedResult is undefined, maybe in future when we
add new evaluateOn_ calls with different parameters, method can return
undefined. But I suppose this patch isn't correct place to process this case.
Removed undefined wrappedResult processing.

[aandrey, 2014-07-05 11:30:33]

lgtm

please wait for vsevik

[kozyatinskiy1, 2014-07-05 13:34:14]

https://codereview.chromium.org/289423002/diff/410001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptCanvasModuleSource.js (right):

https://codereview.chromium.org/289423002/diff/410001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptCanvasModuleSource.js:4584: var
wrappedResult = InjectedScriptHost.evaluate("(" + stringId + ")");
Fixed: wrap InjectedScruptHost.evaluate call,

https://codereview.chromium.org/289423002/diff/410001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/289423002/diff/410001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptSource.js:745: this._lastResult =
wrappedResult.result;
Fixed: now $_ in console works correctly.

[pfeldman, 2014-07-05 20:11:06]

I would not want to land it as is: it converts InjectedScriptHost.evaluate from
simple eval to a method that returns an ad-hoc object.

https://codereview.chromium.org/289423002/diff/410001/Source/bindings/core/v8...
File Source/bindings/core/v8/EvaluateExceptionDetailsFactory.cpp (right):

https://codereview.chromium.org/289423002/diff/410001/Source/bindings/core/v8...
Source/bindings/core/v8/EvaluateExceptionDetailsFactory.cpp:14:
v8::Handle<v8::Object> createEvaluateExceptionDetails(v8::Handle<v8::Message>
message, v8::Isolate* isolate)
Converting a v8::Message into v8::Object of an ad-hoc structure does not sounds
like a generally useful thing. Others would be puzzled with this class lying in
bindings. Declare it in other of the call-sites and reuse in another?

https://codereview.chromium.org/289423002/diff/410001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (left):

https://codereview.chromium.org/289423002/diff/410001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:316: void
V8InjectedScriptHost::evaluateMethodCustom(const
v8::FunctionCallbackInfo<v8::Value>& info)
InjectedScriptHost.evaluate is just a raw window.eval that page can't override.
I'd really like to keep it this way. What are you trying to do? Adding a bug id
with the sample test case would help.

https://codereview.chromium.org/289423002/diff/410001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptCanvasModuleSource.js (right):

https://codereview.chromium.org/289423002/diff/410001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptCanvasModuleSource.js:4585: if
(wrappedResult.exceptionDetails)
So this code is weird (you have a check, but return .result in any case). Also,
we don't expect error to happen here, error in the id means that the client is
using malformed id.

[kozyatinskiy1, 2014-07-07 12:20:47]

Logic was moved to InjectedScriptHost.evaluateWithDetails.
InjectedScriptHost.evaluate was restored.

@vsevik, @aandrey, @pfeldman please take a look.

[kozyatinskiy1, 2014-07-11 19:50:49]

@vsevik, @aandrey, @pfeldman: friendly ping!

[kozyatinskiy1, 2014-07-14 11:51:00]

Renamed InjectedScriptHost.evaluate to InjectedScriptHost.eval.
Renamed InjectedScriptHost.evaluateWithDetails, JavaScriptCallFrame.evaluate to
InjectedScriptHost.evaluateWithExceptionDetails and
JavaScriptCallFrame.evaluateWithExceptionDetails.

@vsevik, @aandrey, @pfeldman, please take a look.

[vsevik, 2014-07-14 11:52:21]

lgtm

[pfeldman, 2014-07-14 11:57:26]

owners lgtm

[vsevik, 2014-07-14 11:58:35]

The CQ bit was checked by vsevik@chromium.org

[commit-bot: I haz the power, 2014-07-14 11:59:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kozyatinskiy@google.com/289423002/450001

[commit-bot: I haz the power, 2014-07-14 12:54:12]

Change committed as 178051

[Justin Novosad, 2014-07-14 16:15:05]

A revert of this CL has been created in
https://codereview.chromium.org/389363002/ by junov@chromium.org.

The reason for reverting is: Speculative revert for failing telemetry unit
tests. Tests that evaluate scripts that are purposely designed to crash the tab
are no longer passing. By intersecting the Mac and windows builder regression
ranges I obtained the following range: r178051-r178054.  This is the only change
in that range that seems related to script execution..

[Justin Novosad, 2014-07-14 19:07:15]

The revert successfully resolved the problems with telemetry unit test.

[kozyatinskiy1, 2014-07-14 19:19:12]

On 2014/07/14 19:07:15, junov wrote:
> The revert successfully resolved the problems with telemetry unit test.

Thank you, I've detected problem and fixed it, I will commit fixed version
tomorrow after review.

[vsevik, 2014-07-15 10:58:20]

The CQ bit was checked by vsevik@chromium.org

[commit-bot: I haz the power, 2014-07-15 10:59:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kozyatinskiy@google.com/289423002/470001

[commit-bot: I haz the power, 2014-07-15 12:14:22]

Change committed as 178164