297277 - potential OOM crash/mlk [@ nsPrintOptions::_CreatePrintSettings]

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[timeless]

this is an audit of sorts, it's based on a dreftool audit

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/nsPrintOptionsImpl.cpp&rev=1.67&mark=265#260
  Deref-error: "sDefaultFont"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/nsPrintOptionsImpl.cpp&rev=1.67&mark=887#882
  Deref-error: "printSettings"

and this is really a followup to bug 235643 ...

bonus points for replacing SetLength(0) with Truncate()
partial credit for fixing indentation to not have 3 space (code seems to be a
mix of 1space, 2space, 3space, 4space, plus combinations)

Comment 1

[Bastiaan Jacques]

>  sDefaultFont = new nsFont(aFont);

> aFont = *sDefaultFont;

I don't see how these are dereferencing errors. Could you elaborate?

Comment 2

[timeless]

nothing makes sure the new doesn't fail. new is an alloc and can fail, presume
we don't throw (because if we throw, the world ends, we try to ask vc6 and gcc
not to throw [we incorrectly encourage vc7 to throw, but that's very much our
fault and a regression]).

Comment 3

[Bastiaan Jacques]

Attachment: cleanup patch (diff -u)

This patch fixes the dereferencing errors, and is a general cleanup of
nsPrintOptionsImpl.*. It does in particular:
  * use the new string classes;
  * check for pointers/null correctly;
  * fix whitespacing and indentation;
  * more appropriate code patterns, return from errors immediately;
  * use macros for repetitive code;
  * null out _retvals;
  * ensure that parameter names (at least vaguely) reflect their purpose;
  * replaces SetLength(0) with Truncate().

Comment 4

[Bastiaan Jacques]

Attachment: cleanup patch (diff -w)

Comment 5

[Bastiaan Jacques]

Comment on attachment 186174 [details] [diff] [review]
cleanup patch (diff -u)

I did a call for volunteers for reviewing this patch (since printing doesn't
seem covered by any module owners/peers in particular), but there were none.
So, I'm setting asking the same reviewers as I did for the last printing patch.
I Hope that's okay.

Comment 6

[timeless]

Comment on attachment 186174 [details] [diff] [review]
cleanup patch (diff -u)

i gave feedback to baafie on irc.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 186174 [details] [diff] [review]
cleanup patch (diff -u)

+      NS_ERROR("Operator new() returned nsnull; out of memory?");

(maybe timeless already mentioned this, but) you can't use NS_ERROR for OOM.
it's an error you need to handle, you can't assert allocs succeed.

Comment 8

[Bastiaan Jacques]

Attachment: cleanup patch, revision 1 (diff -u)

Summary of timeless' comments:

<timeless> if [you have something in the constructor that] you need to init
something that can fail, it really should be moved to an nsresult Init(){}
method

<timeless>  NS_ENSURE_ARG_POINTER(sDefaultFont); ->
NS_ENSURE_STATE(sDefaultFont);

<timeless> NS_ENSURE_ARG_POINTER(ioParamBlock); -> NS_ENSURE_TRUE(.., OOM)

<timeless> [whitespace alignment]

<timeless> +  NS_ADDREF(*aPrinterEnumerator =
NS_STATIC_CAST(nsISimpleEnumerator*, printerListEnum));
<timeless> is that cast needed?

