fix: maintain a ref count for objects sent over remote #17464
Conversation
Previously when this assertion failed all tests that ran after the failed assertion also failed. This ensure that the assertion fails for the test that actually caused the issue but cleans up the left-over windows so that future tests do not fail.
367088b to
54d20d1
Compare
54d20d1 to
89f9de3
Compare
|
This would trigger another regression. Consider following situation:
See #4733 (comment) for more history on the original regression, and #4869 for the fix. I think this PR fixes the CI failure because it deferred the |
89f9de3 to
f737634
Compare
|
@zcbenz I have updated the PR with a (IMO) better approach to ensuring that we only ever remove an object from the cache if all sent references have been dereffed. |
|
This is the exact race condition we were hitting. In a rare case when a remote object was fetched a second time and a GC occured at the right time (between the main process sending the remote object metadata and the renderer process pulling the proxy out of a weak map) the renderer would tell the main process that the object had been GC'ed but would then on the next tick receive a remote message telling it to use the object it just free'ed. I've included the commit message below for more info:
|
f737634 to
cb9aeb9
Compare
|
Note to self: test failures are because I didn't update https://github.com/electron/electron/blob/master/lib/browser/objects-registry.js#L69 Secondary note to self: typescript would have caught this 😆 |
Previously there was a race condition where a GC could occur in the renderer process between the main process sending a meta.id and the renderer pulling the proxy out its weakmap to stop it being GC'ed. This fixes that race condition by maintaining a "sent" ref count in the object registry and a "received" ref count in the object cache on the renderer side. The deref request now sends the number of refs the renderer thinks it owns, if the number does not match the value in the object registry it is assumed that there is an IPC message containing a new reference in flight and this race condition was hit. The browser side ref count is then reduced and we wait for the new deref message. This guaruntees that an object will only be removed from the registry if every reference we sent has been guarunteed to be unreffed.
6347a68 to
3e0f213
Compare
|
Release Notes Persisted
|
|
I was unable to backport this PR to "5-0-x" cleanly; |
* spec: clean up after a failed window count assertion Previously when this assertion failed all tests that ran after the failed assertion also failed. This ensure that the assertion fails for the test that actually caused the issue but cleans up the left-over windows so that future tests do not fail. * fix: maintain a ref count for objects sent over remote Previously there was a race condition where a GC could occur in the renderer process between the main process sending a meta.id and the renderer pulling the proxy out its weakmap to stop it being GC'ed. This fixes that race condition by maintaining a "sent" ref count in the object registry and a "received" ref count in the object cache on the renderer side. The deref request now sends the number of refs the renderer thinks it owns, if the number does not match the value in the object registry it is assumed that there is an IPC message containing a new reference in flight and this race condition was hit. The browser side ref count is then reduced and we wait for the new deref message. This guaruntees that an object will only be removed from the registry if every reference we sent has been guarunteed to be unreffed.
|
A maintainer has manually backported this PR to "5-0-x", please check out #17825 |
1 similar comment
|
A maintainer has manually backported this PR to "5-0-x", please check out #17825 |
* spec: clean up after a failed window count assertion Previously when this assertion failed all tests that ran after the failed assertion also failed. This ensure that the assertion fails for the test that actually caused the issue but cleans up the left-over windows so that future tests do not fail. * fix: maintain a ref count for objects sent over remote Previously there was a race condition where a GC could occur in the renderer process between the main process sending a meta.id and the renderer pulling the proxy out its weakmap to stop it being GC'ed. This fixes that race condition by maintaining a "sent" ref count in the object registry and a "received" ref count in the object cache on the renderer side. The deref request now sends the number of refs the renderer thinks it owns, if the number does not match the value in the object registry it is assumed that there is an IPC message containing a new reference in flight and this race condition was hit. The browser side ref count is then reduced and we wait for the new deref message. This guaruntees that an object will only be removed from the registry if every reference we sent has been guarunteed to be unreffed.
* spec: clean up after a failed window count assertion Previously when this assertion failed all tests that ran after the failed assertion also failed. This ensure that the assertion fails for the test that actually caused the issue but cleans up the left-over windows so that future tests do not fail. * fix: maintain a ref count for objects sent over remote Previously there was a race condition where a GC could occur in the renderer process between the main process sending a meta.id and the renderer pulling the proxy out its weakmap to stop it being GC'ed. This fixes that race condition by maintaining a "sent" ref count in the object registry and a "received" ref count in the object cache on the renderer side. The deref request now sends the number of refs the renderer thinks it owns, if the number does not match the value in the object registry it is assumed that there is an IPC message containing a new reference in flight and this race condition was hit. The browser side ref count is then reduced and we wait for the new deref message. This guaruntees that an object will only be removed from the registry if every reference we sent has been guarunteed to be unreffed.
Remote Fix
See #17464 (comment) for explanation
Fixes #17039
Refs: https://chromium-review.googlesource.com/c/chromium/src/+/1514516
This also includes the test change as that is the "repro" for this issue 😕
Testing Changes
Previously when this assertion failed all tests that ran after the failed assertion also failed. This ensure that the assertion fails for the test that actually caused the issue but cleans up the left-over windows so that future tests do not fail.
Release Notes
Notes: Fixed race condition where the
remotemodule would sometimes fail to fetch properties of a remote object