1227008 - GtkPaperSize leak in nsPrintSettingsGTK::SetGtkPrintSettings()

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

https://dxr.mozilla.org/mozilla-central/source/widget/gtk/nsPrintSettingsGTK.cpp#186
https://git.gnome.org/browse/gtk+/tree/gtk/gtkprintsettings.c?h=3.18.5#n643

Comment 1

[Karl Tomlinson (:karlt)]

https://dxr.mozilla.org/mozilla-central/rev/abbd213422a560f1180c4ec6e3bf4792c2ea81ba/widget/gtk/nsPrintSettingsGTK.cpp#186

Comment 2

[Karl Tomlinson (:karlt)]

The GtkPageSetup already has a GtkPaperSize that we can manipulate directly.
The GtkPrintSettings has a serialization of the GtkPaperSize.
gtk_page_setup_get_paper_size() returns a pointer to the object in the
GtkPageSetup, but gtk_print_settings_get_paper_size() returns a new
GtkPaperSize.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1227008 use GtkPaperSize in GtkPageSetup directly, instead of maintaining a separate copy r?acomminos

bug 1227008 use GtkPaperSize in GtkPageSetup directly, instead of maintaining a separate copy r?acomminos

Fixes a leak in nsPrintSettingsGTK::SetGtkPrintSettings() and
missed paper size copy in operator= (bug 965991).

GtkPageSetup always has a GtkPaperSize [1], which is returned from
gtk_page_setup_get_paper_size() as indicated in the docs [2]:
"Return value: the paper size" without "or NULL".

[1] https://git.gnome.org/browse/gtk+/tree/gtk/gtkpagesetup.c?h=2.18.0#n68
[2] https://git.gnome.org/browse/gtk+/tree/gtk/gtkpagesetup.c?h=2.18.0#n164

Comment 4

[Andrew Comminos [:acomminos]]

Comment on attachment 8691193 [details]
MozReview Request: bug 1227008 use GtkPaperSize in GtkPageSetup directly, instead of maintaining a separate copy r?acomminos

https://reviewboard.mozilla.org/r/26105/#review23463

Looks good to me, thanks!

::: widget/gtk/nsPrintSettingsGTK.h:138
(Diff revision 1)
>     * - mPrintSettings, mPageSetup and mPaperSize are just new objects with defaults determined by GTK.

Comment referring to mPaperSize can be removed, no big deal though.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45a665ae8dc7

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Andrew Comminos [:acomminos] from comment #4)
> Comment referring to mPaperSize can be removed

Done, thanks.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45a665ae8dc7