Home .NET Checking the source code of Microsoft’s WPF Samples

Checking the source code of Microsoft’s WPF Samples

by admin

To popularize PVS-Studiocode analyzer which can check C# projects in addition to C++, we decided to check the source code of WPF samples offered by Microsoft.
Checking the source code of Microsoft's WPF Samples
With the release of Windows Vista, a new system for building beautiful client applications, the Windows Presentation Foundation (WPF), was introduced.This graphical subsystem has been part of the .NET Framework since version 3.0. It uses markup language XAML and has replaced outdated WinForms. In my opinion, the main drawback of WinForms was that it did all the drawing on the CPU. WPF was more logical and gave its components to DirectX. Now WPF had almost superseded WinForms and allows to make universal interfaces for three platforms (PC, XBOXOne, Winphone).
To analyze the source code of the WPF examples from Microsoft ( source code ) a static code analyzer was used PVS-Studio version 6.05.
An interesting thing about this Solution is that along with C# projects it contains projects written in C++. I learned about this feature only from the list of errors found by PVS-Studio. The PVS-Studio plugin for Visual Studio analyzed and printed both C# and C++ code into the messages without any additional manipulations from the user.
Checking the source code of Microsoft's WPF Samples
Figure 1. The PVS-Studio window displays warnings concerning both C# and C++ code of the checked project simultaneously (click on the figure to enlarge).

C# Errors

1. Errors in making instructional conditions if
Errors when comparing something to something is a very common problem among programmers. Let’s go over them.
There are two exactly the same conditions in this source code :

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
What exactly they wanted to compare in the second case is not entirely clear, but clearly not what they wrote.
We are writing checks on null into conditions, which is how we protect the program from possible contingencies. We can say that most of the conditions if consists precisely in checks on null of any fields or variables. However, sometimes these checks are not only unnecessary, but may also cause a logical error:

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 can assume that the author wanted to check that values contains more than 0 elements, but I never could think of a situation where Split will return an empty array. In any case, no matter how you spin it, checking for null was completely unnecessary here.
As I said, the project contains code from C++ and C# diagnostics. I got the impression that it was a C++ programmer who had a hand in the next code.

