Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- [SIL.Windows.Forms.Clearshare] New ILicenseWithImage interface handles "GetImage" method for Winforms-dependent licenses, implemented in CreativeCommonsLicense and CustomLicense, and formerly included in LicenseInfo.
- [SIL.Core.Clearshare] New tests MetadataBareTests are based on previous MetadataTests in SIL.Windows.Forms.Clearshare. The tests were updated to use ImageSharp instead of Winforms for handling images.
- [SIL.Core.Desktop] Added a constant (kBrowserCompatibleUserAgent) to RobustNetworkOperation: a browser-like User Agent string that can be used when making HTTP requests to strict servers.
- [SIL.Core] Added an Exception property to NonFatalErrorReportExpected to return the previous reported non-fatal exception.
- [SIL.Media] Added a static PlaybackErrorMessage property to AudioFactory and a public const, kDefaultPlaybackErrorMessage, that will be used as the default message if the client does not set PlaybackErrorMessage.

### Fixed
- [SIL.DictionaryServices] Fix memory leak in LiftWriter
- [SIL.WritingSystems] Fix IetfLanguageTag.GetGeneralCode to handle cases when zh-CN or zh-TW is a prefix and not the whole string.
- [SIL.WritingSystems] More fixes to consistently use 繁体中文 and 简体中文 for Traditional and Simplified Chinese native language names, and Chinese (Traditional) and Chinese (Simplified) for their English names.
- [SIL.Windows.Forms] Prevent BetterLabel from responding to OnTextChanged when it has been disposed.
- [SIL.Windows.Forms] Prevent ContributorsListControl.GetContributionFromRow from throwing an exception when the DataGridView has no valid rows selected.
- [SIL.Media] BREAKING CHANGE (subtle and unlikely): WindowsAudioSession.OnPlaybackStopped now passes itself as the sender instead of a private implementation object, making the event arguments correct.

### Changed
- [SIL.Windows.Forms.i18n, SIL.Core.Desktop.i18n] BREAKING CHANGE: Move L10NSharpLocalizer from Windows.Forms to Core.Desktop so it can be accessed without Winforms dependency.
Expand All @@ -51,6 +54,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- [SIL.Windows.Forms] BREAKING CHANGE: Removed optional moreSelected parameter from ToolStripExtensions.InitializeWithAvailableUILocales method. This parameter was no longer being used. Clients that want to have a More menu item that performs a custom action will now need to add it themselves.
- [SIL.Windows.Forms] BREAKING CHANGE: LocalizationIncompleteDlg's EmailAddressForLocalizationRequests is no longer autopopulated from LocalizationManager. A new optional constructor parameter, emailAddressForLocalizationRequests, can be used instead. If not supplied, the "More information" controls will be hidden.
- [SIL.Core.Desktop] Added an optional userAgentHeader parameter to DoHttpGetAndGetProxyInfo to allow a client to mimic a real browser if necessary.
- [SIL.Media] ISimpleAudioWithEvents now extends ISimpleAudioSession. Technically a breaking change, but the only class in this library that implements it already implemented ISimpleAudioSession. Any custom implementations by clients would very likely have implemented both as well.
- [SIL.Core] Changed the Message property on NonFatalErrorReportExpected (presumably intended for use only in tests) to return the message of the previous reported non-fatal exception if no ordninary non-fatal message has been reported.
- [SIL.Media] In the event of an audio playback error in Windows, the non-fatal exception reported will also include an accompanying (localizable/customizable) user-friendly message.

### Removed
- [SIL.Windows.Forms] In .NET 8 builds, removed Scanner and Camera options from the Image Toolbox.
Expand Down
2 changes: 2 additions & 0 deletions Palaso.sln.DotSettings
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,12 @@
<s:Boolean x:Key="/Default/UserDictionary/Words/=taskbar/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=triglot/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Unfilter/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=unmanifested/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=vcodec/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=versifications/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=vshost/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Wasta/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Wiktionary/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Winforms/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=xkal/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=xklavier/@EntryIndexedValue">True</s:Boolean></wpf:ResourceDictionary>
129 changes: 59 additions & 70 deletions SIL.Core/Reporting/ErrorReport.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Reflection;
using System.Runtime.InteropServices;
using System.Text;
using JetBrains.Annotations;
using Microsoft.Win32;
using SIL.IO;
using SIL.PlatformUtilities;
Expand All @@ -29,10 +30,12 @@ public interface IErrorReporter
void NotifyUserOfProblem(IRepeatNoticePolicy policy, Exception exception, string message);

