Disposing page stream eagerly

[pchaurasia14]

Fixes #7567

Description

Stream is not explicitly disposed when retrieving page contents. This causes stream to stay open even when the usage is over. The next iteration of the same code path then throws the error since it sees that the underlying stream is already open.

Customer Impact

Unable to search when using docviewer control

Regression

Don't know.

Testing

Under testing.

Risk

Low. Since the scope of the stream is anyways local.

Microsoft Reviewers: Open in CodeFlow

[pchaurasia14]

@bgrainger - This may also fix #6842 (since the top call stacks look similar)

[bgrainger]

This may also fix #6842 (since the top call stacks look similar)

I don't think it will, because my call stack didn't go through FixedFindEngine._GetPageString (where you made the fix).

I wonder if there's a lower-level place to fix this that can address #6842, #7567, #3206 (and maybe others?) all at once.

[miloush]

Risk
Low. Since the scope of the stream is anyways local.

The risk of regression is that you will dispose a stream that is not yours and that someone else expects to reuse. For example cached site of origin streams could be worth checking.

[pchaurasia14]

GetPageStream relays to WpfWebRequestHelper.CreateRequestAndGetResponseStream which in turn is responsible for getting the response stream of the document. This is implemented via PackWebRequest and PackWebResponse abstractions.

PackWebResponse internally handles the actual reading of response from the file and then caching it via CachedResponse. However, the cached response is not reused due to new instantiation of PackWebResponse for every incoming request.

Without the using statement, the CachedResponse objects that are going out of scope do NOT release the underlying stream causing the subsequent opening of the stream to throw an exception.

@miloush - Please let me know if I have missed anything.

[miloush]

@pchaurasia14 yes but where does the PackWebResponse resp. CachedResponse get the stream from? From PackagePart. And we have various types of package parts. The exception that is fixed here is coming from ZipPackagePart, but there is also ResourcePart, ContentFilePart and SiteOfOriginPart and the document can be loaded from those as well. I suggested to check whether these would be happy with having the returned stream disposed, most notably ensure they do not cache it and return the same instance next time you ask. The ResourcePart and ContentFilePart trivially return new stream each time. The SiteOfOriginPart has _cacheStream so that's what raised the flag for me. I have now inspected it and this cache is only triggered by reading PackagePart.ContentType and cleared next time the stream is requested:

wpf/src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/AppModel/SiteOfOriginPart.cs

Lines 105 to 121 in e9ea2a3

	// If GetContentTypeCore is called before GetStream()
	// then we need to retrieve the stream to get the mime type.
	// That stream is then stored as _cacheStream and returned
	// the next time GetStreamCore() is called.
	if (_cacheStream != null)
	{
	#if DEBUG
	if (SiteOfOriginContainer._traceSwitch.Enabled)
	System.Diagnostics.Trace.TraceInformation(
	DateTime.Now.ToLongTimeString() + " " + DateTime.Now.Millisecond + " " +
	Environment.CurrentManagedThreadId +
	"SiteOfOriginPart: Using Cached stream");
	#endif
	Stream temp = _cacheStream;
	_cacheStream = null;
	return temp;
	}

So you would have to ask for content type, dispose the stream and then ask for the stream again with new search. However, I don't see how that could possibly happen, because the cached stream is not accessible without clearing the cache (and PackagePart.ContentType caches the content type anyway). Thus I have satisfied myself that this PR looks OK.

[pchaurasia14]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/wpf/actions/runs/4851890830

[pchaurasia14]

/backport to release/7.0

[github-actions]

@pchaurasia14 an error occurred while backporting to release/6.0, please check the run log for details!

Error: @pchaurasia14 is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=pchaurasia14

[github-actions]

Started backporting to release/7.0: https://github.com/dotnet/wpf/actions/runs/4851892749

[github-actions]

@pchaurasia14 an error occurred while backporting to release/7.0, please check the run log for details!

Error: @pchaurasia14 is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=pchaurasia14

[dipeshmsft]

/backport to release/7.0

[github-actions]

Started backporting to release/7.0: https://github.com/dotnet/wpf/actions/runs/4861072541

[dipeshmsft]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/wpf/actions/runs/4861082815