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

Fix FromGlibContainer double freeing #160

Merged
merged 1 commit into from May 3, 2017

Conversation

Projects
None yet
4 participants
@EPashkin
Member

EPashkin commented Apr 29, 2017

Revert one change from #153
Was wrong replacing g_free with g_strfreev by two reason:

  1. g_strfreev frees inner string that already freed
  2. g_strfreev must used only for string arrays, but this implementation for any arrays
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 29, 2017

Member

@Susurrus please check that this don't broke gtk-rs/gtk#469

Member

EPashkin commented Apr 29, 2017

@Susurrus please check that this don't broke gtk-rs/gtk#469

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 2, 2017

Member

Might make sense to add a simple test for this, looks good and correct to me apart from that

Member

sdroege commented May 2, 2017

Might make sense to add a simple test for this, looks good and correct to me apart from that

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 2, 2017

Member

No idea how to test it :(

Member

EPashkin commented May 2, 2017

No idea how to test it :(

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 2, 2017

Member

How did you get the bug?

Member

GuillaumeGomez commented May 2, 2017

How did you get the bug?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin
Member

EPashkin commented May 2, 2017

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 2, 2017

Member

@EPashkin You could manually create such a NULL-terminated array of e.g. strings with glib_sys::g_malloc() and ::g_strdup(). When passing that in there it should crash.

Member

sdroege commented May 2, 2017

@EPashkin You could manually create such a NULL-terminated array of e.g. strings with glib_sys::g_malloc() and ::g_strdup(). When passing that in there it should crash.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 2, 2017

Member

@sdroege, Thanks I don't even think to test it (forgot that it not Gtk and can be tested).

Member

EPashkin commented May 2, 2017

@sdroege, Thanks I don't even think to test it (forgot that it not Gtk and can be tested).

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus May 3, 2017

gtk-rs/gtk#469 is still fixed with this code, but this doesn't seem to fix gtk-rs/examples#119. I'm getting new errors now, like:

  • (.:12663): Gtk-CRITICAL **: GtkBox 0x7f8893b191a0 returned a widget path for type (null), but child is GtkScrolledWindow
  • (.:12663): Gtk-CRITICAL **: gtk_style_provider_get_style_property: assertion 'g_type_is_a (gtk_widget_path_get_object_type (path), pspec->owner_type)' failed
  • (.:12809): GLib-GObject-WARNING **: g_object_weak_unref: couldn't find weak ref 0x7ff1eb45c5e0(0x7ff1ece22ed8)

But I don't get hard segfaults anymore with no warnings or errors, so I guess that's an improvement.

Susurrus commented May 3, 2017

gtk-rs/gtk#469 is still fixed with this code, but this doesn't seem to fix gtk-rs/examples#119. I'm getting new errors now, like:

  • (.:12663): Gtk-CRITICAL **: GtkBox 0x7f8893b191a0 returned a widget path for type (null), but child is GtkScrolledWindow
  • (.:12663): Gtk-CRITICAL **: gtk_style_provider_get_style_property: assertion 'g_type_is_a (gtk_widget_path_get_object_type (path), pspec->owner_type)' failed
  • (.:12809): GLib-GObject-WARNING **: g_object_weak_unref: couldn't find weak ref 0x7ff1eb45c5e0(0x7ff1ece22ed8)

But I don't get hard segfaults anymore with no warnings or errors, so I guess that's an improvement.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 3, 2017

Member

@GuillaumeGomez ready for merge

Member

EPashkin commented May 3, 2017

@GuillaumeGomez ready for merge

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 3, 2017

Member

Thanks!

Member

GuillaumeGomez commented May 3, 2017

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit ad0062a into gtk-rs:master May 3, 2017

2 checks passed

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

@EPashkin EPashkin deleted the EPashkin:from_glib_fix branch May 3, 2017

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