287106 - fix PreFast warnings in layout/base

Status: NEW

(Core :: Layout, defect)

Description

[Bernd]

e:\moz_src\mozilla\layout\base\nsbidiutils.cpp (441): warning 262: This function
uses 16476 bytes of stack:  exceeds /StackHogThreshold=16384.  Consider moving
some data to heap.
	FUNCTION: Conv_06_FE_WithReverse (441)
	PATH: 

e:\moz_src\mozilla\layout\base\nscssframeconstructor.cpp (7098): warning 246:
Local declaration of 'isBlock' hides declaration of the same name in an outer
scope:  see previous declaration at line 7072 of
e:\moz_src\mozilla\layout\base\nscssframeconstructor.cpp.
	FUNCTION: nsCSSFrameConstructor::ConstructXTFFrame (7058)
	PATH: 7072 

e:\moz_src\mozilla\layout\base\nscssframeconstructor.cpp (7449): warning 246:
Local declaration of 'disp' hides declaration of the same name in an outer
scope:  see previous declaration at line 7327 of
e:\moz_src\mozilla\layout\base\nscssframeconstructor.cpp.
	FUNCTION: nsCSSFrameConstructor::ConstructSVGFrame (7312)
	PATH: 7327 

e:\moz_src\mozilla\layout\base\nscssframeconstructor.cpp (9838): warning 246:
Local declaration of 'placeholderFrame' hides declaration of the same name in an
outer scope:  see previous declaration at line 9797 of
e:\moz_src\mozilla\layout\base\nscssframeconstructor.cpp.
	FUNCTION: nsCSSFrameConstructor::ContentRemoved (9634)
	PATH: 9797 

e:\moz_src\mozilla\layout\base\nscssframeconstructor.cpp (9868): warning 246:
Local declaration of 'placeholderFrame' hides declaration of the same name in an
outer scope:  see previous declaration at line 9797 of
e:\moz_src\mozilla\layout\base\nscssframeconstructor.cpp.
	FUNCTION: nsCSSFrameConstructor::ContentRemoved (9634)
	PATH: 9797 

e:\moz_src\mozilla\layout\base\nscssframeconstructor.cpp (10245): warning 246:
Local declaration of 'frame' hides declaration of the same name in an outer
scope:  see previous declaration at line 10209 of
e:\moz_src\mozilla\layout\base\nscssframeconstructor.cpp.
	FUNCTION: nsCSSFrameConstructor::ProcessRestyledFrames (10184)
	PATH: 10209 

e:\moz_src\mozilla\layout\base\nsCaret.cpp (935): warning 286: (<non-zero
constant> || <expression>) is always TRUE.  <expression> is never evaluated and
may have side effects.
	FUNCTION: nsCaret::GetCaretRectAndInvert (905)
	PATH: 

e:\moz_src\mozilla\layout\base\nsCaret.cpp (1059): warning 246: Local
declaration of 'domSelection' hides declaration of the same name in an outer
scope:  see previous declaration at line 978 of
e:\moz_src\mozilla\layout\base\nsCaret.cpp.
	FUNCTION: nsCaret::GetCaretRectAndInvert (905)
	PATH: 978 

e:\moz_src\mozilla\layout\base\nsdocumentviewer.cpp (553): warning 1: Using
uninitialized memory 'rv'.
	FUNCTION: DocumentViewerImpl::LoadStart (537)
	PATH: 543 544 547 553 

e:\moz_src\mozilla\layout\base\nsdocumentviewer.cpp (786): warning 246: Local
declaration of 'rv' hides declaration of the same name in an outer scope:  see
previous declaration at line 764 of
e:\moz_src\mozilla\layout\base\nsdocumentviewer.cpp.
	FUNCTION: DocumentViewerImpl::InitInternal (756)
	PATH: 764 

e:\moz_src\mozilla\layout\base\nsdocumentviewer.cpp (948): warning 1: Using
uninitialized memory 'status'.
	FUNCTION: DocumentViewerImpl::DumpContentToPPM (852)
	PATH: 854 856 857 858 859 860 864 865 867 868 869 870 871 873 875 878 879 883
886 887 888 891 892 893 895 896 896 897 899 902 903 904 906 907 908 909 934 936
938 883 943 944 945 946 948 

