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

FrameClock: fix segmentation fault after getting `FrameTimings` #246

Merged
merged 1 commit into from Sep 16, 2018

Conversation

Projects
None yet
4 participants
@fengalin
Copy link
Contributor

fengalin commented Sep 16, 2018

Use case and symptoms

In the draw callback of a DrawingArea, I want to retrieve the predicted presentation time for the frame being prepared:

[...]
    let presentation_time = match da.get_frame_clock().unwrap().get_current_timings() {
        Some(frame_timings) => {
            let time = frame_timings.get_predicted_presentation_time() as u64;
            // std::mem::forget(frame_timings); // Uncomment to avoid the segmentation fault
            time
        }
        None => {
            debug!("can't get frame timings");
            return Inhibit(false);
        }
    };
[...]

With the above code, I get a couple of frames ok, and then a segmentation fault with a critical GDK message:

(.:2995): Gdk-CRITICAL **: 12:01:03.723: gdk_frame_timings_unref: assertion 'timings->ref_count > 0' failed

Thread 1 "media-toc" received signal SIGSEGV, Segmentation fault.

Uncommenting std::mem::forget(frame_timings) prevents the segmentation fault from happening.

Analysis

gdk_frame_clock_get_current_timings makes use of gdk_frame_clock_get_timings, which returns a pointer directly from the priv->timings array, without increasing the reference counter on the returned GdkFrameTimings.

Since the documentation doesn't explicitly sates that the user should unref the returned pointer after use, I guess this is intentional.

My understanding is that in the Rust GDK binding, FrameTimings is treated as a reference counted object, which it is, but which causes Rust to decrement the reference counter when the Rust object is dropped, which deallocates the C object in the priv->timings array, causing the segmentation fault when the C code attempts to access it later.

Proposal

I'm still far from being fluent with GLib and its binding. My best proposal so far is to borrow the returned object. The C code seems to be the same in current master. Should it change, we could add a feature gate based on the targetted GDK (GTK) version.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 16, 2018

I agree with your analysis and your fix. So unless anyone else has a better proposal, I think we'll go for this one. Thanks!

cc @sdroege @EPashkin

#[cfg(any(feature = "v3_8", feature = "dox"))]
pub fn get_current_timings(&self) -> Option<FrameTimings> {
unsafe {
from_glib_borrow(ffi::gdk_frame_clock_get_current_timings(self.to_glib_none().0))

This comment has been minimized.

@sdroege

sdroege Sep 16, 2018

Member

This is wrong. You must never ever return a from_glib_borrow() return value to safe code. It's borrowed but without making sure the owner is kept alive.

This needs to be from_glib_none(). Which is also what the C documentation seems to suggest, not sure why the .gir file says transfer full.

This comment has been minimized.

@EPashkin

EPashkin Sep 16, 2018

Member

If from_glib_none works then change better be done in https://github.com/gtk-rs/gir-files with fix.sh instead

This comment has been minimized.

@EPashkin

EPashkin Sep 16, 2018

Member

With something like

# fix wrong "full" transfer ownership
xmlstarlet ed -P -L \
	-u '//_:method[@c:identifier="gdk_frame_clock_get_current_timings"]/_:return-value/@transfer-ownership' -v "none" \
	-u '//_:method[@c:identifier="gdk_frame_clock_get_timings"]/_:return-value/@transfer-ownership' -v "none" \
	Gdk-3.0.gir

This comment has been minimized.

@fengalin

fengalin Sep 16, 2018

Author Contributor

@EPashkin, it does work with from_glib_none. Applying your suggestion right now. Thanks

FrameClock: fix segmentation fault after getting `FrameTimings`
`gdk_frame_clock_get_current_timings` & `gdk_frame_clock_get_timings`
don't `ref` the returned `GdkFrameTimings`, causing a segmentation
fault when the Rust object is dropped (and `unref`ed).

@fengalin fengalin force-pushed the fengalin:ref_frame_timings branch from 0369bc4 to 3f5d523 Sep 16, 2018

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Sep 16, 2018

@fengalin Thanks again.
👍

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Sep 16, 2018

All green :)

@GuillaumeGomez GuillaumeGomez merged commit d964fdc into gtk-rs:master Sep 16, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fengalin fengalin deleted the fengalin:ref_frame_timings branch Sep 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.