From 6ceee5369846ec2c95e9262bf6f55dd07749e8e8 Mon Sep 17 00:00:00 2001 From: Alexey Kozyatinskiy Date: Mon, 14 Aug 2017 15:21:45 -0700 Subject: [PATCH] [inspector] aligned Runtime.evaluate(awaitPromise: true) with await semantic This one allows us to support custom promises implementation. With awaitPromise flag Runtime.evaluate awaits Promise.resolve(). This also allows to await for any non-Promise value, similar to await expression, which is more convenient for most protocol users. R=dgozman@chromium.org Bug: chromium:755104 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Iee798b33b6fb7de7d393372e164c0481d1bbf7eb Reviewed-on: https://chromium-review.googlesource.com/614308 Commit-Queue: Aleksey Kozyatinskiy Reviewed-by: Dmitry Gozman Cr-Commit-Position: refs/heads/master@{#47354} --- src/inspector/injected-script.cc | 36 ++++++++++--------- src/inspector/injected-script.h | 1 - src/inspector/js_protocol.json | 6 ++-- src/inspector/v8-runtime-agent-impl.cc | 19 +++++----- .../call-function-on-async-expected.txt | 10 ++++-- .../runtime/call-function-on-async.js | 4 +-- .../runtime/evaluate-async-expected.txt | 33 ++++++++++++----- test/inspector/runtime/evaluate-async.js | 10 +++++- .../runtime/run-script-async-expected.txt | 12 ++++--- test/inspector/runtime/run-script-async.js | 2 +- 10 files changed, 86 insertions(+), 47 deletions(-) diff --git a/src/inspector/injected-script.cc b/src/inspector/injected-script.cc index bf930ed120..ea5b70e115 100644 --- a/src/inspector/injected-script.cc +++ b/src/inspector/injected-script.cc @@ -60,11 +60,21 @@ using protocol::Maybe; class InjectedScript::ProtocolPromiseHandler { public: static bool add(V8InspectorSessionImpl* session, - v8::Local context, - v8::Local promise, - const String16& notPromiseError, int executionContextId, - const String16& objectGroup, bool returnByValue, - bool generatePreview, EvaluateCallback* callback) { + v8::Local context, v8::Local value, + int executionContextId, const String16& objectGroup, + bool returnByValue, bool generatePreview, + EvaluateCallback* callback) { + v8::Local resolver; + if (!v8::Promise::Resolver::New(context).ToLocal(&resolver)) { + callback->sendFailure(Response::InternalError()); + return false; + } + if (!resolver->Resolve(context, value).FromMaybe(false)) { + callback->sendFailure(Response::InternalError()); + return false; + } + + v8::Local promise = resolver->GetPromise(); V8InspectorImpl* inspector = session->inspector(); ProtocolPromiseHandler* handler = new ProtocolPromiseHandler(session, executionContextId, objectGroup, @@ -456,24 +466,18 @@ std::unique_ptr InjectedScript::wrapTable( void InjectedScript::addPromiseCallback( V8InspectorSessionImpl* session, v8::MaybeLocal value, - const String16& notPromiseError, const String16& objectGroup, - bool returnByValue, bool generatePreview, + const String16& objectGroup, bool returnByValue, bool generatePreview, std::unique_ptr callback) { if (value.IsEmpty()) { callback->sendFailure(Response::InternalError()); return; } - if (!value.ToLocalChecked()->IsPromise()) { - callback->sendFailure(Response::Error(notPromiseError)); - return; - } v8::MicrotasksScope microtasksScope(m_context->isolate(), v8::MicrotasksScope::kRunMicrotasks); - if (ProtocolPromiseHandler::add(session, m_context->context(), - value.ToLocalChecked().As(), - notPromiseError, m_context->contextId(), - objectGroup, returnByValue, generatePreview, - callback.get())) { + if (ProtocolPromiseHandler::add( + session, m_context->context(), value.ToLocalChecked(), + m_context->contextId(), objectGroup, returnByValue, generatePreview, + callback.get())) { m_evaluateCallbacks.insert(callback.release()); } } diff --git a/src/inspector/injected-script.h b/src/inspector/injected-script.h index 52ed7ce558..e5c393df5b 100644 --- a/src/inspector/injected-script.h +++ b/src/inspector/injected-script.h @@ -99,7 +99,6 @@ class InjectedScript final { void addPromiseCallback(V8InspectorSessionImpl* session, v8::MaybeLocal value, - const String16& notPromiseError, const String16& objectGroup, bool returnByValue, bool generatePreview, std::unique_ptr callback); diff --git a/src/inspector/js_protocol.json b/src/inspector/js_protocol.json index b61ce88bb3..446d51e0d2 100644 --- a/src/inspector/js_protocol.json +++ b/src/inspector/js_protocol.json @@ -218,7 +218,7 @@ { "name": "returnByValue", "type": "boolean", "optional": true, "description": "Whether the result is expected to be a JSON object that should be sent by value." }, { "name": "generatePreview", "type": "boolean", "optional": true, "experimental": true, "description": "Whether preview should be generated for the result." }, { "name": "userGesture", "type": "boolean", "optional": true, "experimental": true, "description": "Whether execution should be treated as initiated by user in the UI." }, - { "name": "awaitPromise", "type": "boolean", "optional":true, "description": "Whether execution should wait for promise to be resolved. If the result of evaluation is not a Promise, it's considered to be an error." } + { "name": "awaitPromise", "type": "boolean", "optional":true, "description": "Whether execution should await for resulting value and return once awaited promise is resolved." } ], "returns": [ { "name": "result", "$ref": "RemoteObject", "description": "Evaluation result." }, @@ -249,7 +249,7 @@ { "name": "returnByValue", "type": "boolean", "optional": true, "description": "Whether the result is expected to be a JSON object which should be sent by value." }, { "name": "generatePreview", "type": "boolean", "optional": true, "experimental": true, "description": "Whether preview should be generated for the result." }, { "name": "userGesture", "type": "boolean", "optional": true, "experimental": true, "description": "Whether execution should be treated as initiated by user in the UI." }, - { "name": "awaitPromise", "type": "boolean", "optional":true, "description": "Whether execution should wait for promise to be resolved. If the result of evaluation is not a Promise, it's considered to be an error." } + { "name": "awaitPromise", "type": "boolean", "optional":true, "description": "Whether execution should await for resulting value and return once awaited promise is resolved." } ], "returns": [ { "name": "result", "$ref": "RemoteObject", "description": "Call result." }, @@ -336,7 +336,7 @@ { "name": "includeCommandLineAPI", "type": "boolean", "optional": true, "description": "Determines whether Command Line API should be available during the evaluation." }, { "name": "returnByValue", "type": "boolean", "optional": true, "description": "Whether the result is expected to be a JSON object which should be sent by value." }, { "name": "generatePreview", "type": "boolean", "optional": true, "description": "Whether preview should be generated for the result." }, - { "name": "awaitPromise", "type": "boolean", "optional": true, "description": "Whether execution should wait for promise to be resolved. If the result of evaluation is not a Promise, it's considered to be an error." } + { "name": "awaitPromise", "type": "boolean", "optional": true, "description": "Whether execution should await for resulting value and return once awaited promise is resolved." } ], "returns": [ { "name": "result", "$ref": "RemoteObject", "description": "Run result." }, diff --git a/src/inspector/v8-runtime-agent-impl.cc b/src/inspector/v8-runtime-agent-impl.cc index 6255eac444..2641e1f4dc 100644 --- a/src/inspector/v8-runtime-agent-impl.cc +++ b/src/inspector/v8-runtime-agent-impl.cc @@ -190,9 +190,8 @@ void V8RuntimeAgentImpl::evaluate( return; } scope.injectedScript()->addPromiseCallback( - m_session, maybeResultValue, "Result of the evaluation is not a promise", - objectGroup.fromMaybe(""), returnByValue.fromMaybe(false), - generatePreview.fromMaybe(false), + m_session, maybeResultValue, objectGroup.fromMaybe(""), + returnByValue.fromMaybe(false), generatePreview.fromMaybe(false), EvaluateCallbackWrapper::wrap(std::move(callback))); } @@ -206,10 +205,14 @@ void V8RuntimeAgentImpl::awaitPromise( callback->sendFailure(response); return; } + if (!scope.object()->IsPromise()) { + callback->sendFailure( + Response::Error("Could not find promise with given id")); + return; + } scope.injectedScript()->addPromiseCallback( - m_session, scope.object(), "Could not find promise with given id", - scope.objectGroupName(), returnByValue.fromMaybe(false), - generatePreview.fromMaybe(false), + m_session, scope.object(), scope.objectGroupName(), + returnByValue.fromMaybe(false), generatePreview.fromMaybe(false), EvaluateCallbackWrapper::wrap(std::move(callback))); } @@ -304,8 +307,7 @@ void V8RuntimeAgentImpl::callFunctionOn( } scope.injectedScript()->addPromiseCallback( - m_session, maybeResultValue, - "Result of the function call is not a promise", scope.objectGroupName(), + m_session, maybeResultValue, scope.objectGroupName(), returnByValue.fromMaybe(false), generatePreview.fromMaybe(false), EvaluateCallbackWrapper::wrap( std::move(callback))); @@ -514,7 +516,6 @@ void V8RuntimeAgentImpl::runScript( } scope.injectedScript()->addPromiseCallback( m_session, maybeResultValue.ToLocalChecked(), - "Result of the script execution is not a promise", objectGroup.fromMaybe(""), returnByValue.fromMaybe(false), generatePreview.fromMaybe(false), EvaluateCallbackWrapper::wrap(std::move(callback))); diff --git a/test/inspector/runtime/call-function-on-async-expected.txt b/test/inspector/runtime/call-function-on-async-expected.txt index f396b0540e..5212da8033 100644 --- a/test/inspector/runtime/call-function-on-async-expected.txt +++ b/test/inspector/runtime/call-function-on-async-expected.txt @@ -69,8 +69,14 @@ Running test: testExceptionInFunctionExpression Running test: testFunctionReturnNotPromise { - code : -32000 - message : Result of the function call is not a promise + id : + result : { + result : { + description : 239 + type : number + value : 239 + } + } } Running test: testFunctionReturnResolvedPromiseReturnByValue diff --git a/test/inspector/runtime/call-function-on-async.js b/test/inspector/runtime/call-function-on-async.js index cce28565c1..4f701027eb 100644 --- a/test/inspector/runtime/call-function-on-async.js +++ b/test/inspector/runtime/call-function-on-async.js @@ -50,10 +50,10 @@ InspectorTest.runTestSuite([ "({a : 1})", "(function() { return 239; })", [], - /* returnByValue */ false, + /* returnByValue */ true, /* generatePreview */ false, /* awaitPromise */ true) - .then((result) => InspectorTest.logMessage(result.error)) + .then((result) => InspectorTest.logMessage(result)) .then(() => next()); }, diff --git a/test/inspector/runtime/evaluate-async-expected.txt b/test/inspector/runtime/evaluate-async-expected.txt index 9442280c98..763b4295c7 100644 --- a/test/inspector/runtime/evaluate-async-expected.txt +++ b/test/inspector/runtime/evaluate-async-expected.txt @@ -129,20 +129,25 @@ Running test: testRejectedPromiseWithSyntaxError Running test: testPrimitiveValueInsteadOfPromise { - error : { - code : -32000 - message : Result of the evaluation is not a promise - } id : + result : { + result : { + type : boolean + value : true + } + } } Running test: testObjectInsteadOfPromise { - error : { - code : -32000 - message : Result of the evaluation is not a promise - } id : + result : { + result : { + type : object + value : { + } + } + } } Running test: testPendingPromise @@ -182,6 +187,18 @@ Running test: testExceptionInEvaluate } } +Running test: testThenableJob +{ + id : + result : { + result : { + description : 42 + type : number + value : 42 + } + } +} + Running test: testLastEvaluatedResult { id : diff --git a/test/inspector/runtime/evaluate-async.js b/test/inspector/runtime/evaluate-async.js index 6df2b05331..385b3f6fb4 100644 --- a/test/inspector/runtime/evaluate-async.js +++ b/test/inspector/runtime/evaluate-async.js @@ -80,7 +80,8 @@ InspectorTest.runAsyncTestSuite([ { InspectorTest.logMessage(await Protocol.Runtime.evaluate({ expression: "({})", - awaitPromise: true + awaitPromise: true, + returnByValue: true })); }, @@ -101,6 +102,13 @@ InspectorTest.runAsyncTestSuite([ })); }, + async function testThenableJob() + { + InspectorTest.logMessage(await Protocol.Runtime.evaluate({ + expression: '({then: resolve => resolve(42)})', + awaitPromise: true})); + }, + async function testLastEvaluatedResult() { InspectorTest.logMessage(await Protocol.Runtime.evaluate({ diff --git a/test/inspector/runtime/run-script-async-expected.txt b/test/inspector/runtime/run-script-async-expected.txt index fc1ce0eb97..29b9c526dd 100644 --- a/test/inspector/runtime/run-script-async-expected.txt +++ b/test/inspector/runtime/run-script-async-expected.txt @@ -141,11 +141,15 @@ Running test: testRunScriptReturnByValue Running test: testAwaitNotPromise { - error : { - code : -32000 - message : Result of the script execution is not a promise - } id : + result : { + result : { + type : object + value : { + a : 1 + } + } + } } Running test: testAwaitResolvedPromise diff --git a/test/inspector/runtime/run-script-async.js b/test/inspector/runtime/run-script-async.js index 484ad37ef9..7448337387 100644 --- a/test/inspector/runtime/run-script-async.js +++ b/test/inspector/runtime/run-script-async.js @@ -82,7 +82,7 @@ InspectorTest.runTestSuite([ { Protocol.Runtime.enable() .then(() => Protocol.Runtime.compileScript({ expression: "({a:1})", sourceURL: "boo.js", persistScript: true })) - .then((result) => Protocol.Runtime.runScript({ scriptId: result.result.scriptId, awaitPromise: true })) + .then((result) => Protocol.Runtime.runScript({ scriptId: result.result.scriptId, awaitPromise: true, returnByValue: true })) .then((result) => InspectorTest.logMessage(result)) .then(() => Protocol.Runtime.disable()) .then(() => next());