private void LoadNotes(){var fs = new FileStream("NotesFile", FileMode.Open);if (fs != null){....}

V3022 Expression ‘fs != null’ is always true. MainWindow.cs 66
In fact, even in C++ code this way is wrong, while in C# it looks "strange". Why it is wrong to write this way in C++ code is described in the article " Checking 7-Zip source code with PVS-Studio " well, we’ll keep on parsing the C# code.
We didn’t get far from such situations. In Solution we found two almost identical functions (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
Well it won’t be writer zero link…
Throwing an exception is not a bad solution. The main thing is not to make a mistake when to throw an exception, because you can give the user of your application a very unpleasant impression when the program crashes on nothing.

protected Size SizeParser(string o){....if (sizeString.Length== 0 || sizeString.Length != 2)throw new ApplicationException("Error: a size shouldcontain two doublethat seperated by a spaceor ', ' 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, in this case, checking for 0 is unnecessary. sizeString.Length for an inequality of 2.
In long bodies of instruction if is very difficult to detect nonsensical eye checks.

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
This is not a problem for the analyzer. By the way, thanks to our favorite copy-paste, the error was found in two projects at once : HtmlToXamlDemo and DocumentSerialization
Of course, meaningless checks occur not only in long functions, but also within two or three lines.

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 principle, it’s not a big deal, but when followed by the instruction if nested in another if and on and on… I want to get rid of meaningless checks at least to improve readability of the code which is actually read much more often than written.
Let’s take a little break. Let’s take a look at this unusual function I came across. The body of the function is given in full :

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
We also found many (over 60) comparisons of real numbers ( double ) with a specific 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…

The article is not enough to give all the lines. We have this warning on level 3 because its relevance depends strongly on the specifics of the program. In the case where the numbers are made mathematical calculations (value manipulation), it is not guaranteed that we get a specific number: -1, 0, 1. A deviation of 0.00000000001 will already lead to an incorrect comparison result. However, if the logic of a program involves writing to real numbers ( double ) of discrete values, then such checks are not an error.
2. Errors in initialization and variable assignment
A function is a great thing that not only allows you to remove repetitive code, but also makes it easier to understand the section of code where that function is used. It’s especially important that a function performs exactly the task described in its name and call signature. But this is not always the case. Take, for instance, the following code fragment. The function is given in its entirety for better understanding of the situation.

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 to be opened is overwritten just before its first use fileName = dialog.FileName Yes, in this case, the dialog box will open and the user’s file will be selected, but then why do we need a parameter that is not actually used?
Lack of time and copy-paste sometimes give rise to very strange constructions :

public MailSettingsDialog(){...._timerClock = _timerClock = new DispatcherTimer();....}

V3005 The ‘_timerClock’ variable is assigned to itself. MailSettingsDialog.cs 56
It may not seem like a terrible typo, but it makes you wonder if we’re "writing a second time" in the right place. Well, like here :

private void DumpAllClipboardContentsInternal(){....if (dataObject == null){clipboardInfo.Text =clipboardInfo.Text ="Can't access clipboard now!\n\nPlease click Dump All ClipboardContents button again.";}else{....}

V3005 The ‘clipboardInfo.Text’ variable is assigned to itself. MainWindow.cs 204
In general, the code abounds with strange assignments :

private void DoParse(stringcommandLine){....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 the type string
Or here is a further code example which is more incorrect. In it you counted so much something for some reason, and then you recalculated it again and 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 many, many more such or similar examples of meaningless primary assignment :

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. Check lines: 140, 138. MainWindow.cs 140
  • V3008 The ‘matrixResult’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 351, 349. MainWindow.cs 351
  • V3008 The ‘matrixResult’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 369, 367. MainWindow.cs 369
  • V3008 The ‘pointResult’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 480, 478. MainWindow.cs 480
  • 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 every example, especially since there are many more important and serious mistakes waiting for us.
3. A pair of heterogeneous errors
When throwing exceptions, it is important to save call stack to understand later from logs "what the user actually dropped". Not everyone knows how to do it correctly.

public static object InvokePropertyOrMethod(....){try{....}catch(MissingMethodException e){....throwe;}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 you pass the exception up the function call stack in the throw e; , we will lose the call stack that was there before the exception was caught in the catch To save the entire call stack and keep it going, we just need to write one word throw in the block catch and that’s it.
Sometimes the checks are unnecessary, and sometimes they are just not 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 namefontFamily =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 here that fontFamily.Length is greater than 1, and in consequence of this subtraction from fontFamily.Length number 2, we can get a value less than 0. And this function throws an exception in such cases ArgumentOutOfRangeException
It would have been safer to write a check :

if (fontFamilyList == null fontFamily.Length > 1)

4. WPF error
DependencyProperty is one of the great things about WPF. Creating properties that can notify the developer about changes out of the box is incredibly handy. But the important thing is not to mix up the signature to describe them, and it’s especially important to keep this in mind when showing examples, because that’s what people are guided by.

public double Radius{get { return (double) GetValue(RadiusProperty); }set { SetValue(RadiusProperty, value); }}public static readonly DependencyPropertyRadiusProperty = 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 the dependency property does not match the name of the wrapper property to access with the DependencyProperty from the code. This variant causes big problems when working from XAML markup. WPF allows a simple property to be accessed from XAML Radius and read value from it, but changes to that property will not be picked up from XAML.
In fact, in PVS-Studio has a number of diagnostics to detect errors in the signature of the DependencyProperty creation [ 3044 , 3045 , 3046 , 3047 , 3048 , 3049 ]. Most errors of this kind cause the program to crash as soon as you start using a class with these dependency properties. So these diagnostics are designed to save you from searching for and analyzing long pieces of signatures, especially after copying. This, of course, requires regular code checks, not just analysis of the final version of the program.
Let’s look at another interesting trigger. In this case, the new V3095 diagnostic is triggered. This diagnostic shows the places where we first address a variable and then check its equality to 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 ternary operator condition, we check that the variable htmlChildNode can be null In this case, the variable htmlChildNode is nothing more than a reference to the variable htmlLiElement But exactly to the variable htmlLiElement we accessed without checking for null As a result, we either have code that will never execute, or we get an exception altogether NullReferenceException at line htmlLiElement.LocalName.ToLower()
In addition to the described errors, the diagnostic number V3072 which is designed to detect the presence of fields with the type that implements the interface IDisposable , but the class itself, where the fields are declared, has no such implementation.

internal class Email{private readonly SmtpClient _client;....}

V3072 The ‘Email’ class containing IDisposablemembers does not itself implement IDisposable. Inspect: _client. Email.cs 15
With IDisposable has always been tight. From critical errors, as a consequence of its improper use, of course, often saves Finalize , well, at least in standard classes. Programmers often hammer, forget, omit or just ignore fields with the type that implements this interface. It is difficult for an outsider to justify such code or acknowledge it to be incorrect, but there are some patterns you should pay attention to. In this Solution, there are a lot of such things, too.

  • 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
  • V3072 The ‘MainWindow’ class containing IDisposable members does not itself implement 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
  • V3072 The ‘MainWindow’ class containing IDisposable members does not itself implement IDisposable. Inspect: _store. MainWindow.cs 20
  • V3072 The ‘MainWindow’ class containing IDisposable members does not itself implement IDisposable. Inspect: _customCursor. MainWindow.cs 14
  • V3072 The ‘MainWindow’ class containing IDisposable members does not itself implement IDisposable. Inspect: _speechSynthesizer. MainWindow.cs 14

C++ Errors

1. Errors in making instructional conditions if
It was certainly a revelation for me to find C++ projects in this Solution, but nevertheless, mistakes are mistakes and let’s look at them.
As in C#, let’s start with different comparisons and immediately look at the very C++ error I talked about in the C# block above.

STDMETHOD(CreateInstance)(....){....T *obj= newT();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 memory allocation error. classfactory.h 76
If the operator new cannot allocate memory, then according to C++ standard std::bad_alloc() exception is generated. Thus it makes no sense to check for null, because the pointer obj will never be equal to NULL If memory cannot be allocated, an exception occurs, which can be handled at a higher level, and the equality check NULL can be simply removed. If exceptions in the application are undesirable, you can use the operator new which does not generate exceptions ( T *obj = new (std::nothrow) T() ):in this case you can check the return value for zero. There are 4 more similar checks in 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 using the ‘new’ operator. The exception will be generated in the case of memory allocation error. aitencoder.cpp 157
  • 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. aitencoder.cpp 221
  • V668 There is no sense in testing the ‘bytes’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. aitencoder.cpp 275

Superfluous conditions are relevant in 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 do not know that the following two operations on Nullable types are equivalent :

  • _isInDesignMode != null
  • _isInDesignMode.HasValue

And do similar checks :

if (_isInDesignMode != null _isInDesignMode.HasValue)

In C++, they like to nonsensically check a pointer for null before freeing the memory allocated to the address 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. Logical error
The following code presents a very interesting logical comparison situation, although at first glance you wouldn’t think so :

STDMETHODIMP AitDecoder::QueryCapability(....){....// If this is our format, we can do everythingif (strcmp(bh.Name, "AIT") == 0){*pCapability =WICBitmapDecoderCapabilityCanDecodeAllImages ||WICBitmapDecoderCapabilityCanDecodeThumbnail ||WICBitmapDecoderCapabilityCanEnumerateMetadata ||WICBitmapDecoderCapabilitySameEncoder;}....}

V560 A part of conditional expression is always true. aitdecoder.cpp 634
The diagnostician thought that part of the expression is always true, and she is right, since the words WICBitmapDecoderCapabilityCanDecodeXXX are simply the values of enum named WICBitmapDecoderCapabilities :

enum WICBitmapDecoderCapabilities{WICBitmapDecoderCapabilitySameEncoder = 0x1, WICBitmapDecoderCapabilityCanDecodeAllImages = 0x2, WICBitmapDecoderCapabilityCanDecodeSomeImages = 0x4, WICBitmapDecoderCapabilityCanEnumerateMetadata = 0x8, WICBitmapDecoderCapabilityCanDecodeThumbnail = 0x10, WICBITMAPDECODERCAPABILITIES_FORCE_DWORD = 0x7fffffff};

As a consequence, it is likely that someone just mixed up symbols and wrote logical OR "|" instead of bitwise OR "||". Unlike the C# compiler, C++ easily swallowed this.
3. Errors in initialization and variable assignment
Of course, after refactoring, there are often two variables that have been 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 variables are initialized after a few lines of code, it’s easy to see why the person missed, but sometimes these two lines go in a row :

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 an opinion that C# code is less error-prone than C++; in some cases this is true. However, an interesting fact is that the bulk of errors are not in specific constructs, but in simple expressions. For example, in the conditions of an instruction if The static code analyzer for PVS-Studio C, C++ and C# will let you control the quality of your code and do its best to prevent fatal errors from reaching your user.
Checking the source code of Microsoft's WPF Samples

If you want to share this article with an English-speaking audience, please use the translation link : Alferov Vitalii. Source code of WPF samples by Microsoft was checked
Read the article and have a question? Often the same questions are asked to our articles. We’ve compiled the answers to them here : Answers to questions from readers of articles about PVS-Studio, version 2015 Please review the list.

You may also like