Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not wait on promise returned by remote APIs #18990

Merged
merged 2 commits into from Jul 10, 2019

Conversation

@zcbenz
Copy link
Member

zcbenz commented Jun 26, 2019

Description of Change

Close #18638.

This PR fixes the bug in ipcMainUtils.handle that, the helper waits on Promise returned by handler when it shouldn't.

For the original issue, ipcMainUtils.handle waits on the Promised returned by WebContents.loadURL, resulting in setting src being blocked until the URL is loaded.

Checklist

Release Notes

Notes: Fix setting src on <webview> being too slow.

@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Jun 26, 2019

I get the problem we are trying to solve, however I am not sure if the solution is correct. I need to think about it a bit more

@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Jun 26, 2019

@miniak Another way to fix it is to split ipcMainUtils.handle into 2 parts, one handling normal functions and one handling async functions. Would that feel right to you?

@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Jun 27, 2019

I have refactored the code to add ipcMainUtils.handleAsync, so the behavior of the helper is more clear.

@zcbenz zcbenz requested a review from miniak Jun 27, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 27, 2019
@zcbenz zcbenz mentioned this pull request Jun 28, 2019
5 of 6 tasks complete
@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Jun 28, 2019

@zcbenz We actually had handle and handleSync before :) I've merged them into a single method, as I wanted the caller to decide whether to use invoke or invokeSync and have the handler be generic.

Copy link
Contributor

miniak left a comment

I think we should handle this without changing the IPC helpers. I am prototyping a solution, which I will share shortly.

@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Jun 28, 2019

@miniak The results of asyncMethods are still being awaited, so <webview>.loadURL still won't work correctly in first version.

I'm good with the second version.

@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Jun 28, 2019

@zcbenz it is awaited but it does not block the renderer. It transfers the Promise to the caller side properly.

@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Jun 28, 2019

@miniak Yeah you are right, both versions should work then.

@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Jul 1, 2019

@miniak Do you want to open a PR for your solution?

@codebytere codebytere requested a review from miniak Jul 2, 2019
@zcbenz zcbenz force-pushed the ipc-handle-promise branch from e2b68b7 to c56b5b6 Jul 3, 2019
@miniak
miniak approved these changes Jul 3, 2019
Copy link
Contributor

jkleinsc left a comment

I think we need a doc change for loadURL.

@@ -54,6 +53,7 @@ exports.syncMethods = new Set([
])

exports.asyncMethods = new Set([
'loadURL',

This comment has been minimized.

Copy link
@jkleinsc

jkleinsc Jul 3, 2019

Contributor

Shouldn't there be a corresponding doc change to note the loadURL is now async?

This comment has been minimized.

Copy link
@zcbenz

zcbenz Jul 3, 2019

Author Member

@jkleinsc The <webview>.loadURL was actually asynchronous while it was under put syncMethods, the WebContents.loadURL API works asynchronously that it only starts the loading and returns immediately without waiting for anything.

So with this PR <webview>.loadURL won't change any documented behavior.

The side effect of this PR is making <webview>.loadURL return a Promise, but since we are backporting it to 6.0, I think it is probably better keeping it undocumented otherwise it would be a feat.

This comment has been minimized.

Copy link
@jkleinsc

jkleinsc Jul 9, 2019

Contributor

@zcbenz I understand what you are saying, but what happens then is we do not end documenting this behaviour. I would rather see the doc change made and go through @electron/wg-releases to approve moving this feat to 6.0. Given the issue it solves I don't think it will be a problem to get @electron/wg-releases approval.

@miniak miniak mentioned this pull request Jul 5, 2019
3 of 3 tasks complete
@jkleinsc jkleinsc requested a review from electron/wg-releases Jul 5, 2019
@zcbenz zcbenz requested a review from jkleinsc Jul 9, 2019
Copy link
Contributor

jkleinsc left a comment

Still think we need the doc change.

@@ -54,6 +53,7 @@ exports.syncMethods = new Set([
])

exports.asyncMethods = new Set([
'loadURL',

This comment has been minimized.

Copy link
@jkleinsc

jkleinsc Jul 9, 2019

Contributor

@zcbenz I understand what you are saying, but what happens then is we do not end documenting this behaviour. I would rather see the doc change made and go through @electron/wg-releases to approve moving this feat to 6.0. Given the issue it solves I don't think it will be a problem to get @electron/wg-releases approval.

@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Jul 9, 2019

I agree @jkleinsc we should document the api signature change for master, for the backport we can do two things either, take this as a feature and get it through the release-wg process which should be easy based on

https://github.com/electron/governance/tree/master/wg-releases#feature-backport-requests-for-major-versions-in-beta

Backport requests after Week 3 will only be considered if there is a very good reason. For backport requests please submit the link to the PR and reason for backport to the Releases WG agenda for consideration.

or the backport PR can create a separate handler for loadURL that still uses the ipcRendererUtils.invoke but maintains the current return value type.

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Jul 10, 2019

A maintainer has manually backported this PR to "6-0-x", please check out #19190

@trop trop bot added in-flight/6-0-x and removed target/6-1-x labels Jul 10, 2019
@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Jul 10, 2019

I have added documentation for the return value change.

For the backport (#19190), I found the code would be rather dirty if we want to keep the return value unchanged, as currently <webview>.loadURL returns a serialized version of Promise object. So I kept the backport as it is and it requires release-wg to approve.

@jkleinsc jkleinsc merged commit faa2710 into master Jul 10, 2019
14 checks passed
14 checks passed
Artifact Comparison Changes Detected
Details
Backportable? - 6-0-x Cancelled
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190710.6 succeeded
Details
electron-arm64-testing Build #20190710.6 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jul 10, 2019

Release Notes Persisted

Fix setting src on <webview> being too slow.

@jkleinsc jkleinsc deleted the ipc-handle-promise branch Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.