/// <summary>
/// Notify the user of the <paramref name="message"/>, if <paramref name="policy"/> permits. Customize the alternate button label. Wait for user input. Return button clicked to the user.
/// Notify the user of the <paramref name="message"/>, if <paramref name="policy"/> permits.
/// Customize the alternate button label. Wait for user input. Return button clicked to the user.
/// </summary>
/// <remarks>The method overload should block and wait for the user to press the required button</remarks>
/// <returns>The method should return <paramref name="resultIfAlternateButtonPressed"/> if the alternate button is clicked, ErrorResult.OK otherwise</returns>
/// <returns>The method should return <paramref name="resultIfAlternateButtonPressed"/> if
/// the alternate button is clicked; <see cref="ErrorResult.OK"/> otherwise</returns>
ErrorResult NotifyUserOfProblem(IRepeatNoticePolicy policy,
string alternateButton1Label,
ErrorResult resultIfAlternateButtonPressed,
Expand Down Expand Up @@ -144,7 +147,7 @@ public static void SetErrorReporter(IErrorReporter reporter)
protected static string s_emailAddress = null;
protected static string s_emailSubject = "Exception Report";
private static Action<Exception, string> s_onShowDetails;
private static bool s_justRecordNonFatalMessagesForTesting=false;
private static bool s_justRecordNonFatalMessagesForTesting = false;
private static string s_previousNonFatalMessage;
private static Exception s_previousNonFatalException;

Expand Down Expand Up @@ -177,23 +180,22 @@ public static string GetVersionForErrorReporting()
return ReflectionHelper.LongVersionNumberString;
}

[PublicAPI]
public static object GetAssemblyAttribute(Type attributeType)
{
Assembly assembly = Assembly.GetEntryAssembly();
if (assembly != null)
{
object[] attributes =
assembly.GetCustomAttributes(attributeType, false);
if (attributes != null && attributes.Length > 0)
{
object[] attributes = assembly.GetCustomAttributes(attributeType, false);
if (attributes.Length > 0)
return attributes[0];
}
}
return null;
}

public static string VersionNumberString => ReflectionHelper.VersionNumberString;

[PublicAPI]
public static string UserFriendlyVersionString
{
get
Expand All @@ -220,40 +222,37 @@ public static string DotNet4VersionFromWindowsRegistry()
if (!Platform.IsWindows)
return string.Empty;

using (var key = Registry.LocalMachine.OpenSubKey(@"SOFTWARE\Microsoft\NET Framework Setup\NDP\v4\Full"))
{
return key == null ? "(unable to determine)" : $"{key.GetValue("Version")} ({key.GetValue("Release")})";
}
using var key = Registry.LocalMachine.OpenSubKey(@"SOFTWARE\Microsoft\NET Framework Setup\NDP\v4\Full");
return key == null ? "(unable to determine)" : $"{key.GetValue("Version")} ({key.GetValue("Release")})";
#endif
}

