Closed Bug 1175171 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Widget: Gtk, defect)

Unspecified
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: acomminos, Assigned: acomminos)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
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)
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)
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-
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+
Removed type checking GObject casts.
Attachment #8624754 - Attachment is obsolete: true
Attachment #8626635 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4eaa77aeece1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.