Closed Bug 202594 Opened 17 years ago Closed 13 years ago

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

Categories

(Core Graveyard :: GFX: Gtk, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: saugart, Assigned: saugart)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
-> gfx:gtk
Assignee: seawood → blizzard
Component: Build Config → GFX: Gtk
QA Contact: granrose → ian
confirming
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 121026 [details] [diff] [review]
(Av1) Patch and full documentation of the warnings it fixes.

>+          if (rv) {

if (NS_FAILED(rv)) {
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.
Attachment #121026 - Attachment is obsolete: true
Attachment #121046 - Flags: review?
Attachment #121046 - Flags: review? → review?(Louie.Zhao)
Summary: Patch fixes warnings from compiling nsGfxFactoryGTK.cpp nsFreeType.cpp nsFT2FontCatalog.cpp → Patch fixes use of uninitialized pointers, and warnings from compiling nsGfxFactoryGTK.cpp nsFreeType.cpp nsFT2FontCatalog.cpp
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.
Assignee: blizzard → steve+bugzilla
Blocks: 59652, 179819
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?
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.
Uses "nsnull" correctly, as described in comment #7.
Attachment #121046 - Attachment is obsolete: true
Attachment #121046 - Flags: review?(Louie.Zhao)
Attachment #121520 - Flags: review?
Why not cc: some module peers and owners, and actually request review from one
of them in the attachment manager?

/be
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.
Attachment #121520 - Flags: review? → review?(bstell)
Attachment #121026 - Attachment description: Patch and full documentation of the warnings it fixes. → (Av1) Patch and full documentation of the warnings it fixes.
Attachment #121046 - Attachment description: Patch and full documentation of the warnings it fixes. → (Av1b) Patch and full documentation of the warnings it fixes.
Attachment #121520 - Attachment description: Patch to fix warnings; documentation of the warnings it fixes. → (Av1c) Patch to fix warnings; documentation of the warnings it fixes.
Attachment #121520 - Attachment is obsolete: true
Attachment #121520 - Flags: review?(bstell)
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.
Attachment #146097 - Flags: superreview?(blizzard)
Attachment #146097 - Flags: review?(blizzard)
Blocks: 59668
No longer blocks: 59652, 179819
Attachment #146097 - Flags: review?(blizzard) → review?(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.
Attachment #146097 - Flags: superreview?(blizzard)
Attachment #146097 - Flags: review?(jshin1987)
Attachment #146097 - Flags: review?(blizzard)
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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
Attachment #146097 - Flags: review?(jshin1987)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.