Source code of WPF samples by Microsoft was checked

Source code of WPF samples by Microsoft
was checked
Author: Vitaliy Alferov
Date: 27.06.2016
To let people know about PVS-Studio, which is now able to check not only C++ projects, but C# as well,
we decided to check the source code of WPF examples, offered by Microsoft.
Upon Windows Vista release, the company introduced a new subsystem for rendering user interfaces in
Windows-based applications - Windows Presentation Foundation (WPF). This graphic subsystem is a
part of the .NET Framework, starting with version 3.0. It uses XAML markup language. Now, it has
almost replaced the outdated WinForms. In my humble opinion, the main disadvantage of WinForms,
was the fact that it was doing all the rendering on the CPU. WPF approached this in a more sensible way,
and let DirectX do the rendering of the components. Now WPF allows the making of universal interfaces
for three platforms at once (PC, XBOXOne, Winphone), and has practically ousted WinForms.
To do the analysis of WPF examples from Microsoft (the source code of the examples), we used PVS-
Studio static code analyzer, version 6.05.
An interesting thing about this solution, is the fact that along with the projects written in C#, there are
also several C++ projects. But I found it only from the list of the bugs found by PVS-Studio. PVS-Studio
plugin for Visual Studio, without any additional settings from the user's side, performed the analysis and
displayed warnings for both C++ and C# projects.

