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 race condition with registering webview postMessage calls over RPC #1406

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lyonsil
Copy link
Member

@lyonsil lyonsil commented Dec 18, 2024

The problem here is that when a window is moved within the docking framework, the webview component is unmounted and then remounted in a very short period of time. When the unmount happens, an RPC method is unregistered. When the mount happens again, the RPC method is registered again. However, when unmount and mount run asynchronously so closely to each other, the unregister call fires, and before it returns from the main process, the register call fails since the RPC client sees that the method is currently (still) registered. Then the unregister call from main returns, and the RPC client removes the method. That means when you moved the window, you lost the RPC postMessage endpoint. What gets confusing, then, is that when you move the window again, the unmount tries to unregister (which fails but otherwise does nothing), and the mount tries to register (which succeeds this time since there was nothing currently registered). So every other time you move the window, you have a functioning postMessage RPC method. The other half of the time, you don't have a functioning method.

Fixing this involves sequencing the registration calls per webview to the RPC client. That way, the call to register doesn't occur until the call to unregister completes. No more race conditions.

This was made a little more complicated by the fact that when React unmounts a component, all values inside the component are destroyed. That means keeping track of the pending, chained promises cannot be done with the React component itself. It could be done, I think, via a Context object, but that's a little messier than I wanted. Since the component is created by a function inside of a module, I'm using a module-level map to store all the promise chains.

In case we run into this scenario somewhere else in the future, I created a class in our utils lib specifically for the special promise chain map. This might end up being the only place it gets used, but it seemed cleaner to separate out the functionality into its own class.

This change is Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant