235643 - all _CreatePrintSettings leak on failure

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[timeless]

66   nsPrintSettingsMac* printSettings = new nsPrintSettingsMac(); // does not
initially ref count
 67   if (!printSettings)
 68     return NS_ERROR_OUT_OF_MEMORY;
 69   rv = printSettings->Init();
 70   if (NS_FAILED(rv))
 71     return rv;

printSettings is leaked when init fails. similar things happen in the other impls

Comment 1

[Bastiaan Jacques]

Attachment: clean up on fail

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 185850 [details] [diff] [review]
clean up on fail

This code seems overly complicated.  Why not just assign printSettings to
*_retval and then addref if it's not null?

In other words, I don't think this QI ever fails.

On a separate note, just 3 more lines of context would have made this patch
much much more readable.  I suggest using -8 at least for diffs unless you have
good reason to use less context.

Comment 3

[Bastiaan Jacques]

Attachment: remove QI, use addref

Like so?

I've used more effective context for this patch. I'll keep in mind for future
ones, as well.

Comment 4

[timeless]

Comment on attachment 185855 [details] [diff] [review]
remove QI, use addref

>   nsPrintSettings* printSettings = new nsPrintSettings(); // does not initially ref count
>+  *_retval = printSettings;
>+  if (!*_retval) return NS_ERROR_FAILURE;
NS_ERROR_OUT_OF_MEMORY, and personally i tend to check local variables.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I think the clearest and fastest code pattern here would be:

nsPrintSettings * printSettings = ...;
NS_ENSURE_TRUE(printSettings, NS_ERROR_OUT_OF_MEMORY);
NS_ADDREF(*_retval = printSettings);

Comment 6

[Bastiaan Jacques]

Attachment: address comments

Comment 7

[Bastiaan Jacques]

Attachment: same, for Mac and Windows

This patches the remaining two _CreatePrintSettings() implementations.

Comment 8

[Bastiaan Jacques]

Attachment: remove cvs noise

The right patch this time.

Comment 9

[timeless]

Comment on attachment 185858 [details] [diff] [review]
address comments

mozilla/gfx/src/nsPrintOptionsImpl.cpp	1.73

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 185902 [details] [diff] [review]
remove cvs noise

Generally, calling Init() on an object with a refcount of 0 is dangerous... I'd
suggest doing the NS_ADDREF before calling Init(), with an NS_RELEASE if Init()
fails.

Comment 11

[Bastiaan Jacques]

Attachment: address comments

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 186020 [details] [diff] [review]
address comments

Make that an NS_RELEASE(*_retval) so you're not setting the out param to a
known-bogus value, and sr=bzbarsky

Comment 13

[Bastiaan Jacques]

Attachment: patch for checkin

Comment 14

[Bastiaan Jacques]

Attachment: patch for checkin

removed some unintended whitespace messing about.

Comment 15

[Bastiaan Jacques]

Attachment: final patch

Both reviewers suggested that it is preferred to null out *_retval in case of
failure; thus, here is the hopefully final version of the patch which does just
that. sr=bzbarsky stands (which he conceded via email), assuming timeless
agrees.

Comment 16

[timeless]

Comment on attachment 186051 [details] [diff] [review]
final patch

r=me, marking sr=bz

Comment 17

[timeless]

Comment on attachment 186051 [details] [diff] [review]
final patch

mozilla/gfx/src/mac/nsPrintOptionsX.cpp 	1.15
mozilla/gfx/src/windows/nsPrintOptionsWin.cpp	1.10