Figure 1. As you can see, in the PVS-Studio window there are warnings issued for both C# and C++ code
(click on the image to enlarge).
C# Errors
1. Errors made during the forming the conditions of the if statement
For programmers it's a common problem - errors in the comparisons. Let's have a look at them.
In this code there are two absolutely identical conditions:
public int Compare(GlyphRun a, GlyphRun b)
{
....
if (aPoint.Y > bPoint.Y) //<==
{
return -1;
}
else if (aPoint.Y > bPoint.Y) //<==
{
result = 1;
}
else if (aPoint.X < bPoint.X)
{
result = -1;
}
else if (aPoint.X > bPoint.X)
{
result = 1;
}
....

}
V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error
presence. Check lines: 418, 422. txtserializerwriter.cs 418
It's not really clear what was meant here, but apparently, it was something different from what we see
now.
We like to do the verifications against null in the conditions, and thus try to protect the program from
emergency events. We can even say that the majority of if conditions are the null-checks of some fields
or variables. But sometimes such checks can be redundant and even contain logical errors:
public static string FindNumeric(string content)
{
string[] values = content.Split(' ');
if (values != null)
{
return values[0];
}
return "none";
}
V3022 Expression 'values != null' is always true. Util.cs 287
We could assume that the author wanted to check that values has more than 0 elements, I personally
couldn't think of a situation where Split returns an empty array. Anyway, the verification against null is
completely unnecessary here.
As I have already said, the project contains code from C++ and C# diagnostics. I got the impression that
the following code was written by a C++ programmer.
private void LoadNotes()
{
var fs = new FileStream("NotesFile", FileMode.Open);
if (fs != null)
{
....
}
V3022 Expression 'fs != null' is always true. MainWindow.cs 66
Actually, even in C++ this variant is erroneous, in C# it will at least look "weird". More details of why it is
incorrect to write such code are given in the article "Checking 7-Zip with PVS-Studio analyzer" and we'll
continue looking at C# code.
We don't have to go far to find more buggy fragments. There were two practically identical functions in
the solution (thanks to copy-paste) with the same error:
private void SerializeObjectTree(object objectTree)
{

TextWriter writer = new StreamWriter(_stream);
try
{
string fileContent =
XamlRtfConverter.ConvertXamlToRtf(
XamlWriter.Save(objectTree));
writer.Write(fileContent);
}
finally
{
if (writer != null)
writer.Close();
}
}
V3022 Expression 'writer != null' is always true. htmlserializerwriter.cs 324
Writer won't be a null reference...
Throwing an error in exceptional situations is not the worst decision. But the main thing is not to make
an error in the condition when the exception should be thrown, because it can create an unpleasant
impression in the eyes of our user, when the program crashes all of a sudden.
protected Size SizeParser(string o)
{
....
if (sizeString.Length == 0 || sizeString.Length != 2)
throw new ApplicationException("Error: a size should
contain two double that seperated by a space
or ',' or ';'");
....
}
V3023 Consider inspecting the 'sizeString.Length == 0 || sizeString.Length != 2' expression. The
expression is excessive or contains a misprint. MainWindow.cs 140
Judging by the text of the error, the comparison with 0 is excessive, it was enough to check if
sizeString.Length is not equal to 2.
In the long bodies of if instruction sometimes it's very hard to notice meaningless checks while doing
code review.
private static void WriteElement(....)
{

if (htmlWriter == null)
{
....
}
else
{
if (htmlWriter != null && htmlElementName != null)
{
....
....
}
V3063 A part of conditional expression is always true: htmlWriter != null. HtmlFromXamlConverter.cs
491
It's no problem for the analyzer. By the way, thanks to our beloved copy-paste, an error was found in
two projects: HtmlToXamlDemo and DocumentSerialization.
Of course meaningless checks can be found not only in long functions, but within several strings.
private void OnFlipPicTimeline(object sender, EventArgs e)
{
var clock = (Clock) sender;
if (clock.CurrentState == ClockState.Active) // Begun case
{
return;
}
if (clock.CurrentState != ClockState.Active) // Ended case
{
....
}
}
V3022 Expression 'clock.CurrentState != ClockState.Active' is always true. MainWindow.cs 103
In general, it's quite fine, but when later we have an if statement nested in another if statement, and
another... If only we could get rid of meaningless checks for better understanding of the code, which is
read more often than it is written...
Let's take a short break and have a look at one function that I have recently come across. This is the
body of the function:
private void UpdateSavings()
{

Savings = TotalIncome - (Rent + Misc + Food);
if (Savings < 0)
{
}
else if (Savings >= 0)
{
}
}
V3022 Expression 'Savings >= 0' is always true. NetIncome.cs 98
Also we have found a lot (more than 60) comparisons of real numbers (double) with a precise 0.
if (leftConst != null && leftConst.Value == 0)
{
// 0 + y; return y;
return newRight;
}
For example:
 V3024 An odd precise comparison: leftConst.Value == 0. Consider using a comparison with
defined precision: Math.Abs(A - B) < Epsilon. AddExpression.cs 34
 V3024 An odd precise comparison: leftConst.Value == 1. Consider using a comparison with
defined precision: Math.Abs(A - B) < Epsilon. MultExpression.cs 42
 V3024 An odd precise comparison: leftConst.Value == -1. Consider using a comparison with
defined precision: Math.Abs(A - B) < Epsilon. MultExpression.cs 47
 and so on ...
All the lines won't fit in one article. This warning is third level for us, because, its relevance strongly
depends on the specifics of the program. In case there are mathematical evaluations (manipulations
with the value), there is no guarantee that we will get a specific number: -1, 0, 1. But even slight
deviation in 0.00000000001 will lead to incorrect result in comparisons. But if the program logic
presupposes writing discrete values to the real numbers (double), then these checks aren't a mistake.
2. Errors in the initialization and assigning of variables
Functions are great things that help not only to remove duplicate code, but simplify the readability of
the code where this function is used. It is especially important that this function will do exactly the task
that is stated in its name, and the signature of the call. But this is not always the case, for example,
consider the following code fragment. I'll write the whole function so you can understand the situation
clearer.
public bool OpenDocument(string fileName)
{
Microsoft.Win32.OpenFileDialog dialog;
// If there is a document currently open, close it.
if (this.Document != null) CloseFile();

dialog = new Microsoft.Win32.OpenFileDialog();
dialog.CheckFileExists = true;
dialog.InitialDirectory = GetContentFolder();
dialog.Filter = this.OpenFileFilter;
bool result = (bool)dialog.ShowDialog(null);
if (result == false) return false;
fileName = dialog.FileName; //<==
return OpenFile(fileName);
}
V3061 Parameter 'fileName' is always rewritten in method body before being used.
ThumbViewer.xaml.cs 192
The name of the file that should be opened, is lost right before its first use fileName = dialog.FileName.
Yes, a dialog window will be opened and the user file will be chosen, but why do we need a parameter
that isn't really used?
Lack of time and copy-paste sometimes produces very strange constructions:
public MailSettingsDialog()
{
....
_timerClock = _timerClock = new DispatcherTimer();
....
}
V3005 The '_timerClock' variable is assigned to itself. MailSettingsDialog.cs 56
This may not seem the most horrible typo, but it makes us think, "are we writing to the correct place for
the second time?" Well, for example, like this:
private void DumpAllClipboardContentsInternal()
{
....
if (dataObject == null)
{
clipboardInfo.Text =
clipboardInfo.Text =
"Can't access clipboard now!
nnPlease click Dump All Clipboard
Contents button again.";
}

else
{
....
}
V3005 The 'clipboardInfo.Text' variable is assigned to itself. MainWindow.cs 204
In general, the code abounds in strange assignments:
private void DoParse(string commandLine)
{
....
strLeft = strRight = string.Empty;
strLeft = strs[0];
strRight = strs[1];
....
}
V3008 The 'strLeft' variable is assigned values twice successively. Perhaps this is a mistake. Check lines:
55, 54. CommandLine.cs 55
V3008 The 'strRight' variable is assigned values twice successively. Perhaps this is a mistake. Check lines:
56, 54. CommandLine.cs 56
strLeft and strRight - are just local variables of string type.
The following code is even more incorrect. For some reason the programmer did a lot of evaluations and
reevaluations and then wrote it into the same variable.
private object InvokMethod(....)
{
arg = commandLine.Substring(
commandLine.IndexOf("(", StringComparison.Ordinal) + 1,
commandLine.IndexOf(")",
StringComparison.Ordinal) -
(commandLine.IndexOf("(",
StringComparison.Ordinal) + 1));
arg = commandLine.Substring(
commandLine.IndexOf("(",
StringComparison.Ordinal) + 1);
}
V3008 The 'arg' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 176,
173. CommandLine.cs 176
And some more examples of meaningless primary assignments:

private void DrawFormattedText(DpiScale dpiInfo)
{
....
Geometry geometry = new PathGeometry();
geometry = formattedText.BuildGeometry(
new System.Windows.Point(0, 0));
....
}
 V3008 The 't' variable is assigned values twice successively. Perhaps this is a mistake. Check
lines: 141, 115. TrackBall.cs 141
 V3008 The 't' variable is assigned values twice successively. Perhaps this is a mistake. Check
lines: 141, 115. TrackBall.cs 141
 V3008 The 'columnSpan' variable is assigned values twice successively. Perhaps this is a mistake.
Check lines: 2115, 2101. HtmlToXamlConverter.cs 2115
 V3008 The '_timerInterval' variable is assigned values twice successively. Perhaps this is a
mistake. Check lines: 52, 47. ClientForm.cs 52
 V3008 The 'matrix1' variable is assigned values twice successively. Perhaps this is a mistake.
Check lines: 126, 125. MainWindow.cs 126
 V3008 The 'matrixResult' variable is assigned values twice successively. Perhaps this is a mistake.
 V3008 The 'pointResult' variable is assigned values twice successively. Perhaps this is a mistake.
 V3008 The 'columnSpan' variable is assigned values twice successively. Perhaps this is a mistake.
Check lines: 1930, 1917. htmltoxamlconverter.cs 1930
 V3008 The 'geometry' variable is assigned values twice successively. Perhaps this is a mistake.
Check lines: 56, 55. MainWindow.xaml.cs 56
 V3008 The 'pathGeometry' variable is assigned values twice successively. Perhaps this is a
mistake. Check lines: 66, 65. MainWindow.xaml.cs 66
There is no point in writing each example, more interesting bugs are waiting ahead.
3. A couple of miscellaneous errors
Throwing the exception, it's important to save the stack call, so that we can later understand when
looking at the logs, 'what exactly went wrong on the user's side', But not everybody knows how to do
that.
public static object InvokePropertyOrMethod(....)
{
try
{
....
}
catch (MissingMethodException e)

{
....
throw e;
}
catch (AmbiguousMatchException e)
{
throw e;
}
return resultObject;
}
V3052 The original exception object 'e' was swallowed. Stack of original exception could be lost.
ReflectionUtils.cs 797
V3052 The original exception object 'e' was swallowed. Stack of original exception could be lost.
ReflectionUtils.cs 806
According to the standard, if we pass the exception above in the function call stack by means of throw
e;, we'll lose the call stack that was before the catch of the exception in the catch block. To keep the
whole stack call, and its further continuation, we just need to write one throw word in the catch block
and that's it.
Sometimes the checks are unnecessary, and sometimes they aren't enough as in the following code:
private static void ParseCssFontFamily(....)
{
....
if (fontFamilyList == null && fontFamily.Length > 0)
{
if (fontFamily[0] == '"' || fontFamily[0] == ''')
{
// Unquote the font family name
fontFamily =
fontFamily.Substring(1, fontFamily.Length - 2);
....
}
V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect
the second argument. HtmlCSSParser.cs 645
There is no check that fontFamily.Length is bigger than 1, thus, subtracting from fontFamily.Length
number 2 we can get a value less than 0. And in such cases this function throws an exception
ArgumentOutOfRangeException.
If would be safer to write a check:
if (fontFamilyList == null && fontFamily.Length > 1)

4. WPF bug
The DependencyProperty is one of the most remarkable features of WPF. Creating properties that can
notify the developer right from the box about the changes made is incredibly convenient. But the main
thing is to avoid confusing the signature to describe them, it is particularly important to remember this
when showing the examples, because that's what people judge by
public double Radius
{
get { return (double) GetValue(RadiusProperty); }
set { SetValue(RadiusProperty, value); }
}
public static readonly DependencyProperty
RadiusProperty = DependencyProperty.Register(
"RadiusBase",
typeof (double),
typeof (FireworkEffect),
new FrameworkPropertyMetadata(15.0));
V3045 WPF: the names of the registered property 'RadiusBase', and of the property 'Radius', do not
correspond with each other. FireworkEffect.cs 196
In this particular case, the name that is registered for a dependency property does not match the name
of the wrapper property to access the DependencyProperty from the code. This option causes big
problems when working from XAML markup. WPF allows from XAML access a simple property Radius
and read the value from it, but the changes of this property won't get fetched from XAML.
Actually, in PVS-Studio, there are a number of diagnostics to detect errors in the signature when
creating DependencyProperty [3044, 3045, 3046, 3047, 3048, 3049]. But most errors of this kind lead to
the program crash as soon as the program starts using the class with these dependency properties.
That's why these diagnostics are intended to save us from searching and analyzing long texts of
signatures, especially after copying. Of course, the most efficient would be to check the code with PVS-
Studio regularly, not just do the analysis of the final version of the program.
Let's look at another interesting warning. In this case it was our new diagnostic V3095. This diagnostic
shows the places where we access the variable first, and then verify it against null.
private static XmlElement AddOrphanListItems(....)
{
Debug.Assert(htmlLiElement.LocalName.ToLower() == "li");
....
XmlNode htmlChildNode = htmlLiElement;
var htmlChildNodeName = htmlChildNode == null
? null
: htmlChildNode.LocalName.ToLower();
....

}
V3095 The 'htmlLiElement' object was used before it was verified against null. Check lines: 916, 936.
HtmlToXamlConverter.cs 916
In this case, in the condition of the ternary operator we check if the variable htmlChildNode can be null.
At the same time the variable htmlChildNode, is nothing more than a reference to the variable
htmlLiElement. But we accessed the variable htmlLiElement without the verification against null. As a
result, we have code that will never be executed, or we'll get an exception NullReferenceException in the
string htmlLiElement.LocalName.ToLower().
Besides the errors that we've described, a lot of attention is drawn to the diagnostic V3072, which is
meant for detecting fields with the type that is implemented by the IDisposable interface, but the class
where the fields aren't declared doesn't have this implementation.
internal class Email
{
private readonly SmtpClient _client;
....
}
V3072 The 'Email' class containing IDisposable members does not itself implement IDisposable. Inspect:
_client. Email.cs 15
IDisposable has always been troublesome. Sometimes Finalize can be of great help, at least in standard
classes, to avoid critical errors related to its incorrect usage. Programmers often forget, miss, or just
don't pay attention to the field with the type, implementing this interface. It's not that easy to justify
such code, or admit having an error there while doing code review, but there are patterns that are
worth paying attention to. In this solution there were also quite a lot of these warnings:
 V3072 The 'HtmlLexicalAnalyzer' class containing IDisposable members does not itself
implement IDisposable. Inspect: _inputStringReader. HtmlLexicalAnalyzer.cs 16
 V3072 The 'MainWindow' class containing IDisposable members does not itself implement
IDisposable. Inspect: _customersTableAdapter, _nwDataSet... MainWindow.cs 15
IDisposable. Inspect: _listControl. MainWindow.cs 14
 V3072 The 'ThumbViewer' class containing IDisposable members does not itself implement
IDisposable. Inspect: _annStore, _annotationBuffer. ThumbViewer.xaml.cs 31
 V3072 The 'HtmlLexicalAnalyzer' class containing IDisposable members does not itself
implement IDisposable. Inspect: _inputStringReader. htmllexicalanalyzer.cs 24
IDisposable. Inspect: _store. MainWindow.cs 20
IDisposable. Inspect: _customCursor. MainWindow.cs 14
IDisposable. Inspect: _speechSynthesizer. MainWindow.cs 14
C++ Errors
1. Errors when writing if statement conditions
It was quite a revelation for me to find C++ projects in this Solution, but nevertheless these are also
bugs, so let's take a look.

As in C#, let's start with various comparisons. Let's look at that very C++ bug that I mentioned in the C#
block.
STDMETHOD(CreateInstance)(....)
{
....
T *obj = new T();
if (NULL != obj)
{
....
}
V668 There is no sense in testing the 'obj' pointer against null, as the memory was allocated using the
'new' operator. The exception will be generated in the case of a memory allocation error. classfactory.h
76
If the new operator was unable to allocate the memory, then according to the C++ standard, an
exception std::bad_alloc() is thrown. Thus, the verification against null is meaningless, as the obj pointer
will never be equal to NULL. If it is impossible to allocate the memory, then we have an exception which
should be handled on a higher level, and the verification against null can just be deleted. In case it's not
desirable to have exceptions in the application, we can use the new operator which doesn't generate
exceptions (T *obj = new (std::nothrow) T()), , and thus, the return value can be verified against null.
There were four more similar checks in the Solution:
 V668 There is no sense in testing the 'colors' pointer against null, as the memory was allocated
using the 'new' operator. The exception will be generated in the case of memory allocation
error. aitdecoder.cpp 182
 V668 There is no sense in testing the 'pixels' pointer against null, as the memory was allocated
error. aitencoder.cpp 157
 V668 There is no sense in testing the 'colors' pointer against null, as the memory was allocated
 V668 There is no sense in testing the 'bytes' pointer against null, as the memory was allocated
Excessive conditions are common for both programming languages:
if (bitmapLock && bitmap)
{
if(bitmapLock)
{
bitmapLock->Release();
bitmapLock = NULL;
}
}

V571 Recurring check. The 'bitmapLock' condition was already verified in line 104. aitdecoder.cpp 106
Some C# programmers aren't aware that the following two operations over the Nullable type are
equivalent:
 _isInDesignMode != null
 _isInDesignMode.HasValue
Thatt's why they write the following checks:
if (_isInDesignMode != null && _isInDesignMode.HasValue)
At the same time, C++ people like to make pointless verifications against null, before freeing the
memory that was allocated by the address that it points to.
static HRESULT OutputColorContext(....)
{
....
if (pixels)
delete[] pixels;
....
}
V809 Verifying that a pointer value is not NULL is not required. The 'if (pixels)' check can be removed.
aitencoder.cpp 189
static HRESULT OutputBitmapPalette(....)
{
....
if (colors)
delete[] colors;
....
}
V809 Verifying that a pointer value is not NULL is not required. The 'if (colors)' check can be removed.
aitencoder.cpp 241
static HRESULT OutputColorContext(....)
{
if (bytes)
delete[] bytes;
}
V809 Verifying that a pointer value is not NULL is not required. The 'if (bytes)' check can be removed.
aitencoder.cpp 292
2. Logic error
The following code shows quite an interesting situation of logical comparison, although you wouldn't
say so.

STDMETHODIMP AitDecoder::QueryCapability(....)
{
....
// If this is our format, we can do everything
if (strcmp(bh.Name, "AIT") == 0)
{
*pCapability =
WICBitmapDecoderCapabilityCanDecodeAllImages ||
WICBitmapDecoderCapabilityCanDecodeThumbnail ||
WICBitmapDecoderCapabilityCanEnumerateMetadata ||
WICBitmapDecoderCapabilitySameEncoder;
}
....
}
V560 A part of conditional expression is always true. aitdecoder.cpp 634
The diagnostic thought that a part of the condition is always true and it is really right, as the words
WICBitmapDecoderCapabilityCanDecodeXXX are just enum values withe the name
WICBitmapDecoderCapabilities:
enum WICBitmapDecoderCapabilities
{
WICBitmapDecoderCapabilitySameEncoder = 0x1,
WICBitmapDecoderCapabilityCanDecodeAllImages = 0x2,
WICBitmapDecoderCapabilityCanDecodeSomeImages = 0x4,
WICBitmapDecoderCapabilityCanEnumerateMetadata = 0x8,
WICBitmapDecoderCapabilityCanDecodeThumbnail = 0x10,
WICBITMAPDECODERCAPABILITIES_FORCE_DWORD = 0x7fffffff
};
As a result, perhaps, someone confused the symbols, and instead of the bitwise OR "|" wrote logical OR
"||". In contrast to the C# compiler, the C++ one didn't see a problem with it.
3. Error in initialization and assigning variables
Of course after refactoring we may have variables that were initialized twice in a row.
STDMETHODIMP BaseFrameEncode::WritePixels(....)
{
result = S_OK;
....
result = factory->CreateBitmapFromMemory(....);

}
V519 The 'result' variable is assigned values twice successively. Perhaps this is a mistake. Check lines:
269, 279. baseencoder.cpp 279
When the variables are initialized further after several lines of code, we can easily understand, why the
person made a mistake. Sometimes such strings are written successively:
STDMETHODIMP AitFrameEncode::Commit()
{
HRESULT result = E_UNEXPECTED;
result = BaseFrameEncode::Commit();
....
}
V519 The 'result' variable is assigned values twice successively. Perhaps this is a mistake. Check lines:
320, 321. aitencoder.cpp 321
Conclusion
There is a point of view, that C# is less subject to errors than C++, and in some cases it is really so. But an
interesting fact is that the majority of errors aren't in specific constructions, but in simple expressions.
For example, in the condition of the if statement. Static code analyzer PVS-Studio for C, C++ and C#, will
allow you to control the code quality, and will do its best to safeguard you from the fatal errors that can
get to your users.

Source code of WPF samples by Microsoft was checked

Recommended

More Related Content

What's hot (20)

Viewers also liked (17)

Similar to Source code of WPF samples by Microsoft was checked (20)

Recently uploaded (20)

Source code of WPF samples by Microsoft was checked