/// <summary>
/// use this in unit tests to cleanly check that a message would have been shown.
/// E.g. using (new SIL.Reporting.ErrorReport.NonFatalErrorReportExpected()) {...}
/// E.g., using (new SIL.Reporting.ErrorReport.NonFatalErrorReportExpected()) {...}
/// </summary>
public class NonFatalErrorReportExpected :IDisposable
{
private readonly bool previousJustRecordNonFatalMessagesForTesting;
private readonly bool _previousJustRecordNonFatalMessagesForTesting;
public NonFatalErrorReportExpected()
{
previousJustRecordNonFatalMessagesForTesting = s_justRecordNonFatalMessagesForTesting;
_previousJustRecordNonFatalMessagesForTesting = s_justRecordNonFatalMessagesForTesting;
s_justRecordNonFatalMessagesForTesting = true;
s_previousNonFatalMessage = null;//this is a static, so a previous unit test could have filled it with something (yuck)
}
Comment on lines 237 to 242

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 NonFatalErrorReportExpected constructor does not reset s_previousNonFatalException, allowing stale exceptions to mask missing errors

The NonFatalErrorReportExpected constructor resets s_previousNonFatalMessage = null but does NOT reset s_previousNonFatalException. The Dispose() method checks if (s_previousNonFatalException == null && s_previousNonFatalMessage == null) to verify that a non-fatal error was actually generated during the using block.

Root Cause and Impact

If a previous test (outside this using scope) set s_previousNonFatalException — for example via ReportNonFatalException or ReportNonFatalExceptionWithMessage at SIL.Core/Reporting/ErrorReport.cs:565-566 — that stale value will persist into the new NonFatalErrorReportExpected scope. The Dispose() check at line 246 will see s_previousNonFatalException is non-null and will NOT throw the expected "wasn't generated" exception, even though no non-fatal error was actually reported during the current using block.

Similarly, the new Exception property at line 255 would return a stale exception from a prior test, and the Message fallback (s_previousNonFatalException?.Message) could return a misleading message.

The comment on line 241 even acknowledges the issue for the message field: "this is a static, so a previous unit test could have filled it with something (yuck)" — the same logic should apply to s_previousNonFatalException.

Impact: Tests using NonFatalErrorReportExpected may silently pass when they should fail, hiding real test regressions.

Suggested change
public NonFatalErrorReportExpected()
{
previousJustRecordNonFatalMessagesForTesting = s_justRecordNonFatalMessagesForTesting;
_previousJustRecordNonFatalMessagesForTesting = s_justRecordNonFatalMessagesForTesting;
s_justRecordNonFatalMessagesForTesting = true;
s_previousNonFatalMessage = null;//this is a static, so a previous unit test could have filled it with something (yuck)
}
public NonFatalErrorReportExpected()
{
_previousJustRecordNonFatalMessagesForTesting = s_justRecordNonFatalMessagesForTesting;
s_justRecordNonFatalMessagesForTesting = true;
s_previousNonFatalMessage = null;//this is a static, so a previous unit test could have filled it with something (yuck)
s_previousNonFatalException = null;
}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👎

public void Dispose()
{
s_justRecordNonFatalMessagesForTesting= previousJustRecordNonFatalMessagesForTesting;
s_justRecordNonFatalMessagesForTesting = _previousJustRecordNonFatalMessagesForTesting;
if (s_previousNonFatalException == null && s_previousNonFatalMessage == null)
throw new Exception("Non Fatal Error Report was expected but wasn't generated.");
s_previousNonFatalMessage = null;
s_previousNonFatalException = null;
}
/// <summary>
/// use this to check the actual contents of the message that was triggered
/// </summary>
public string Message
{
get { return s_previousNonFatalMessage; }
}
public string Message => s_previousNonFatalMessage ?? s_previousNonFatalException?.Message;
public Exception Exception => s_previousNonFatalException;
}

/// <summary>
Expand All @@ -280,28 +279,25 @@ public void Dispose()
/// <summary>
/// use this to check the actual contents of the message that was triggered
/// </summary>
public string Message
{
get { return s_previousNonFatalMessage; }
}
public string Message => s_previousNonFatalMessage;
}

/// <summary>
/// set this property if you want the dialog to offer to create an e-mail message.
/// </summary>
public static string EmailAddress
{
set { s_emailAddress = value; }
get { return s_emailAddress; }
set => s_emailAddress = value;
get => s_emailAddress;
}

/// <summary>
/// set this property if you want something other than the default e-mail subject
/// </summary>
public static string EmailSubject
{
set { s_emailSubject = value; }
get { return s_emailSubject; }
set => s_emailSubject = value;
get => s_emailSubject;
}

/// <summary>
Expand All @@ -317,14 +313,8 @@ public static string EmailSubject
/// </summary>
public static Action<Exception, string> OnShowDetails
{
get
{
return s_onShowDetails ?? (s_onShowDetails = ReportNonFatalExceptionWithMessage);
}
set
{
s_onShowDetails = value;
}
get => s_onShowDetails ?? (s_onShowDetails = ReportNonFatalExceptionWithMessage);
set => s_onShowDetails = value;
}

