202594 - Patch fixes use of uninitialized pointers, and warnings from compiling nsGfxFactoryGTK.cpp nsFreeType.cpp nsFT2FontCatalog.cpp

Status: RESOLVED WONTFIX

(Core Graveyard :: GFX: Gtk, defect)

Description

[Steven Augart]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4a) Gecko/20030415
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4a) Gecko/20030415

I fixed some "variable might be used uninitialized in this function" warnings
from g++.  In all cases, the warning was correct: the variable in question might
in fact have been used uninitialized under certain error conditions, so these
may  clear up an old bug or two.

I'll attach an attachment containing the patch and explanatory text showing the
context of each of the warnings, including the compiler invocation line that
generated it.  To summarize:

nsFT2FontCatalog.cpp: In member function `nsFontCatalogEntry* 
   nsFT2FontCatalog::NewFceFromFontFile(FT_LibraryRec_*, const char*, int, 
   int*)':
nsFT2FontCatalog.cpp:1307: warning: `FT_Error fterror' might be used 
   uninitialized in this function
nsFT2FontCatalog.cpp: In member function `nsFontCatalogEntry* 
   nsFT2FontCatalog::NewFceFromSummary(nsNameValuePairDB*)':
nsFT2FontCatalog.cpp:1609: warning: unused variable `int i'
nsFT2FontCatalog.cpp: In member function `long unsigned int 
   nsFT2FontCatalog::GetRangeLanguage(const nsACString&, short int)':
nsFT2FontCatalog.cpp:2205: warning: `long unsigned int*bit' might be used 
   uninitialized in this function
nsGfxFactoryGTK.cpp: In function `nsresult 
   nsScriptableRegionConstructor(nsISupports*, const nsIID&, void**)':
nsGfxFactoryGTK.cpp:177: warning: `nsIScriptableRegion*inst' might be used 
   uninitialized in this function
nsFreeType.cpp: In member function `PRBool nsFreeType2::InitLibrary()':
nsFreeType.cpp:595: warning: `FT_Error error' might be used uninitialized in 
   this function


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Steven Augart]

Attachment: (Av1) Patch and full documentation of the warnings it fixes.

Comment 2

[hacker formerly known as [e-mail]]

-> gfx:gtk

Comment 3

[Andrew Schultz]

confirming

Comment 4

[Andrew Schultz]

Comment on attachment 121026 [details] [diff] [review]
(Av1) Patch and full documentation of the warnings it fixes.

>+          if (rv) {

if (NS_FAILED(rv)) {

Comment 5

[Steven Augart]

Attachment: (Av1b) Patch and full documentation of the warnings it fixes.

This version integrates Andrew Schultz's correction from comment #4.
Thank you, Andrew.  I'm learning more about the system as I go along.
I had left out NS_FAILED() in two different places; both fixed now.

Comment 6

[Steven Augart]

I assume that I should reassign the bug to myself, since I have a patch all
written up and attached.  Since I'm new to this, please let me know if that's
not the right thing to do.

Comment 7

[Aleksey Nogin]

Comment on attachment 121046 [details] [diff] [review]
(Av1b) Patch and full documentation of the warnings it fixes.

>+  if (rgn == nsnull) {
>+    rv = NS_ERROR_OUT_OF_MEMORY;
>+    return rv;
>   }

Why not just "return NS_ERROR_OUT_OF_MEMORY"?

>+  unsigned long *bit = 0ul;

Shouldn't that be nsnull, not 0?

Comment 8

[Steven Augart]

Response to comment #7:

1)

nogin> patch>   rv = NS_ERROR_OUT_OF_MEMORY;
nogin> patch>   return rv;
nogin> Why not just "return NS_ERROR_OUT_OF_MEMORY"?

Because other code in that part of the file uses the construct:

   rv = <error-code>
   return rv;

and I wanted to be consistent, rather than leave people wondering "why did the
author of this code use the rv = ... construct in one place and the "return
<error-code> construct in another".
I'd prefer to change all of them to what seems to me the Right Way To Do It, but
I comfort myself by noting that it'll be optimized away in any case. 

(http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsGfxFactoryGTK.cpp#182 ,
http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsGfxFactoryGTK.cpp#202 ,
http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsGfxFactoryGTK.cpp#188 )

2) 
nogin> patch>+  unsigned long *bit = 0ul;
nogin> Shouldn't that be nsnull, not 0?

You're right.  Good call, Aleksey.  I'll upload a revised patch later tonight.

Comment 9

[Steven Augart]

Attachment: (Av1c) Patch to fix warnings; documentation of the warnings it fixes.

Uses "nsnull" correctly, as described in comment #7.

Comment 10

[Brendan Eich [:brendan]]

Why not cc: some module peers and owners, and actually request review from one
of them in the attachment manager?

/be

Comment 11

[timeless]

Comment on attachment 121520 [details] [diff] [review]
(Av1c) Patch to fix warnings; documentation of the warnings it fixes.

-    FREETYPE_PRINTF(("\n\n*********\nFreeType initialization error = %d",
-			  error));
Please move this to
nsFreeType2::InitFreeType(FT_Library *library)

so that you can actually spit out the error value.

-    FONT_SCAN_PRINTF(("  FreeType failed to open, error=%d", fterror));
same idea

-	   fterror = mFt2->SetCharmap(face, face->charmaps[i]);
-	   if (fterror) {
+	   rv = mFt2->SetCharmap(face, face->charmaps[i]);
This changes the behavior in the case where rv held something interesting. to
be used later. your diff doesn't have anywhere near enough context for me to be
able to determine whether that's a problem.

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <nsGfxFactoryGTK.cpp>

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 146097 [details] [diff] [review]
(Bv1) <nsGfxFactoryGTK.cpp>


This patch is based on both patch Av1c and bug 230397 patch Cv1.

I have no compiler: Could you compile/test/(super-)review/check in this patch ?
Thanks.

Comment 14

[Christopher Blizzard (:blizzard)]

Comment on attachment 146097 [details] [diff] [review]
(Bv1) <nsGfxFactoryGTK.cpp>

I'm not doing reviews at the moment.  Should go over to jshin anyway.

Comment 15

[Mats Palmgren (inactive)]

The remaining patch fixes mozilla/gfx/src/gtk/nsGfxFactoryGTK.cpp
which has been removed on trunk.  Please open a new bug if you have
additional fixes.  Thanks.

-> WONTFIX