Fix image delete on window.location.replace() #185
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a website does JS window.location.replace(), the Webkit tears down the entire PlatformDisplayLibWPE(), which destroy surface and other resources. The destroy may happen before registered .release callbacks of e.g. wl_buffer, which may then attempt to access already free()d Image memory. This happens with COG, where if the website does window.location.replace() then resources are destroyed and src/view-backend-exportable-fdo-egl.cpp bufferDestroyListenerCallback() is called, which frees the Image. However COG may have registered a .release callback in .export_fdo_egl_image() in exportImage, which maps to COG platform/wayland/cog-platform-wl.c on_export_wl_egl_image() -> on_dmabuf_buffer_release() and that may not have been called yet, and may even be called after bufferDestroyListenerCallback() . The situation is worse, since during window.location.replace() the next page is being loaded, and that triggers exportBuffer() of new resources, which does findImage() and looks up Image based on assigned destroy callback bufferDestroyListenerCallback. That lookup may return Image which is just about to be destroyed, i.e. bufferDestroyListenerCallback() is already planned to be called. If that happens, COG wl_buffer .release callback would access Image which was already free()d by the bufferDestroyListenerCallback(). Worse, the image returned by findImage() have incorrect bufferResource associated with it, not the one passed to exportBuffer(), but the old one from the original image . Fix the last part by dropping the entire findImage() mechanism, just allocate new Image for each exportBuffer() call, this happens seldom enough to not pose any significant overhead. Fix the first part by assuring the release and destroy order is never reversed, i.e. if destroy is called first, do not free() the Image, wait for releaseImage() to be called and release the image there. If releaseImage() was called, let the bufferDestroyListenerCallback() destroy and free() the Image . Signed-off-by: Marek Vasut <marex@denx.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a website does JS window.location.replace(), the Webkit tears down the entire PlatformDisplayLibWPE(), which destroy surface and other resources. The destroy may happen before registered .release callbacks of e.g. wl_buffer, which may then attempt to access already free()d Image memory.
This happens with COG, where if the website does window.location.replace() then resources are destroyed and src/view-backend-exportable-fdo-egl.cpp bufferDestroyListenerCallback() is called, which frees the Image. However COG may have registered a .release callback in .export_fdo_egl_image() in exportImage, which maps to COG platform/wayland/cog-platform-wl.c on_export_wl_egl_image() -> on_dmabuf_buffer_release() and that may not have been called yet, and may even be called after bufferDestroyListenerCallback() .
The situation is worse, since during window.location.replace() the next page is being loaded, and that triggers exportBuffer() of new resources, which does findImage() and looks up Image based on assigned destroy callback bufferDestroyListenerCallback. That lookup may return Image which is just about to be destroyed, i.e. bufferDestroyListenerCallback() is already planned to be called. If that happens, COG wl_buffer .release callback would access Image which was already free()d by the bufferDestroyListenerCallback(). Worse, the image returned by findImage() have incorrect bufferResource associated with it, not the one passed to exportBuffer(), but the old one from the original image .
Fix the last part by dropping the entire findImage() mechanism, just allocate new Image for each exportBuffer() call, this happens seldom enough to not pose any significant overhead.
Fix the first part by assuring the release and destroy order is never reversed, i.e. if destroy is called first, do not free() the Image, wait for releaseImage() to be called and release the image there. If releaseImage() was called, let the bufferDestroyListenerCallback() destroy and free() the Image .