/// ------------------------------------------------------------------------------------
Expand Down Expand Up @@ -358,7 +348,7 @@ public static void AddStandardProperties()
else
AddProperty("DotNetVersion", DotNet4VersionFromWindowsRegistry());
// https://docs.microsoft.com/en-us/dotnet/api/system.environment.version?view=netframework-4.8 warns that using the Version property is
// no longer recommended for .NET Framework 4.5 or later. I'm leaving it in in hope that it may once again become useful. Note that, as
// no longer recommended for .NET Framework 4.5 or later. I'm leaving it in with the hope that it may once again become useful. Note that, as
// of .NET Framework 4.8, it is *not* marked as deprecated.
// https://www.mono-project.com/docs/about-mono/versioning/#framework-versioning (accessed 2020-04-02) states that
// "Mono's System.Environment.Version property [. . .] should be the same version number that .NET would return."
Expand Down Expand Up @@ -400,7 +390,7 @@ class Version
private readonly PlatformID _platform;
private readonly int _major;
private readonly int _minor;
public string Label { get; private set; }
public string Label { get; }

public Version(PlatformID platform, int major, int minor, string label)
{
Expand All @@ -421,9 +411,11 @@ public static string GetOperatingSystemLabel()
{
if (Environment.OSVersion.Platform == PlatformID.Unix)
{
var startInfo = new ProcessStartInfo("lsb_release", "-si -sr -sc");
startInfo.RedirectStandardOutput = true;
startInfo.UseShellExecute = false;
var startInfo = new ProcessStartInfo("lsb_release", "-si -sr -sc")
{
RedirectStandardOutput = true,
UseShellExecute = false
};
var proc = new Process { StartInfo = startInfo };
try
{
Expand All @@ -434,7 +426,7 @@ public static string GetOperatingSystemLabel()
var si = proc.StandardOutput.ReadLine();
var sr = proc.StandardOutput.ReadLine();
var sc = proc.StandardOutput.ReadLine();
return String.Format("{0} {1} {2}", si, sr, sc);
return $"{si} {sr} {sc}";
}
}
catch (Exception)
Expand All @@ -445,17 +437,19 @@ public static string GetOperatingSystemLabel()
else
{
// https://msdn.microsoft.com/en-us/library/windows/desktop/ms724832%28v=vs.85%29.aspx
var list = new List<Version>();
list.Add(new Version(PlatformID.Win32NT, 5, 0, "Windows 2000"));
list.Add(new Version(PlatformID.Win32NT, 5, 1, "Windows XP"));
list.Add(new Version(PlatformID.Win32NT, 6, 0, "Vista"));
list.Add(new Version(PlatformID.Win32NT, 6, 1, "Windows 7"));
// After Windows 8 the Environment.OSVersion started misreporting information unless
// your app has a manifest which says it supports the OS it is running on. This is not
// helpful if someone starts using an app built before the OS is released. Anything that
// reports its self as Windows 8 is suspect, and must get the version info another way.
list.Add(new Version(PlatformID.Win32NT, 6, 3, "Windows 8.1"));
list.Add(new Version(PlatformID.Win32NT, 10, 0, "Windows 10"));
var list = new List<Version>
{
new Version(PlatformID.Win32NT, 5, 0, "Windows 2000"),
new Version(PlatformID.Win32NT, 5, 1, "Windows XP"),
new Version(PlatformID.Win32NT, 6, 0, "Vista"),
new Version(PlatformID.Win32NT, 6, 1, "Windows 7"),
// After Windows 8 the Environment.OSVersion started misreporting information unless
// your app has a manifest which says it supports the OS it is running on. This is not
// helpful if someone starts using an app built before the OS is released. Anything that
// reports its self as Windows 8 is suspect, and must get the version info another way.
new Version(PlatformID.Win32NT, 6, 3, "Windows 8.1"),
new Version(PlatformID.Win32NT, 10, 0, "Windows 10")
};

foreach (var version in list)
{
Expand Down Expand Up @@ -529,9 +523,7 @@ public static ErrorResult NotifyUserOfProblem(IRepeatNoticePolicy policy,
{
var message = string.Format(messageFmt, args);
return NotifyUserOfProblemWrapper(message, null, () =>
{
return _errorReporter.NotifyUserOfProblem(policy, alternateButton1Label, resultIfAlternateButtonPressed, message);
});
_errorReporter.NotifyUserOfProblem(policy, alternateButton1Label, resultIfAlternateButtonPressed, message));
}

/// <summary>
Expand All @@ -540,7 +532,7 @@ public static ErrorResult NotifyUserOfProblem(IRepeatNoticePolicy policy,
/// * Checks if s_justRecordNonFatalMessagesForTesting is true and if so, skips calling the notification/reporting logic.
/// * Calls UsageReporter.ReportException if exception is non-null
/// </summary>
/// <param name="message">The message that would be showed to the user</param>
/// <param name="message">The message that would be shown to the user</param>
/// <param name="exception">The associated exception. May be null.</param>
/// <param name="notifyUserOfProblem">A delegate that takes no arguments.
/// It should call (via a closure) the actual core logic of having the _errorReporter notify the user of the problem.
Expand All @@ -551,6 +543,7 @@ protected static ErrorResult NotifyUserOfProblemWrapper(string message, Exceptio
if (s_justRecordNonFatalMessagesForTesting)
{
s_previousNonFatalMessage = message;
s_previousNonFatalException = exception;
return ErrorResult.OK;
}

Expand All @@ -564,7 +557,7 @@ protected static ErrorResult NotifyUserOfProblemWrapper(string message, Exceptio
}

/// <summary>
/// Bring up a "yellow box" that let's them send in a report, then return to the program.
/// Bring up a "yellow box" that lets them send in a report, then return to the program.
/// This version assumes the message has already been formatted with any arguments.
/// </summary>
public static void ReportNonFatalExceptionWithMessage(Exception error, string message)
Expand All @@ -575,8 +568,9 @@ public static void ReportNonFatalExceptionWithMessage(Exception error, string me
}

/// <summary>
/// Bring up a "yellow box" that let's them send in a report, then return to the program.
/// Bring up a "yellow box" that lets them send in a report, then return to the program.
/// </summary>
[PublicAPI]
public static void ReportNonFatalExceptionWithMessage(Exception error, string message, params object[] args)
{
s_previousNonFatalMessage = message;
Expand All @@ -585,16 +579,17 @@ public static void ReportNonFatalExceptionWithMessage(Exception error, string me
}

/// <summary>
/// Bring up a "yellow box" that let's them send in a report, then return to the program.
/// Bring up a "yellow box" that lets them send in a report, then return to the program.
/// Use this one only when you don't have an exception (else you're not reporting the exception's message)
/// </summary>
public static void ReportNonFatalMessageWithStackTrace(string message, params object[] args)
{
s_previousNonFatalMessage = message;
_errorReporter.ReportNonFatalMessageWithStackTrace(message, args);
}

/// <summary>
/// Bring up a "green box" that let's them send in a report, then exit.
/// Bring up a "green box" that lets them send in a report, then exit.
/// </summary>
public static void ReportFatalMessageWithStackTrace(string message, params object[] args)
{
Expand All @@ -616,7 +611,7 @@ public static void ReportNonFatalException(Exception exception, IRepeatNoticePol
{
if (s_justRecordNonFatalMessagesForTesting)
{
ErrorReport.s_previousNonFatalException = exception;
s_previousNonFatalException = exception;
return;
}
_errorReporter.ReportNonFatalException(exception, policy);
Expand Down Expand Up @@ -664,10 +659,7 @@ public bool ShouldShowMessage(string message)
return true;
}

public string ReoccurrenceMessage
{
get { return string.Empty; }
}
public string ReoccurrenceMessage => string.Empty;
}

public class ShowOncePerSessionBasedOnExactMessagePolicy :IRepeatNoticePolicy
Expand All @@ -687,10 +679,7 @@ public bool ShouldShowMessage(string message)
return true;
}

public string ReoccurrenceMessage
{
get { return "This message will not be shown again this session."; }
}
public string ReoccurrenceMessage => "This message will not be shown again this session.";

public static void Reset()
{
Expand Down
Loading
Loading