Singleton KeymapWrapper never gets deallocated on GTK3 when we don't call gdk_display_close

RESOLVED FIXED in Firefox 41

Status

()

Core
Widget: Gtk
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: acomminos, Assigned: acomminos)

Tracking

unspecified
mozilla41
Unspecified
Linux
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

The workaround in bug 884831 causes the singleton KeymapWrapper to stay alive forever on systems with GTK versions before 3.10, as the backing GdkKeymap is never derefed- causing a memory leak. Normally the KeymapWrapper would be freed when our weak ref handler for GdkKeymap is called.
Created attachment 8623125 [details] [diff] [review]
Bug 1176171 - Deallocate GTK's KeymapWrapper on shutdown.

This manually deallocates the KeymapWrapper in MOZ_gdk_display_close instead of depending on weak ref callbacks. Thanks!

Try push (GTK3): https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c418a37fdf9
Attachment #8623125 - Flags: review?(karlt)
Created attachment 8623141 [details] [diff] [review]
Bug 1176171 - Deallocate GTK's KeymapWrapper on shutdown.

Fixed exports and visibility.

Try (GTK3): https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbfaa6618fb4
Attachment #8623125 - Attachment is obsolete: true
Attachment #8623125 - Flags: review?(karlt)
Attachment #8623141 - Flags: review?(karlt)
Created attachment 8623162 [details] [diff] [review]
Deallocate GTK's KeymapWrapper on shutdown.

Disconnect signal handler in destructor.
Attachment #8623141 - Attachment is obsolete: true
Attachment #8623141 - Flags: review?(karlt)
Attachment #8623162 - Flags: review?(karlt)
Comment on attachment 8623162 [details] [diff] [review]
Deallocate GTK's KeymapWrapper on shutdown.

>@@ -2911,16 +2912,19 @@ static void MOZ_gdk_display_close(GdkDisplay *display)
>   }
> 
> #if (MOZ_WIDGET_GTK == 3)
>   // A workaround for https://bugzilla.gnome.org/show_bug.cgi?id=703257
>   if (gtk_check_version(3,9,8) != NULL)
>     skip_display_close = true;
> #endif
> 
>+  // Manually destroy the singleton KeymapWrapper, see bug 1175171
>+  widget::KeymapWrapper::Destroy();

Can this be called from widget code, such as nsWidgetFactory.cpp.
Best to keep as much platform-specific code out of nsAppRunner as we can.

I think this kind of method is usually called Shutdown() (though the verb
should be two words "ShutDown()").

>+    g_signal_handler_disconnect(mGdkKeymap, mKeysChangedHandler);

Use g_signal_handlers_disconnect_by_func to avoid needing to track the handler
id.

>-KeymapWrapper::OnDestroyKeymap(KeymapWrapper* aKeymapWrapper,
>-                               GdkKeymap *aGdkKeymap)
>-{
>-    MOZ_LOG(gKeymapWrapperLog, LogLevel::Info,
>-        ("KeymapWrapper: OnDestroyKeymap, aGdkKeymap=%p, aKeymapWrapper=%p",
>-         aGdkKeymap, aKeymapWrapper));
>-    MOZ_ASSERT(aKeymapWrapper == sInstance,
>-               "Desroying unexpected instance");
>-    delete sInstance;
>-    sInstance = nullptr;
>-}

If this code is removed, then the validity of the bare pointer mGdkKeymap
depends on the shutdown order.  I'd like to avoid having safety depend on
shutdown order, so please make mGdkKeymap a strong reference if removing this
code.
Attachment #8623162 - Flags: review?(karlt) → review-
Created attachment 8624754 [details] [diff] [review]
Deallocate GTK's KeymapWrapper on shutdown.

Thanks, updated with review changes.
Attachment #8623162 - Attachment is obsolete: true
Attachment #8624754 - Flags: review?(karlt)
Comment on attachment 8624754 [details] [diff] [review]
Deallocate GTK's KeymapWrapper on shutdown.

>+    g_object_ref(G_OBJECT(mGdkKeymap));

>+    g_object_unref(G_OBJECT(mGdkKeymap));

The parameter to g_object_ref is void* so no need for and please remove the 
G_OBJECT() G_TYPE_CHECK_INSTANCE_CAST()s.
Attachment #8624754 - Flags: review?(karlt) → review+
Created attachment 8626635 [details] [diff] [review]
Deallocate GTK's KeymapWrapper on shutdown. Carry r=karlt

Removed type checking GObject casts.
Attachment #8624754 - Attachment is obsolete: true
Attachment #8626635 - Flags: review+
Keywords: checkin-needed

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eaa77aeece1
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4eaa77aeece1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.