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

Keep track of remote object reference count. #9389

Merged
merged 2 commits into from May 17, 2017

Conversation

Projects
None yet
3 participants
@tarruda
Contributor

tarruda commented May 6, 2017

This is required to ensure page reloads are handled properly.

Close #9387

@@ -28,11 +28,9 @@ class ObjectsRegistry {
owner = this.owners[webContentsId] = new Set()
this.registerDeleteListener(webContents, webContentsId)
}
if (!owner.has(id)) {

This comment has been minimized.

@zcbenz

zcbenz May 11, 2017

Contributor

Both remote and ObjectsRegistry assume that one webContents can only hold one reference to an object in the main process, so when remote requests the same object for multiple times in one renderer process, the reference count to the object only adds one. The if (!owner.has(id)) condition is to guard this assumption.

If we change this, the object would be leaked in the main process when a webContents requested the same remote object for multiple times.

Regarding the issue #9387:

initial load
hello.js was added to objects-registry and referenced count increased
reload
hello.js was added to objects-registry(no reference count increased)

The root cause is: after page was reloaded all remote objects referenced by the reloaded webContents should be cleared, but for some reason it is not true for sandboxed webContents.

The clearance is done by listening to the render-view-deleted event of webContents in the registerDeleteListener method, the bug is probably caused by the render-view-deleted event not emitting when reloading sandboxed webContents.

This comment has been minimized.

@tarruda

tarruda May 11, 2017

Contributor

Thanks for the explanation, I will fix the PR to ensure render-view-deleted is emitted/handled properly in sandbox

@tarruda

This comment has been minimized.

Contributor

tarruda commented May 11, 2017

@zcbenz it turns out render-view-deleted was never called because sandbox reuses the same process for reload, so the fix was to use the same ChildWebContentsTracker trick used by nativeWindowOpen to force a process respawn on reload of the top-level window.

This creates another question though: Won't this be a problem for child windows given that they also have dedicated webContents that can be reloaded without creating new processes? If so, maybe we should fix the assumption of one webContents only having one reference in objects-registry.js?

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented May 15, 2017

@tarruda We can probably solve this by sending an sync message to the main process when a web page's V8 context is going to be released, and clear the ObjectsRegistry when received that event instead of listening to render-view-deleted.

maybe we should fix the assumption of one webContents only having one reference in objects-registry.js?

This would result in the same object in main process having different remote objects, e.g. in renderer process remote.require('module') !== remote.require('module').

@tarruda

This comment has been minimized.

Contributor

tarruda commented May 15, 2017

@zcbenz I have fixed according to your specifications, but sending a synchronous message before navigation seems risky due to the possibility of deadlocks(I remember reading somewhere in chrome docs that it is a good practice to avoid relying on synchronous messages, if possible). Also it doesn't seem very robust since if the renderer crashes no message will be sent(assuming "render-view-deleted" is emitted even if the process crashes).

IMHO I think it is cleaner if we simply assume one webcontents can reference the same object multiple times, I don't see how remote.require('module') !== remote.require('module') could happen in this case. If the renderer receives the same id for an object, it would simply fetch the existing object from its remoteObjectCache, no?

As for a solution, I imagine the following: We maintain a global per-object reference count(all references to the object across all webcontents) plus a webcontents-specific per-object reference count. If a webcontents is ever killed of removed before all objects referenced by it are explicitly garbage collected, we simply subtract the webcontents count from the global count. If you are ok with it, I can give a shot at implementing this.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented May 16, 2017

but sending a synchronous message before navigation seems risky due to the possibility of deadlocks

It depends on the direction of the message, sending sync message from renderer process to main process is totally fine, and the remote module has been doing it very heavily.

IMHO I think it is cleaner if we simply assume one webcontents can reference the same object multiple times, I don't see how remote.require('module') !== remote.require('module') could happen in this case. If the renderer receives the same id for an object, it would simply fetch the existing object from its remoteObjectCache, no?

The reference count of remote object in renderer process is only subtracted when it gets garbage collected, currently it works like this:

let a = remote.require('module')
// referenceCount +=1 
let b = remote.require('module')
// the object exists in remoteCache, so referenceCount is not changed
a === b

a = null
gc()
// the object is still referenced by b, so referenceCount is not changed

b = null
gc()
// referenceCount -=1

// referenceCount ==0, object is released.

If we add reference count whenever an object is requested, the there will have leak:

let a = remote.require('module')
// referenceCount +=1 
let b = remote.require('module')
// referenceCount +=1 
a === b

a = null
gc()
// the object is still referenced by b, so referenceCount is not changed

b = null
gc()
// garbage collection happened, referenceCount -=1

// referenceCount == 1, the object is leaked.

Unless we don't cache remote objects and create a proxy whenever an object is requested:

let a = remote.require('module')
// referenceCount +=1 
let b = remote.require('module')
// referenceCount +=1 
a !== b

a = null
gc()
// garbage collection happened, referenceCount -=1

b = null
gc()
// garbage collection happened, referenceCount -=1

// referenceCount ==0, object is released.

But it breaks object equality and consumes much more resources, (operations like remote.getCurrentWindow() would create a huge proxy object every time).

tarruda added some commits May 15, 2017

Fix how rpc-server releases references after page reload
In addition to listening for "render-view-deleted", listen for
"ELECTRON_BROWSER_CONTEXT_RELEASE" synchronous message, which is sent by the
remote module when the page is about to be navigated.

This is required to allow child windows running in the same renderer to
correctly manage remote object references, since `render-view-deleted` is only
called when the renderer exits.

Close #9387
@tarruda

This comment has been minimized.

Contributor

tarruda commented May 16, 2017

OK @zcbenz I understand the issue now, thanks again for explaining.

I have added the synchronous message to release references but didn't remove the render-view-deleted listener since that will be fired even if the renderer is terminated abnormally. It should be fine since the clear method guards against double release.

@lneir

This comment has been minimized.

lneir commented May 16, 2017

any updates when this will be merged? thanks.

@zcbenz

zcbenz approved these changes May 17, 2017

@zcbenz zcbenz merged commit 6826595 into master May 17, 2017

6 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-mas-x64 Build #4208 succeeded in 10 min
Details
electron-osx-x64 Build #4212 succeeded in 16 min
Details
electron-win-ia32 Build #3184 succeeded in 10 min
Details
electron-win-x64 Build #3157 succeeded in 22 min
Details

@zcbenz zcbenz deleted the fix-remote-exception-sandbox branch May 17, 2017

@sethlu sethlu referenced this pull request Jul 9, 2018

Closed

Missing remote object #8205

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment