-
Notifications
You must be signed in to change notification settings - Fork 23
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
Remove NodeId
usage from ChromiumSenderThread
#356
Conversation
af669d9
to
50d2589
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please verify if this approach works (does not crash) even without validation that only one component is using the renderer.
|
||
pub fn resize(&mut self, size: usize) -> Result<(), SharedMemoryError> { | ||
// Releasing the current `Shmem` instance to ensure it does not erase the shared memory descriptor from the file system | ||
// This is critical to ensure when a new `Shmem` is created at the same location, it doesn't conflict with the old descriptor | ||
drop(self.inner.take()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop(self.inner.take()); | |
self.inner.take(); |
wouldn't this be the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole line is not needed unless I forgot about something.
} | ||
} | ||
|
||
// Create additional shared memory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the number of inputs is smaller, there is no cleanup of already allocated memory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no cleanup since it requires synchronization. It will be added eventually
frame.send_process_message(cef::ProcessId::Renderer, process_message)?; | ||
// ----- | ||
|
||
shmem.resize(size)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if you only resize if shm is too small? Would this avoid potential crashes even without synchronization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would work
I need those changes for the other PR I'm working on and it seems everyone already reviewed it, so I'll merge it. Please take a look at the comments and if there is anything actionable there address that in a follow-up PR. |
Initially, when I implemented the web renderer I thought that it would be possible to have one web renderer that is used by multiple web views. In this PR I removed some of the unnecessary stuff that was added because of the assumptions I made when I was implementing it.
Issue: #352