965991 - nsPrintSettingsGTK copy constructor missed initializing paper size

Status: RESOLVED DUPLICATE of bug 1227008

(Core :: Widget: Gtk, defect)

Description

[Alex Vincent [:WeirdAl]]

Attachment: #developers conversation about this bug

Sample code:
const Ci = Components.interfaces;
var wbPrint = getBrowser().contentWindow
                          .QueryInterface(Ci.nsIInterfaceRequestor)
                          .getInterface(Ci.nsIWebBrowserPrint);
var settings = wbPrint.globalPrintSettings.clone();
wbPrint.print(settings, this); // CRASH

(gdb) print mPaperSize
$1 = (GtkPaperSize *) 0xa5a5a5a5a5a5a5a5

Comment 1

[Oskar Eisemuth]

From a quick look the operator= should create copies for all internals/add references for the gtk objects, that should be called by the second constructor, that is called by _Clone.

Comment 2

[Alex Vincent [:WeirdAl]]

If anyone thinks they have the time to develop a fix for this bug, go for it; it'll be some time before I can find the cycles.

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: Copy constructor support for paperName in nsPrintSettingsGTK

Comment 4

[Alex Vincent [:WeirdAl]]

Comment on attachment 8690220 [details] [diff] [review]
Copy constructor support for paperName in nsPrintSettingsGTK

I hope this is right, or close to it.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8690220 [details] [diff] [review]
Copy constructor support for paperName in nsPrintSettingsGTK

>+  {
>+    nsPrintSettingsGTK& other = const_cast<nsPrintSettingsGTK&>(rhs);
>+    char16_t* paperName;
>+    other.GetPaperName(&paperName);
>+    SetPaperName(paperName);
>+    NS_Free(paperName);
>+  }
>+

mPageSetup already has a suitable GtkPaperSize, so all that is needed
here is to gtk_paper_size_free(mPaperSize), and set mPaperSize from
gtk_paper_size_copy(gtk_page_setup_get_paper_size()).

(I'm not sure that the paperName always describes all of the info in GtkPaperSize).

> nsresult nsPrintSettingsGTK::_Clone(nsIPrintSettings **_retval)

>+  char16_t* paperName;
>+  GetPaperName(&paperName);
>+  newSettings->SetPaperName(paperName);
>+  NS_Free(paperName);
>+

The copy constructor (used by Clone()) uses operator=, so no need to do
anything in Clone() because fixing operator= is enough to fix this.

Comment 6

[Karl Tomlinson (:karlt)]

The patch in bug 1227008 fixes this by removing mPaperSize altogether.