Close Stream when creating an ImageSource from a Uri #6843
+1
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
During XAML serialization, an ImageSource may first be loaded by ImageSourceConverter.ConvertFrom. This calls BitmapDecoder.CreateFromUriOrStream with a pack URI, opens a Stream, and keeps it open. This will eventually be closed by the BitmapDecoder finalizer (see comments in that method). Subsequently the same pack URI is opened by ImageSourceTypeConverter.ConvertTo. This throws an IOException: 'Entries cannot be opened multiple times in Update mode.'. This occurs because the same ZipArchiveEntry is being opened a second time, while the first stream is still open. By specifying BitmapCacheOption.OnLoad here, BitmapDecoder.Initialize will eagerly close the stream, which allows the ZipArchiveEntry to be opened a second time.
ghost
assigned 
ghost
requested review from
|
The call stack for the first time the pack URI is opened (this is the stack addressed by this PR): The call stack for the second time, which throws the IOException (see #6842): |
I am not sure about this one. Changing the behavior here has a massive impact on all WPF applications because it's used in hot path area. Testing it on a single application for a single purpose does not seem enough. Also you say its a regression from .NET Framework. I've decompiled the .NET Framework GAC assembly and it also uses Decompilation taken from PresentationCore.dll Version 4.8.9032.0:
|
|
Changed my risk assessment to "Unknown" to better align with the two following sentences. I will posit that leaving Streams (a managed object) to be closed by a finalizer is strongly against .NET best practices, and should be avoided if possible. |
|
Ultimately I suspect this is related to the switch from WindowsBase to System.IO.Packaging for |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines failed to run 1 pipeline(s). |
|
This PR should also fix #9418. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #6842
Description
During XAML serialization, an
ImageSourcemay first be loaded byImageSourceConverter.ConvertFrom. This callsBitmapDecoder.CreateFromUriOrStreamwith a pack URI, opens aStream, and keeps it open. This will eventually be closed by theBitmapDecoderfinalizer (see comments in that method).wpf/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/Imaging/BitmapDecoder.cs
Lines 215 to 221 in c8742b5
Subsequently the same pack URI is opened by
ImageSourceTypeConverter.ConvertTo. This throws anIOException: 'Entries cannot be opened multiple times in Update mode.'. This occurs because the sameZipArchiveEntryis being opened a second time, while the first stream is still open.By specifying
BitmapCacheOption.OnLoadhere,BitmapDecoder.Initializewill eagerly close the stream, which allows theZipArchiveEntryto be opened a second time.Customer Impact
Fixes an exception that prevents certain documents (presumably ones containing images, or ImageBrushes?) from printing.
Regression
Yes, regression from .NET Framework 4.8.
Testing
Tested in real-world application by using Faithlife.WPF: https://github.com/Faithlife/wpf/blob/stable/README.md#faithlifewpf
Risk
Unknown. May improve system resource usage by closing streams early instead of letting them be closed by a finalizer? OTOH, may perform unnecessary image decoding work up-front?
Microsoft Reviewers: Open in CodeFlow