Skip to content
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

Jump to bottom

fixing DrtXpsApi failures #1301

Merged
merged 3 commits into from
Jul 18, 2019
Merged

fixing DrtXpsApi failures #1301

merged 3 commits into from
Jul 18, 2019

Conversation

stevenbrix
Copy link
Contributor

@stevenbrix stevenbrix commented Jul 17, 2019 •
edited
Loading

Fixes failures in DrtXpsApi as outlined in #1054

The implementation in CoreFx is more strict on ensuring that callers properly close streams before they are re-opened in update mode. this prevents multiple different streams from being updated at the same time since it uncompresses the data and stores it in memory

The callstacks we get is this:

>	System.IO.Compression.dll!System.IO.Compression.ZipArchiveEntry.OpenInUpdateMode() Line 700	C#
 	System.IO.Compression.dll!System.IO.Compression.ZipArchiveEntry.Open() Line 299	C#
 	System.IO.Packaging.dll!System.IO.Packaging.ZipStreamManager.Open(System.IO.Compression.ZipArchiveEntry zipArchiveEntry, System.IO.FileMode streamFileMode, System.IO.FileAccess streamFileAccess)	Unknown
 	System.IO.Packaging.dll!System.IO.Packaging.ZipPackagePart.GetStreamCore(System.IO.FileMode streamFileMode, System.IO.FileAccess streamFileAccess)	Unknown
 	System.IO.Packaging.dll!System.IO.Packaging.PackagePart.GetStream(System.IO.FileMode mode, System.IO.FileAccess access)	Unknown
 	ReachFramework.dll!System.Windows.Xps.Packaging.XpsDocument.IsSignable.get()	Unknown
 	ReachFramework.dll!System.Windows.Xps.Packaging.XpsDocument.SignDigitally(System.Security.Cryptography.X509Certificates.X509Certificate certificate, bool embedCertificate, System.Windows.Xps.Packaging.XpsDigSigPartAlteringRestrictions restrictions, string signatureId, bool testIsSignable)	Unknown
 	ReachFramework.dll!System.Windows.Xps.Packaging.XpsDocument.SignDigitally(System.Security.Cryptography.X509Certificates.X509Certificate certificate, bool embedCertificate, System.Windows.Xps.Packaging.XpsDigSigPartAlteringRestrictions restrictions)	Unknown
 	DRTXpsApi.dll!DRT.DigSignVerifyTestSuite.SignPackage() Line 1244	C#
 	DRTXpsApi.dll!DRT.DrtBase.RunNextTest() Line 2060	C#
 	DRTXpsApi.dll!DRT.DrtBase.RunNextTestOperation(object arg) Line 2004	C#

Here is the code that throws the exception in CoreFx:
https://github.com/dotnet/corefx/blob/master/src/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs#L697

@ghost ghost requested review from vatsan-madhavan, rladuca and ryalanms July 17, 2019 21:49
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jul 17, 2019
@ghost ghost requested a review from SamBent July 17, 2019 21:49
stevenbrix
stevenbrix commented Jul 17, 2019
View reviewed changes
src/Microsoft.DotNet.Wpf/src/ReachFramework/Packaging/XpsFixedDocumentReaderWriter.cs Outdated
}
}

// Licensed to the .NET Foundation under one or more agreements.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

weird, viewing the diff in VS looks normal, not sure why this looks like the whole doc changed? maybe line-endings?

Copy link
Member

Choose a reason for hiding this comment

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

What's the actual change to look at?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's just wrapping a call to PackagePart.GetStream in a using statement

Here are some pics of the diff in VS that lets you see the changes properly:
image

image

@stevenbrix stevenbrix changed the title fixing xps code fixing DrtXpsApi failures Jul 17, 2019
@vatsan-madhavan
Copy link
Member

Is there a corresponding change needed in dotnet-wpf-test ?

@vatsan-madhavan
Copy link
Member

lgtm

rladuca
rladuca reviewed Jul 17, 2019
View reviewed changes
src/Microsoft.DotNet.Wpf/src/ReachFramework/Packaging/XpsFixedDocumentReaderWriter.cs Outdated
/// <summary>
/// This method closes streams and frees memory for this
/// fixed document.
///
Copy link
Member

Choose a reason for hiding this comment

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

While we're swapping to usings, maybe we should add one for the stream and writer here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea.

@stevenbrix
Copy link
Contributor Author

Is there a corresponding change needed in dotnet-wpf-test ?

Yeah, see this commit here:
https://dev.azure.com/dnceng/internal/_git/dotnet-wpf-test/commit/cb5fee5a70d02034786d3629a1668e816b7995bc

rladuca
rladuca approved these changes Jul 18, 2019
View reviewed changes
Copy link
Member

@rladuca rladuca left a comment

Choose a reason for hiding this comment

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

Seems good to me.

Can you just be sure the line endings in XpsFixedDocumentReaderWriter.cs are correct? If you turn off ignore whitespace in VS that lights up just like GitHub, so you have a line ending transformation there.

@stevenbrix
Copy link
Contributor Author

Seems good to me.

Can you just be sure the line endings in XpsFixedDocumentReaderWriter.cs are correct? If you turn off ignore whitespace in VS that lights up just like GitHub, so you have a line ending transformation there.

Ahh yeah ok i think i remember now. The line endings were LF, but when i went to stage the file with git add i got the following error:

PS D:\dev\src\dotnet\wpf\src\Microsoft.DotNet.Wpf\src\ReachFramework\Packaging> git add .\XpsFixedDocumentReaderWriter.cs
fatal: LF would be replaced by CRLF in src/Microsoft.DotNet.Wpf/src/ReachFramework/Packaging/XpsFixedDocumentReaderWriter.cs

so i had to change the line endings to CRLF

@stevenbrix stevenbrix merged commit c155bce into master Jul 18, 2019
@grubioe grubioe mentioned this pull request Jul 29, 2019
Closed
@vatsan-madhavan vatsan-madhavan deleted the dev/stevenbrix/fixXps branch August 23, 2019 20:11
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vatsan-madhavan vatsan-madhavan vatsan-madhavan left review comments

@rladuca rladuca rladuca approved these changes

@ryalanms ryalanms Awaiting requested review from ryalanms

@SamBent SamBent Awaiting requested review from SamBent

Assignees
No one assigned
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stevenbrix @vatsan-madhavan @rladuca