<timeless>  NS_IMETHODIMP nsPrintOptions::CreatePrintSettings(nsIPrintSettings
**_retval)
<timeless>  {
<timeless> +  NS_ENSURE_ARG_POINTER(_retval);
<timeless> i think we're not ensuring pointers for outs


<timeless> NS_ENSURE_TRUE -> NS_ENSURE_SUCCESS in some places

<timeless> sprintf(str, "%6.2f", aVal); -> snprintf

All of timeless's comments were addressed; most notably I've added an Init()
method, and made its existence known amongst class factories that inherent the
class, as well as the other consumers.

Comment 9

[Bastiaan Jacques]

Attachment: cleanup patch, revision 1 (diff -w)

Comment 10

[Bastiaan Jacques]

Attachment: cleanup patch, revision 1.1 (diff -u)

Let's do it right this time..

Comment 11

[Bastiaan Jacques]

Attachment: cleanup patch, revision 1.1 (diff -w)

Comment 12

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2 (diff -u)

This patch addresses further comments from timeless, most notably the
following:

* sDefaultFont is no longer declared static, so permanent storage is no longer
required for it. However, the caller of GetDefaultFont() deletes the retrieved
font after using it, without setting the pointer to nsnull. Then, our class
would attempt to delete it a second time upon destruction, which would cause a
crash. The patch therefore includes a small change to
layout/generic/nsSimplePageSequence.cpp, which makes the caller nulls out the
pointer to our default font. Arguably the caller should not be deleting it at
all, but that's a subject for another bug.
* My usage of NS_ENSURE_ARG was replaced by NS_ENSURE_ARG_POINTER, because it
is more appropriate.
* Init() was declared virtual.
* ShowPrintSetupDialog() now has better error checking, an unused nsCOMPtr and
an unneeded assertion were removed.

Comment 13

[Bastiaan Jacques]

Attachment: cleanu; patch, revision 2 (diff -w)

Comment 14

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2.1 (diff -u)

This patch adds trivial changes such as slightly better error handling and some
more whitespace cleanups.

Comment 15

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2.1 (diff -w)

Comment 16

[timeless]

Comment on attachment 186867 [details] [diff] [review]
cleanup patch, revision 2.1 (diff -w)

>Index: gfx/src/nsPrintOptionsImpl.cpp
>@@ -340,854 +335,931 @@ const char* nsPrintOptions::GetPrefName(
>  *  This will read in the generic prefs (not specific to a printer)
>  *  or it will it can read the prefs in using the printer name to qualify
"or it will it can" ?!

>+nsPrintOptions::AvailablePrinters(nsISimpleEnumerator **aPrinterEnumerator)
>+  nsCOMPtr<nsPrinterListEnumerator> printerListEnum =
>+      new nsPrinterListEnumerator();
nsCOMPtr was not designed for non interface creatures, i believe you want
nsRefPtr.

>+nsPrintOptions::WriteInchesFromTwipsPref(const char * aPrefId, nscoord aTwips)
> {
>   nsAutoString inchesStr;
>   char * str = ToNewCString(inchesStr);
>   if (str) {
>     mPrefBranch->SetCharPref(aPrefId, str);
doesn't this leak str? :)

>Index: gfx/src/nsPrintOptionsImpl.h
>+class NS_GFX nsPrintOptions : public nsIPrintOptions, public
>+                              nsIPrintSettingsService
>+   * method Init
>+   *  Initializes member variables. Every consumer that does manual
>+   *  addref'ing needs to call this method!
-indicate when and how often-
i think you mean manual creation instead of do_CreateInstance.
>+   * @return nsresult error code
normally we'd list out params as @returns
>+  virtual nsresult Init();
>+   * method WritePrefString
>+   *    Writes PRUnichar* to Prefs and deletes the contents of aString
>+  nsresult WritePrefString(const char * aPrefId, const nsAString& aString);

Comment 17

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2.2 (diff -u)

Address comments.

Comment 18

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2.2 (diff -w)

Comment 19

[rbs]

Comment on attachment 186913 [details] [diff] [review]
cleanup patch, revision 2.2 (diff -u)

I am not interested in super-reviewing this, I don't see much value in such an
invasive cleanup. It messes the bonsai records for little gain.

Comment 20

[Bastiaan Jacques]

Comment on attachment 186913 [details] [diff] [review]
cleanup patch, revision 2.2 (diff -u)

I'll ask bz, in that case.. I <think> he would be willing to do it when he has
time.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 186914 [details] [diff] [review]
cleanup patch, revision 2.2 (diff -w)

   nsAutoString inchesStr;
   inchesStr.AppendFloat(inches);
+  mPrefBranch->SetCharPref(aPrefId, NS_ConvertUTF16toUTF8(inchesStr).get());

OK, why does this use an nsAutoString instead of an nsCAutoString at all?

 nsSharedPageData::~nsSharedPageData()
+    mHeadFootFont = nsnull;

why bother? (note that the other Free calls in this function do not set the
pointer to null)

Comment 22

[Bastiaan Jacques]

(In reply to comment #21)
> OK, why does this use an nsAutoString instead of an nsCAutoString at all?

No particular reason. (I'll change it to nsCAutoString)

>  nsSharedPageData::~nsSharedPageData()
> +    mHeadFootFont = nsnull;
> 
> why bother? (note that the other Free calls in this function do not set the
> pointer to null)

See comment #12.

Comment 23

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2.3 (diff -w)

In this revision, I've addressed biesi's comment about ns[C]AutoString.

In addition, I've removed my change to nsSimplePageSequence.cpp, seeing as
leaving it out doesn't crash my build anymore. The change probably had no
effect anyway, because |delete| is null safe..
At this point I don't understand why the crash was happening in the first
place, but since I can't reproduce it anymore, I'm letting go of this hunk.

Comment 24

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2.3 (diff -u)

Comment 25

[Boris Zbarsky [:bzbarsky]]

It'll take me some time to get to this -- I'm still swamped with things that
collected over the last month.  You may want to look for someone else to
superreview...

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 188243 [details] [diff] [review]
cleanup patch, revision 2.3 (diff -u)

In general it looks good; most of my comments are nit-picking, so addressing
them should be straightforward.

>Index: gfx/idl/nsIPrintOptions.idl
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/idl/nsIPrintOptions.idl,v
>retrieving revision 1.37
>diff -p -8 -u -r1.37 nsIPrintOptions.idl
>--- gfx/idl/nsIPrintOptions.idl	17 Apr 2004 21:52:27 -0000	1.37
>+++ gfx/idl/nsIPrintOptions.idl	4 Jul 2005 12:07:31 -0000
>@@ -87,31 +87,32 @@ interface nsIPrintOptions : nsISupports
>
>-  [noscript] void SetFontNamePointSize(in nsNativeStringRef aName, in PRInt32 aPointSize);
>+  [noscript] void SetFontNamePointSize(in AString aName, in PRInt32 aPointSize);

Since the native reference is gone, how about dropping the [noscript]?

> 
>-[scriptable, uuid(a6cf9128-15b3-11d2-932e-00805f8add32)]
>+[scriptable, uuid(d9948a4d-f49c-4456-938a-acda2c8d7741)]
>+

I'm confused.  Why bump the UUID on the interface after the one that has
actually changed?

>Index: gfx/src/nsPrintOptionsImpl.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/nsPrintOptionsImpl.cpp,v
>retrieving revision 1.73
>diff -p -8 -u -r1.73 nsPrintOptionsImpl.cpp
>--- gfx/src/nsPrintOptionsImpl.cpp	10 Jun 2005 22:18:16 -0000	1.73
>+++ gfx/src/nsPrintOptionsImpl.cpp	4 Jul 2005 12:07:32 -0000
> // Additional Prefs
>-static const char kPrintPaperSize[]     = "print_paper_size"; // this has been deprecated
>+//    this has been deprecated
>+static const char kPrintPaperSize[]     = "print_paper_size";

This makes it look a bit like the comment could apply to everything below it. 
Maybe just change the comment to "// deprecated" at the end of the same line?

> nsPrintOptions::~nsPrintOptions()
> {
>-  if (sDefaultFont != nsnull) {
>-    delete sDefaultFont;
>-    sDefaultFont = nsnull;
>+  if (mDefaultFont) {
>+    delete mDefaultFont;
>+    mDefaultFont = nsnull;
>   }
> }

What's the point of setting mDefaultFont to nsnull after deleting it?  You're
in the destructor, so mDefaultFont is about to go away completely.

>>+NS_IMETHODIMP
> nsPrintOptions::ShowPrintSetupDialog(nsIPrintSettings *aPS)
>+{
>+  NS_ENSURE_ARG_POINTER(aPS);
>+  nsresult rv;
> 
>- return rv;
>-  
>+  // create a nsISupportsArray of the parameters
>+  // being passed to the window
>+  nsCOMPtr<nsISupportsArray> array;
>+  rv = NS_NewISupportsArray(getter_AddRefs(array));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsISupports> psSupports = do_QueryInterface(aPS);
>+  NS_ASSERTION(psSupports, "PrintSettings must be a supports");
>+
>+  if (psSupports)
>+    array->AppendElement(psSupports);

What's the point of putting this inside an if?	There's already an assertion
checking to make sure that you can QI to nsISupports, and if you can't that's a
bug that needs to be fixed, not something that should be checked for at
runtime.  

>+
>+  nsCOMPtr<nsIDialogParamBlock> ioParamBlock =
>+      do_CreateInstance(NS_DIALOGPARAMBLOCK_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  ioParamBlock->SetInt(0, 0);
>+
>+  nsCOMPtr<nsISupports> blkSupps = do_QueryInterface(ioParamBlock);
>+  NS_ASSERTION(blkSupps, "IOBlk must be a supports");
>+  if (blkSupps)
>+    array->AppendElement(blkSupps);

Same question.

>+NS_IMETHODIMP
>+nsPrintOptions::AvailablePrinters(nsISimpleEnumerator **aPrinterEnumerator)
> {
>-   NS_ENSURE_ARG_POINTER(aPrinterEnumerator);
>-   *aPrinterEnumerator = nsnull;
>+  *aPrinterEnumerator = nsnull;
> 
>-   nsCOMPtr<nsPrinterListEnumerator> printerListEnum = new nsPrinterListEnumerator();
>-   NS_ENSURE_TRUE(printerListEnum.get(), NS_ERROR_OUT_OF_MEMORY);
>+  nsRefPtr<nsPrinterListEnumerator> printerListEnum =
>+      new nsPrinterListEnumerator();
>+  NS_ENSURE_TRUE(printerListEnum, NS_ERROR_OUT_OF_MEMORY);
> 
>-   nsresult rv = printerListEnum->Init();
>-   if (NS_SUCCEEDED(rv)) {
>-     *aPrinterEnumerator = NS_STATIC_CAST(nsISimpleEnumerator*, printerListEnum);
>-     NS_ADDREF(*aPrinterEnumerator);
>-   }
>-   return rv;
>+  NS_ADDREF(*aPrinterEnumerator = printerListEnum);
>+
>+  nsresult rv = printerListEnum->Init();
>+  if (NS_FAILED(rv))
>+    NS_RELEASE(*aPrinterEnumerator);
>+
>+  return rv;

Instead of calling Init() after the AddRef, call it before.  Furthermore, you
can avoid unnecessary pointer dereferences by doing the assignment to the out
param at the very end using the form:

NS_ADDREF(*aPrinterEnumerator = printerListEnum.get())

> 
>-NS_IMETHODIMP nsPrintOptions::DisplayJobProperties( const PRUnichar *aPrinter, nsIPrintSettings* aPrintSettings, PRBool *aDisplayed)
>+NS_IMETHODIMP
>+nsPrintOptions::DisplayJobProperties(const PRUnichar *aPrinter,
>+                                     nsIPrintSettings* aPrintSettings,
>+                                     PRBool *aDisplayed)
> {
>-   NS_ENSURE_ARG(aPrinter);
>-   *aDisplayed = PR_FALSE;
>+  NS_ENSURE_ARG_POINTER(aPrinter);
>+  *aDisplayed = PR_FALSE;
>+
>+  nsresult  rv;
>+  nsCOMPtr<nsIPrinterEnumerator> propDlg;
> 
>-   nsresult  rv;
>-   nsCOMPtr<nsIPrinterEnumerator> propDlg;
>+  propDlg = do_CreateInstance(kCPrinterEnumerator, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
> 
>-   propDlg = do_CreateInstance(kCPrinterEnumerator, &rv);
>-   if (NS_FAILED(rv))
>-     return rv;
>+  NS_ENSURE_ARG_POINTER(aPrintSettings);
>+  rv = propDlg->DisplayPropertiesDlg((PRUnichar*)aPrinter, aPrintSettings);

Prefer C++ style casts to C-style casts, since they express intent more
precisely and thus allow the compiler to catch more bugs at build time.  In
this case, NS_CONST_CAST is what you want.

>-/* [noscript] voidPtr GetNativeData (in short aDataType); */
> NS_IMETHODIMP nsPrintOptions::GetNativeData(PRInt16 aDataType, void * *_retval)
> {
>-  NS_ENSURE_ARG_POINTER(_retval);
>   *_retval = nsnull;
>-
>   return NS_ERROR_NOT_IMPLEMENTED;
> }

Might as well get rid of the |*retval = nsnull| line here; you're already
returning an error, so you don't need to guarantee that *retval is set to
anything in particular.

> nsresult nsPrintOptions::_CreatePrintSettings(nsIPrintSettings **_retval)
> {
>   nsresult rv = NS_OK;
>+  *_retval = nsnull;

The above line shouldn't be necessary; see previous comment.  There seem to be
more instances of this pattern throughout the patch.  None should be necessary
and just waste cycles.

>-  nsPrintSettings * printSettings = new nsPrintSettings(); // does not initially ref count
>+  // does not initially ref count
>+  nsPrintSettings * printSettings = new nsPrintSettings();
>   NS_ENSURE_TRUE(printSettings, NS_ERROR_OUT_OF_MEMORY);
> 
>   NS_ADDREF(*_retval = printSettings); // ref count
>-  InitPrintSettingsFromPrefs(*_retval, PR_FALSE, nsIPrintSettings::kInitSaveAll); // ignore return value
>+  InitPrintSettingsFromPrefs(*_retval, PR_FALSE,
>+                             nsIPrintSettings::kInitSaveAll);
>+  // ignore return value

A good way to indicate that a return value is intentionally ignore is simply to
cast the call itself to void.

>   return rv;
> }
> 
>-/* nsIPrintSettings CreatePrintSettings (); */
> NS_IMETHODIMP nsPrintOptions::CreatePrintSettings(nsIPrintSettings **_retval)
> {
>+  *_retval = nsnull;
>   return _CreatePrintSettings(_retval);
> }

What's the point of having this split into two functions?  Why not just move
all the code in _CreatePrintSettings here?

>+nsresult
>+nsPrintOptions::ReadPrefString(const char * aPrefId, nsAString& aString)
> {
>   NS_ENSURE_STATE(mPrefBranch);
>+  NS_ENSURE_ARG_POINTER(aPrefId);
>+
>   char * str = nsnull;
>   nsresult rv = mPrefBranch->GetCharPref(aPrefId, &str);
>-  if (NS_SUCCEEDED(rv) && str) {
>-    CopyUTF8toUTF16(str, aString);
>-    nsMemory::Free(str);
>+  if (NS_SUCCEEDED(rv)) {
>+    // By encapsulating str, we save copying and freeing it
>+    aString = nsDependentString((NS_ConvertUTF8toUTF16(str)));
>   }
>   return rv;
> }

The above code will leak, because str itself is never freed.  Since
NS_ConvertUTF8toUTF16 is itself implicitly copying the string into a stack
variable, there's no real savings there either.  Additionally, this code is
attempting to return a string dependant on a stack variable which is about to
go out of scope.  Either this is going to force a second copy, or else you'll
have a bug; I'm not sure which.  I'd vote with leaving this function that way
it was before.

>+void
>+nsPrintOptions::WriteInchesFromTwipsPref(const char * aPrefId, nscoord aTwips)
> {
>-  if (!mPrefBranch) {
>+  if (!mPrefBranch)
>     return;
>-  }

Why get rid of the braces here?  It's an invitation to a future error if
someone
decides to add some other piece of code to the else clause but forgets to add
them back.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

>+    aString = nsDependentString((NS_ConvertUTF8toUTF16(str)));

yeah, that should absolutely stay with CopyUTF8toUTF16. I'm surprised that you
can construct an nsDependentString like that... 

maybe that function should also use nsXPIDLCString.

Comment 28

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2.4 (diff -u)

This patch addresses dmose's comments.

Comment 29

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2.5 (diff -u)

The right patch this time.

Comment 30

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2.5 (diff -w)

Comment 31

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2.5.1 (diff -u)

timeless mentioned that when removing [noscript], I should switch from
InitialCaps to interCaps.

Comment 32

[Bastiaan Jacques]

Attachment: cleanup patch, revision 2.5.1 (diff -w)

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 189307 [details] [diff] [review]
cleanup patch, revision 2.5.1 (diff -u)

sr=dmose

Comment 34

[Bastiaan Jacques]

Checked in by timeless (2005-07-19 14:55). Thanks guys!

Comment 35

[Simon Fraser [no longer active]]

This caused a bunch of issues on Mac; see bug 306370.