e:\moz_src\mozilla\layout\base\nsframecontentiterator.cpp (197): warning 11:
Dereferencing NULL pointer 'prevSibling'.
	FUNCTION: GetPrevChildFrame (169)
	PATH: 171 171 171 175 176 177 179 179 179 180 181 185 186 188 196 197 

1 of 1 Defect Listed
e:\moz_src\mozilla\layout\base\nsframemanager.cpp (1427): warning 326: Potential
comparison of a constant with constant.
	FUNCTION: nsFrameManager::ReResolveStyleContext (1279)
	PATH: 


e:\moz_src\mozilla\layout\base\nspresshell.cpp (4873): warning 11: Dereferencing
NULL pointer 'before'.
	FUNCTION: PresShell::CancelReflowCallback (4856)
	PATH: 4858 4859 4860 4862 4864 4866 4867 4872 4873 

1 of 1 Defect Listed
e:\moz_src\mozilla\layout\base\nsbidipresutils.cpp (401): warning 1: Using
uninitialized memory 'directionalFrame'.
	FUNCTION: nsBidiPresUtils::InitLogicalArray (361)
	PATH: 366 367 368 369 371 372 374 375 377 378 379 380 400 401

Comment 1

[Bernd]

Attachment: patch

Boris, I am little bit uncertain at two places the shadowing of isblock in the
frame constructor looks like a bug to me, I don't know only how to trigger
this. Second is the DocumentViewerImpl::LoadStart routine.
I am pretty tempted to rewrite the thing as:


  if (!mDocument) {
    mDocument = do_QueryInterface(aDoc, &rv);
  }
  else if (mDocument == aDoc) {
    // Reset the document viewer's state back to what it was
    // when the document load started.
    PrepareToStartLoad();
  }
  else {
    NS_WARN_IF_FALSE("Incorrect call of LoadStart");
    rv = NS_ERROR_UNEXPECTED;
  }

  return rv;

Comment 2

[Boris Zbarsky [:bzbarsky]]

I don't really know what the XTF code is trying to do.  Try asking afri or
bryner (who reviewed it)?

I'll try to look at the rest sometime this week....

Comment 3

[Bernd]

Alex, could you shed some light why the isblock does shadow, I assume its a bug.
Is there any testcase that would execute this code. Thanks

Comment 4

[Brendan Eich [:brendan]]

Cc'ing bryner.

/be

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 178674 [details] [diff] [review]
patch

>Index: nsBidiPresUtils.cpp

> nsBidiPresUtils::InitLogicalArray(nsPresContext* aPresContext,

>-  nsIFrame*             directionalFrame;
>+  nsIFrame*             directionalFrame= nsnull;

Hmm... that looks like a bogus warning to me.  Is there really no way to shut
it up without adding this unnecessary initializer?

In any case, this would need a space before the '='.


>Index: nsBidiUtils.cpp

>@@ -442,16 +442,20 @@ nsresult Conv_06_FE_WithReverse(const ns

>+  PRUint32 len=8192;
>+  PRUnichar *buf = (PRUnichar *) nsMemory::Alloc(len * sizeof(PRUnichar));
>+  NS_ENSURE_TRUE(buf, NS_ERROR_OUT_OF_MEMORY);

Hmm... Why this change?  This allocates the buffer in all cases, even those
when we don't need it, no?  What was wrong with the existing code?

>Index: nsCSSFrameConstructor.cpp
>@@ -7095,17 +7095,17 @@ nsCSSFrameConstructor::ConstructXTFFrame

Skipping this part of the patch, since I have no idea what the right behavior
here is.

>@@ -9818,18 +9817,17 @@ nsCSSFrameConstructor::ContentRemoved(ns

I this method, I think the placeholder should just not be declared in the outer
scope -- it's not used there....

The rest looks ok, but marking r- per above issues.

Comment 6

[Brendan Eich [:brendan]]

Coverity seems to do a better job than PreFast -- it avoids warning in ways that
cause obviously unnecessary and suboptimal changes.  Cc'ing dveditz.

/be

Comment 7

[Bernd]

most of the prefast warnings would go away if we would fix the -Wshadow
warnings, why they are not enabled on gcc is beyond my understanding, there is
probably a bug on it but I can't find it)

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.