Skip to content

Refactor DesignerSerializationManager dictionary usage to use TryGetValue #8702

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 31, 2023
Merged

Refactor DesignerSerializationManager dictionary usage to use TryGetValue #8702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc494b2
Refactor direct access to dictionaries to use TryGetValue
elachlan Feb 26, 2023
77edcb2
Add test for GetSerializer failure when serializers direct access is …
elachlan Mar 28, 2023
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
28 changes: 10 additions & 18 deletions ...rms.Design/src/System/ComponentModel/Design/Serialization/DesignerSerializationManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ private void CheckNoSession()
}

/// <summary>
/// Used to verify that there is an open session. If there isn't, this method throws.
/// Used to verify that there is an open session. If there isn't, this method throws.
/// </summary>
private void CheckSession()
{
Expand Down Expand Up @@ -210,10 +210,7 @@ protected virtual object CreateInstance(Type type, ICollection? arguments, strin
// If we have been asked to recycle instances, look in our nametable and container first for an object matching this name and type. If we find it, we will use it.
if (RecycleInstances && name is not null)
{
if (instancesByName is not null)
{
instance = instancesByName[name];
}
instancesByName?.TryGetValue(name, out instance);

if (instance is null && addToContainer && Container is not null)
{
Expand Down Expand Up @@ -420,9 +417,8 @@ public IDisposable CreateSession()
{
if (serializers is not null)
{
// I don't double hash here. It will be a very rare day where multiple types of serializers will be used in the same scheme. We still handle it, but we just don't cache.
serializer = serializers[objectType];
if (serializer is not null && !serializerType.IsAssignableFrom(serializer.GetType()))
// I don't double hash here. It will be a very rare day where multiple types of serializers will be used in the same scheme. We still handle it, but we just don't cache.
if (serializers.TryGetValue(objectType, out serializer) && !serializerType.IsAssignableFrom(serializer.GetType()))
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for objectType to map to a null value in serializers? If so, we should check if serializer is null before calling serializer.GetType()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We check objectType for null just above on line 416.

Copy link

Choose a reason for hiding this comment

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

I think @lonitra meant, if serializers contained an entry in which the key (i.e. objectType) is not null but the value (i.e. serializer) is null, that could cause a NullReferenceException in serializer.GetType(). Although the code checks objectType is not null, that doesn't help against this risk.

Anyway, it won't happen, because the only code that adds anything to serializers is here and it does not add null values:

winforms/src/System.Windows.Forms.Design/src/System/ComponentModel/Design/Serialization/DesignerSerializationManager.cs

Lines 462 to 466 in 77edcb2

if (serializer is not null && session is not null)
{
serializers ??= new();
serializers[objectType] = serializer;
}

{
serializer = null;
}
Expand Down Expand Up @@ -804,26 +800,22 @@ object IDesignerSerializationManager.CreateInstance(Type type, ICollection? argu
/// </summary>
string? IDesignerSerializationManager.GetName(object value)
{
string? name = null;
ArgumentNullException.ThrowIfNull(value);

CheckSession();
// Check our local nametable first
if (namesByInstance is not null)
if (namesByInstance is not null && namesByInstance.TryGetValue(value, out string? name))
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here. If value can map to a null value in namesByInstance, we should check if name is not null before returning it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

namesByInstance/instancesByName do not accept a nullable value.

{
name = namesByInstance[value];
return name;
}

if (name is null && value is IComponent component)
if (value is not IComponent component)
{
ISite? site = component.Site;
if (site is not null)
{
name = site is INestedSite nestedSite ? nestedSite.FullName : site.Name;
}
return null;
}

return name;
ISite? site = component.Site;
return site is null ? null : site is INestedSite nestedSite ? nestedSite.FullName : site.Name;
}

/// <summary>
Expand Down
3 changes: 3 additions & 0 deletions ...UnitTests/System/ComponentModel/Design/Serialization/DesignerSerializationManagerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,6 +1394,9 @@ public void DesignerSerializationManager_IDesignerSerializationManagerGetSeriali

// Call again.
Assert.Null(iManager.GetSerializer(objectType, typeof(int)));

// Call unrelated object type
Assert.Null(iManager.GetSerializer(typeof(object), typeof(int)));
}

[Fact]
Expand Down