117418 - nsWSRunObject.cpp:704: warning: `PRInt32 curStartOffset' might be used uninitialized in this function

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[timeless]

/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp: In function 
`static nsresult nsWSRunObject::PrepareToJoinBlocks(nsHTMLEditor *, nsIDOMNode 
*, nsIDOMNode *)':
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp:158: warning: 
unused variable `nsresult res'
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp: In function 
`static nsresult nsWSRunObject::PrepareToDeleteRange(nsHTMLEditor *, 
nsCOMPtr<nsIDOMNode> *, PRInt32 *, nsCOMPtr<nsIDOMNode> *, PRInt32 *)':
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp:176: warning: 
unused variable `nsresult res'
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp: In function 
`static nsresult nsWSRunObject::PrepareToSplitAcrossBlocks(nsHTMLEditor *, 
nsCOMPtr<nsIDOMNode> *, PRInt32 *)':
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp:213: warning: 
unused variable `nsresult res'
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp: In method 
`nsresult nsWSRunObject::InsertText(const nsAReadableString &, 
nsCOMPtr<nsIDOMNode> *, PRInt32 *, nsIDOMDocument *)':
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp:447: warning: 
comparison between signed and unsigned
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp: In method 
`nsresult nsWSRunObject::PriorVisibleNode(nsIDOMNode *, int, 
nsCOMPtr<nsIDOMNode> *, PRInt32 *, PRInt16 *)':
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp:572: warning: 
unused variable `nsresult res'
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp: In method 
`nsresult nsWSRunObject::NextVisibleNode(nsIDOMNode *, int, 
nsCOMPtr<nsIDOMNode> *, PRInt32 *, PRInt16 *)':
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp:627: warning: 
unused variable `nsresult res'
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp: In method 
`nsresult nsWSRunObject::GetWSNodes()':
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp:704: warning: 
`PRInt32 curStartOffset' might be used uninitialized in this function
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp:704: warning: 
`PRInt32 curEndOffset' might be used uninitialized in this function
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp: In method 
`nsresult nsWSRunObject::DeleteChars(nsIDOMNode *, int, nsIDOMNode *, int)':
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp:1604: warning: 
comparison between signed and unsigned
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp:1615: warning: 
comparison between signed and unsigned
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp: In method 
`nsresult nsWSRunObject::GetCharAfter(nsWSRunObject::WSPoint &, 
nsWSRunObject::WSPoint *)':
/mnt/3/braddr/mozilla/Linux_2.4.17-
rc1_Clobber/mozilla/editor/libeditor/html/nsWSRunObject.cpp:1730: warning: 
statement with no effect

The highlighted portions are for the #704 build warnings. they're especially 
scary and strange.

there are only two assignments:
curStartOffset = 0;
curEndOffset = len;
otherwise both of them are undefined. please clean this code out, it's scary. 
as is the code could be rewritten with cur*Offset as #defines.

Comment 1

[Joe Francis]

098

Comment 2

[kinmoz]

Hey Joe, this is pretty easy to fix, though, I wasn't sure if you were 
intentionally not checking for returned error values in some cases.

Also, as timeless points out, it's a bit strange how curStartOffset is used in 
nsWSRunObject::GetWSNodes(), was there some code that used to exist in that 
method that modified curStartOffset? If not perhaps we should just remove it, 
and init curEndOffset to zero.

Comment 3

[Joe Francis]

I have begun auditing this code, which was written in a blur of caffeine.  But 
I'm not done yet.  As you describe, it looks halfway rewritten.

Comment 4

[Joe Francis]

pushing off 098 to 099

Comment 5

[Joe Francis]

the swami says: things that will not land in 099!

Comment 6

[timeless]

Attachment: i prefer guru's

Comment 7

[Akkana Peck]

Comment on attachment 66420 [details] [diff] [review]
i prefer guru's

Strangely, I'm not seeing these warnings, though I usually do see "might be
used uninitialized" warnings.  Anyway, the fix looks like a good idea, r=akkana

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 66420 [details] [diff] [review]
i prefer guru's

sr=[e-mail]

Comment 9

[Aleksey Nogin]

Another "uninitialized" warnings that are still there:

editor/libeditor/html/nsWSRunObject.cpp:1804
  `PRInt32 startOffset' might be used uninitialized in this function
  `PRInt32 endOffset' might be used uninitialized in this function

Comment 10

[timeless]

fix checked in

Comment 11

[timeless]

hrm, yes i checked in *a* fix, but this bug isn't fixed. what in the world was 
i thinking?

Comment 12

[Joe Francis]

Attachment: Diffs of nsWSRunObject.cpp

I have rewritten this routine.

Comment 13

[timeless]

more lines removed than added :-)

   nsCOMPtr<nsIDOMNode> priorNode;
   while (!mStartNode)
   {
     // we haven't found the start of ws yet.  Keep looking
+    res = GetPreviousWSNode(curStartNode, blockParent, address_of(priorNode));
+    NS_ENSURE_SUCCESS(res, res);

1230 nsresult 
1231 nsWSRunObject::GetPreviousWSNode(nsIDOMNode *aStartNode, 
1232                                  nsIDOMNode *aBlockParent, 
1233                                  nsCOMPtr<nsIDOMNode> *aPriorNode)
1234 {
1235   // can't really recycle various getnext/prior routines because we have 
special needs
1236   // here.  Need to step into inline containers but not block containers.
1237   if (!aStartNode || !aBlockParent || !aPriorNode) return 
NS_ERROR_NULL_POINTER;

at first i thought i hade found a problem, and then i noticed the address_of.  
conclusion: this code is *scary*.

As lxr qa, http://lxr.mozilla.org/seamonkey/ident?i=GetPreviousWSNode turned up 
two hits for the same function definition, i'll try to file a bug about it 
later.

Comment 14

[Joe Francis]

Attachment: Diffs of nsWSRunObject.cpp, take 2

fixing typo in prior diff

Comment 15

[Joe Francis]

Attachment: Differences of nsWSRunObject.cpp

When it's this hard to get something right, it's a sign you have the wrong data
structures for the job.  I revamped some stuff to make this routine simpler.

Comment 16

[Joe Francis]

Attachment: Differences of nsWSRunObject.h

header file changes

Comment 17

[Joe Francis]

timeless, with latest patch yo uwill see 3 versions of GetPreviousWSNode(). 
However, this is not a bug, merely overloading.  Nor is using pointers to
comptrs a bug, though I admit it is pretty ugly here.  

Comment 18

[timeless]

ok. it turns out my eyes deceived me. the problem was that i thought i saw two 
links to the same line, it turns out i transposed digits.  If i had seen three, 
i probably wouldn't have worried (although it'd really help if i rewrote lxr to 
display the definitions).

Comment 19

[kinmoz]

Comment on attachment 67680 [details] [diff] [review]
Differences of nsWSRunObject.cpp

sr=[e-mail] with the changes we discussed.

Comment 20

[kinmoz]

Comment on attachment 67681 [details] [diff] [review]
Differences of nsWSRunObject.h

sr=[e-mail]

Comment 21

[Kathleen :Brade]

Comment on attachment 67681 [details] [diff] [review]
Differences of nsWSRunObject.h

r=brade

Comment 22

[Kathleen :Brade]

Comment on attachment 67680 [details] [diff] [review]
Differences of nsWSRunObject.cpp

rs=brade

Comment 23

[Joe Francis]

fix landed on trunk

Comment 24

[timeless]

Can you fix this too: 9-10.
      editor/libeditor/html/nsWSRunObject.cpp:1711 (See 1st of 2 warnings in 
build log)
      `PRInt32 startOffset' might be used uninitialized in this function
      `PRInt32 endOffset' might be used uninitialized in this function
      1709 
      1710   nsCOMPtr<nsIDOMNode> startNode, endNode;
      1711   PRInt32 startOffset, endOffset;
      1712 
      1713   nsresult res = NS_OK;

you might fix the following for bonus points:
8.
      editor/libeditor/html/nsWSRunObject.cpp:1614 (See build log excerpt)
      Statement with no effect
      1612 {
      1613   *outPoint = aPoint;
      1614   outPoint->mOffset;
      1615   outPoint->mChar = GetCharAt(aPoint.mTextNode, aPoint.mOffset);
      1616 }
15.
      editor/libeditor/html/nsWSRunObject.cpp:71 (See build log excerpt)
      `PRBool IsInlineNode(nsIDOMNode *)' defined but not used
      69 
      70 static PRBool IsInlineNode(nsIDOMNode* node)
      71 {
      72   return !IsBlockNode(node);
      73 }

Comment 25

[sujay]

timeless can you verify this one? thanks.

Comment 26

[Aleksey Nogin]

I still see some "might be used uninitialized" warnings in nsWSRunObject.cpp

editor/libeditor/html/nsWSRunObject.cpp:1711
 `PRInt32 startOffset' might be used uninitialized in this function
 `PRInt32 endOffset' might be used uninitialized in this function

Comment 27

[Kathleen :Brade]

reopen to address uninitialized variables

Comment 28

[Joe Francis]

might be, but theyt aren't.  Compiler seems a bit picky here.

Comment 29

[Aleksey Nogin]

If GetCharAfter/GetCharBefore fails or returns a point with a null mTextNode,
startOffset and endOffset will be used (and returned in outStartOffset and
outEndOffset!) uninitialized in nsWSRunObject::GetAsciiWSBounds

Comment 30

[Akkana Peck]

Even aside from that: warnings make it more difficult for other developers,
since they have to wade through old warnings to figure out whether they've
introduced any new problems.

Comment 31

[Aleksey Nogin]

editor no longer has any "xxx might be used uninitialized" warnings. Thanks!

Comment 32

[timeless]

vrfy fixed (brad tinderbox)