From 84126a4f23e0ce0c4596ec9a66c60e54e46017a0 Mon Sep 17 00:00:00 2001 From: bughit Date: Mon, 2 Mar 2020 19:11:40 -0500 Subject: [PATCH] feat: optional typically sync callback for WebFrame#executeJavaScript* (#21423) --- docs/api/web-frame.md | 31 +++-- shell/renderer/api/electron_api_web_frame.cc | 77 ++++++++++--- spec/api-web-frame-spec.js | 112 +++++++++++++++++++ 3 files changed, 197 insertions(+), 23 deletions(-) diff --git a/docs/api/web-frame.md b/docs/api/web-frame.md index 7b459244add4..779434d086c2 100644 --- a/docs/api/web-frame.md +++ b/docs/api/web-frame.md @@ -122,13 +122,20 @@ by its key, which is returned from `webFrame.insertCSS(css)`. Inserts `text` to the focused element. -### `webFrame.executeJavaScript(code[, userGesture])` +### `webFrame.executeJavaScript(code[, userGesture, callback])` * `code` String * `userGesture` Boolean (optional) - Default is `false`. +* `callback` Function (optional) - Called after script has been executed. Unless + the frame is suspended (e.g. showing a modal alert), execution will be + synchronous and the callback will be invoked before the method returns. For + compatibility with an older version of this method, the error parameter is + second. + * `result` Any + * `error` Error -Returns `Promise` - A promise that resolves with the result of the executed code -or is rejected if the result of the code is a rejected promise. +Returns `Promise` - A promise that resolves with the result of the executed +code or is rejected if execution throws or results in a rejected promise. Evaluates `code` in page. @@ -136,14 +143,24 @@ In the browser window some HTML APIs like `requestFullScreen` can only be invoked by a gesture from the user. Setting `userGesture` to `true` will remove this limitation. -### `webFrame.executeJavaScriptInIsolatedWorld(worldId, scripts[, userGesture])` +### `webFrame.executeJavaScriptInIsolatedWorld(worldId, scripts[, userGesture, callback])` -* `worldId` Integer - The ID of the world to run the javascript in, `0` is the default world, `999` is the world used by Electrons `contextIsolation` feature. You can provide any integer here. +* `worldId` Integer - The ID of the world to run the javascript + in, `0` is the default main world (where content runs), `999` is the + world used by Electron's `contextIsolation` feature. Accepts values + in the range 1..536870911. * `scripts` [WebSource[]](structures/web-source.md) * `userGesture` Boolean (optional) - Default is `false`. +* `callback` Function (optional) - Called after script has been executed. Unless + the frame is suspended (e.g. showing a modal alert), execution will be + synchronous and the callback will be invoked before the method returns. For + compatibility with an older version of this method, the error parameter is + second. + * `result` Any + * `error` Error -Returns `Promise` - A promise that resolves with the result of the executed code -or is rejected if the result of the code is a rejected promise. +Returns `Promise` - A promise that resolves with the result of the executed +code or is rejected if execution throws or results in a rejected promise. Works like `executeJavaScript` but evaluates `scripts` in an isolated context. diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc index d6fcc3551a98..924bddd73322 100644 --- a/shell/renderer/api/electron_api_web_frame.cc +++ b/shell/renderer/api/electron_api_web_frame.cc @@ -15,6 +15,7 @@ #include "services/service_manager/public/cpp/interface_provider.h" #include "shell/common/api/api.mojom.h" #include "shell/common/gin_converters/blink_converter.h" +#include "shell/common/gin_converters/callback_converter.h" #include "shell/common/gin_helper/dictionary.h" #include "shell/common/gin_helper/promise.h" #include "shell/common/node_includes.h" @@ -110,32 +111,58 @@ class RenderFrameStatus final : public content::RenderFrameObserver { class ScriptExecutionCallback : public blink::WebScriptExecutionCallback { public: + // for compatibility with the older version of this, error is after result + using CompletionCallback = + base::OnceCallback& result, + const v8::Local& error)>; + explicit ScriptExecutionCallback( - gin_helper::Promise> promise) - : promise_(std::move(promise)) {} + gin_helper::Promise> promise, + CompletionCallback callback) + : promise_(std::move(promise)), callback_(std::move(callback)) {} + ~ScriptExecutionCallback() override = default; void Completed( const blink::WebVector>& result) override { + v8::Isolate* isolate = v8::Isolate::GetCurrent(); if (!result.empty()) { if (!result[0].IsEmpty()) { // Right now only single results per frame is supported. + if (!callback_.is_null()) + std::move(callback_).Run(result[0], v8::Undefined(isolate)); promise_.Resolve(result[0]); } else { - promise_.RejectWithErrorMessage( + const char* error_message = "Script failed to execute, this normally means an error " - "was thrown. Check the renderer console for the error."); + "was thrown. Check the renderer console for the error."; + if (!callback_.is_null()) { + std::move(callback_).Run( + v8::Undefined(isolate), + v8::Exception::Error( + v8::String::NewFromUtf8(isolate, error_message) + .ToLocalChecked())); + } + promise_.RejectWithErrorMessage(error_message); } } else { - promise_.RejectWithErrorMessage( + const char* error_message = "WebFrame was removed before script could run. This normally means " - "the underlying frame was destroyed"); + "the underlying frame was destroyed"; + if (!callback_.is_null()) { + std::move(callback_).Run( + v8::Undefined(isolate), + v8::Exception::Error(v8::String::NewFromUtf8(isolate, error_message) + .ToLocalChecked())); + } + promise_.RejectWithErrorMessage(error_message); } delete this; } private: gin_helper::Promise> promise_; + CompletionCallback callback_; DISALLOW_COPY_AND_ASSIGN(ScriptExecutionCallback); }; @@ -373,9 +400,14 @@ v8::Local ExecuteJavaScript(gin_helper::Arguments* args, bool has_user_gesture = false; args->GetNext(&has_user_gesture); + ScriptExecutionCallback::CompletionCallback completion_callback; + args->GetNext(&completion_callback); + GetRenderFrame(window)->GetWebFrame()->RequestExecuteScriptAndReturnValue( blink::WebScriptSource(blink::WebString::FromUTF16(code)), - has_user_gesture, new ScriptExecutionCallback(std::move(promise))); + has_user_gesture, + new ScriptExecutionCallback(std::move(promise), + std::move(completion_callback))); return handle; } @@ -389,6 +421,16 @@ v8::Local ExecuteJavaScriptInIsolatedWorld( gin_helper::Promise> promise(isolate); v8::Local handle = promise.GetHandle(); + bool has_user_gesture = false; + args->GetNext(&has_user_gesture); + + blink::WebLocalFrame::ScriptExecutionType scriptExecutionType = + blink::WebLocalFrame::kSynchronous; + args->GetNext(&scriptExecutionType); + + ScriptExecutionCallback::CompletionCallback completion_callback; + args->GetNext(&completion_callback); + std::vector sources; for (const auto& script : scripts) { @@ -399,7 +441,15 @@ v8::Local ExecuteJavaScriptInIsolatedWorld( script.Get("startLine", &start_line); if (!script.Get("code", &code)) { - promise.RejectWithErrorMessage("Invalid 'code'"); + const char* error_message = "Invalid 'code'"; + if (!completion_callback.is_null()) { + std::move(completion_callback) + .Run(v8::Undefined(isolate), + v8::Exception::Error( + v8::String::NewFromUtf8(isolate, error_message) + .ToLocalChecked())); + } + promise.RejectWithErrorMessage(error_message); return handle; } @@ -408,19 +458,14 @@ v8::Local ExecuteJavaScriptInIsolatedWorld( blink::WebURL(GURL(url)), start_line)); } - bool has_user_gesture = false; - args->GetNext(&has_user_gesture); - - blink::WebLocalFrame::ScriptExecutionType scriptExecutionType = - blink::WebLocalFrame::kSynchronous; - args->GetNext(&scriptExecutionType); - // Debugging tip: if you see a crash stack trace beginning from this call, // then it is very likely that some exception happened when executing the // "content_script/init.js" script. GetRenderFrame(window)->GetWebFrame()->RequestExecuteScriptInIsolatedWorld( world_id, &sources.front(), sources.size(), has_user_gesture, - scriptExecutionType, new ScriptExecutionCallback(std::move(promise))); + scriptExecutionType, + new ScriptExecutionCallback(std::move(promise), + std::move(completion_callback))); return handle; } diff --git a/spec/api-web-frame-spec.js b/spec/api-web-frame-spec.js index 6f7af8a2509c..8fb52fd822a5 100644 --- a/spec/api-web-frame-spec.js +++ b/spec/api-web-frame-spec.js @@ -25,4 +25,116 @@ describe('webFrame module', function () { it('findFrameByRoutingId() does not crash when not found', () => { expect(webFrame.findFrameByRoutingId(-1)).to.be.null() }) + + describe('executeJavaScript', () => { + let childFrameElement, childFrame + + before(() => { + childFrameElement = document.createElement('iframe') + document.body.appendChild(childFrameElement) + childFrame = webFrame.firstChild + }) + + after(() => { + childFrameElement.remove() + }) + + it('executeJavaScript() yields results via a promise and a sync callback', done => { + let callbackResult, callbackError + + childFrame + .executeJavaScript('1 + 1', (result, error) => { + callbackResult = result + callbackError = error + }) + .then( + promiseResult => { + expect(promiseResult).to.equal(2) + done() + }, + promiseError => { + done(promiseError) + } + ) + + expect(callbackResult).to.equal(2) + expect(callbackError).to.be.undefined() + }) + + it('executeJavaScriptInIsolatedWorld() yields results via a promise and a sync callback', done => { + let callbackResult, callbackError + + childFrame + .executeJavaScriptInIsolatedWorld(999, [{ code: '1 + 1' }], (result, error) => { + callbackResult = result + callbackError = error + }) + .then( + promiseResult => { + expect(promiseResult).to.equal(2) + done() + }, + promiseError => { + done(promiseError) + } + ) + + expect(callbackResult).to.equal(2) + expect(callbackError).to.be.undefined() + }) + + it('executeJavaScript() yields errors via a promise and a sync callback', done => { + let callbackResult, callbackError + + childFrame + .executeJavaScript('thisShouldProduceAnError()', (result, error) => { + callbackResult = result + callbackError = error + }) + .then( + promiseResult => { + done(new Error('error is expected')) + }, + promiseError => { + expect(promiseError).to.be.an('error') + done() + } + ) + + expect(callbackResult).to.be.undefined() + expect(callbackError).to.be.an('error') + }) + + // executeJavaScriptInIsolatedWorld is failing to detect exec errors and is neither + // rejecting nor passing the error to the callback. This predates the reintroduction + // of the callback so will not be fixed as part of the callback PR + // if/when this is fixed the test can be uncommented. + // + // it('executeJavaScriptInIsolatedWorld() yields errors via a promise and a sync callback', done => { + // let callbackResult, callbackError + // + // childFrame + // .executeJavaScriptInIsolatedWorld(999, [{ code: 'thisShouldProduceAnError()' }], (result, error) => { + // callbackResult = result + // callbackError = error + // }) + // .then( + // promiseResult => { + // done(new Error('error is expected')) + // }, + // promiseError => { + // expect(promiseError).to.be.an('error') + // done() + // } + // ) + // + // expect(callbackResult).to.be.undefined() + // expect(callbackError).to.be.an('error') + // }) + + it('executeJavaScript(InIsolatedWorld) can be used without a callback', async () => { + expect(await webFrame.executeJavaScript('1 + 1')).to.equal(2) + expect(await webFrame.executeJavaScriptInIsolatedWorld(999, [{ code: '1 + 1' }])).to.equal(2) + }) + }) })