feat: expose sync js exec interface on WebFrame (try 2) #21371
Conversation
There was a problem hiding this comment.
I fundamentally don't think we should be exposing this as API. It is abusing an implementation details of Chromiums API. There is no way we can guarantee it will remain sync and by making this API we are committing to supporting it.
I am firmly against this API being added.
@electron/wg-api for reference
|
|
||
| // this can only happen if the frame is not in the | ||
| // mojom::FrameLifecycleState::kRunning state | ||
| args->ThrowError("execution did not complete synchronously"); |
There was a problem hiding this comment.
This makes no sense as an API, their is no way the app can handle this in any useful way.
There was a problem hiding this comment.
I have never seen the top frame not in the mojom::FrameLifecycleState::kRunning state.
Can you point out when it wouldn't be?
When a frame is in the kRunning state, execution is guaranteed to be synchronous, that's not an "implementation detail" it's a public api guarantee
enum ScriptExecutionType {
// Execute script synchronously, unless the page is suspended.
kSynchronous,
// Execute script asynchronously.
kAsynchronous,
// Execute script asynchronously, blocking the window.onload event.
kAsynchronousBlockingOnload
};Only if the page is suspended, would execution not be synchronous.
If the top frame being suspended is an unavoidable, unpredictable possibility outside of one's control, which seems unlikely, I would agree with you. Otherwise these sync methods are useful and will work with 100% reliability and so should not be rejected.
There was a problem hiding this comment.
I believe this could happen if the window is displaying an alert or similar: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/page/page.h?l=237-244&rcl=879cf27d2d56f2ff63e2ba7041bfabdd2d96deb3
There was a problem hiding this comment.
I'll test it. If so, and if this is something that could be detected, the sync methods could fail early. That would not be unreasonable behavior.
There was a problem hiding this comment.
I'd also test what happens if the executed script calls alert; that seems like it would pause the script also and prevent a synchronous return from happening.
|
|
||
| // this can only happen if the frame is not in the | ||
| // mojom::FrameLifecycleState::kRunning state | ||
| args->ThrowError("execution did not complete synchronously"); |
There was a problem hiding this comment.
I believe this could happen if the window is displaying an alert or similar: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/page/page.h?l=237-244&rcl=879cf27d2d56f2ff63e2ba7041bfabdd2d96deb3
|
@bughit I've come up with a middle ground that should suit your use case. Implement this API to support both callbacks and promises. Then if you really want to do this, you can implement the "fake sync" behavior in JS by using the callback. |
Do you mean to support the old behavior where you could pass in a callback? Seems reasonable to me, and as a bonus old code that depends on the synchronous behavior would continue to work unchanged. |
|
@MarshallOfSound That works for me, I am already using the "fake sync" approach in v4. Please give more details of how this should work.
|
|
Yep exactly that. The current API should work like it currently does (return a promise). But if a callback is also provided it will be called in addition to resolving the promise |
|
could use some help with callback conversion callback signature // for compatibility with the older version of this, error is after result
using CompletionCallback =
base::Callback<void(const v8::Local<v8::Value>& result,
const v8::Local<v8::Value>& error)>;which results in a conversion error |
|
needed "shell/common/gin_converters/callback_converter.h" |
|
closing in favor of #21423 |
Description of Change
Before the promisification (#17312) of
WebFrame#executeJavaScript*their interface was async (callback) but behavior was sync (callback called before the methods returned). So they could be used in sync code (or async, you could wrap them in a promise yourself).The switch from a callback to a promise interface made it impossible to use them in sync mode (#19161), which was a breaking change without a workaround.
This PR exposes the sync js exec methods on WebFrame, providing a workaround for code that used the original methods synchronously. In general, a sync in-process js eval makes perfect sense, because it's sync under the hood.
This attempt uses the same underlying methods
WebLocalFrame::RequestExecuteScript*as the "async" versions.Fixes #19161
Checklist
npm testpassesRelease Notes
Notes: add sync javascript execution methods to WebFrame