[Marouane-elfadli]

What does this change?

Fixes #1175 by addressing PDF processing issues affecting conversion, merging and splitting. Adds regression tests using a generated PDF containing both defects, covering defect detection, repair, merge behavior and the encrypted-file bail-out.

Type of change

• Bug fix
• New feature or improvement
• New format or engine
• Translation
• Documentation
• Build, CI or dependencies

How was it tested?

• dotnet format --verify-no-changes
• dotnet test
• Tried it in the app (what you converted or clicked, and on which OS):

Screenshots (UI changes)

Not applicable.

Checklist

• New UI strings are in all src/Filee.App/Assets/i18n/*.json files
• New third-party code or binaries are listed in THIRD-PARTY-NOTICES.md
• The PR does one thing (unrelated changes go in a separate PR)

[KnifeLemon]

Thanks for digging into this, and for tracking down the file's damage! The merge test is a good one: it fails on main and passes here. I ran the PDF from #1175 through main and through this branch, and I'd like to change how the repair works before merging.

What the file really has: PDFsharp only trips over the extra >> in the two bookmark objects (7 and 11). The lone CR after stream is harmless.

• With only the >> fixed and the CRs left in, everything works on main.
• With only the CRs fixed, it fails the same way.
• If I overwrite the extra >> with two spaces and leave the xref table alone, merge, split, compress, rotate and pages per sheet all work on main.

Must fix

1. Try PDFsharp first, and drop the lone CR check

OpenTolerant repairs any file where HasLoneCrStream is true before PDFsharp has a go, and that check also looks inside stream data. These all work on main and break here:

• A healthy 3-page PDF with an uncompressed attached PDF that has a lone CR: Open returns 1 page, and split, rotate and remove metadata write that 1 page and report success.
• A lone-CR PDF with a bookmark titled Part A >> Part B: every PDFsharp operation fails, because DropSurplusClose counts the >> in the title.
• A lone-CR PDF with endobj 3 0 obj on one line: 0 pages, merge throws a NullReferenceException, and split "completes" with no output.

2. Don't rebuild the xref table

Scanning for N G obj finds objects inside streams too, and the last copy wins. A file with the extra >> plus an attached PDF used to fail with a clear error. Now it writes the attachment's page instead of the document's 3 pages, reported as success. A wrong result is worse than an error.

Overwrite the extra >> with spaces instead: the length stays the same, so the file's own xref stays valid. Please only look at objects the xref table lists, and don't count >> inside strings.

3. Don't read the whole file for a healthy PDF

File.ReadAllBytes runs on every open. A 2.2 GB PDF that merges on main now fails with "The file is too long", and a healthy 300 MB merge goes from about 660 MB to about 960 MB peak memory. Calling PdfReader.Open(path, mode) first solves both. Read the bytes only after it fails.

4. Pages per sheet still fails on this file

PdfSheets.cs:112 calls XPdfForm.FromFile(input) directly, so it needs the repaired file too.

5. Tests

• Please add a test that a healthy file stays as it is, for example the attached-PDF case above, with its page count checked.
• Please replace the encrypted test with a real password-protected PDF (PDFsharp can write one in the test), since the current one only checks for the text /Encrypt.

6. Drop the change to tests/Filee.Core.Tests/MergeImagesTests.cs

It isn't part of this fix, and main already has that test sorted.

Smaller

A truncated PDF now says "Object reference not set to an instance of an object" where main says "The StartXRef table could not be found". That goes away with point 2.

Recovering PDFs with a broken xref table would be useful on its own, so that's welcome as a separate PR once it can skip stream contents.

Could you also tick the test boxes in the PR body once you've run dotnet format and dotnet test, and try the #1175 file in the app? Thanks!

[KnifeLemon]

Thanks for reworking this, @Marouane-elfadli, it's a big improvement! I ran the PDF from #1175 through main and through this branch: merge, split, page range, compress, remove metadata, rotate and pages per sheet all work now, and the pages come out pixel-identical to a copy with the extra >> overwritten by hand.

These all behave the same as main now:

• a healthy file with an attached PDF;
• a Part A >> Part B bookmark;
• endobj 3 0 obj on one line;
• a truncated PDF, with the same message.

The 2.2 GB PDF merges again, too.

A few things are left.

Must fix

1. Pages per sheet on a password-protected PDF (PdfSheets.cs:104, PdfSharpConverter.cs:423-427)

MendedCopy lets PDFsharp's own error through, so the message becomes "A password is required to open the PDF document." instead of the one the other PDF tools give. Please add a test with a real password-protected PDF.

2. The temporary copy (PdfSharpConverter.cs:441)

It goes straight into the system temp folder. On Linux and macOS that's /tmp, which every user shares, and WorkFolders exists to keep users' files out of there.

XPdfForm.FromStream (public in PDFsharp 6.2.4) can read the mended bytes directly. So Open can hand back the bytes, with no temp file, and healthy files skip the second parse.

3. Slow failures (PdfRepair.cs:126, :134)

A PDF that fails for another reason now pays for the repair, and Repair can't be cancelled.

Case	main	This branch
100 MB, 20,000 objects with no endobj	427 ms	205 s
250 MB unreadable file, pages per sheet	55 MB peak memory	1,557 MB peak memory

Each object searches to the end of the file. Please put a limit on both searches in SurplusClose. With a limit, the first case takes about 3 s. Taking a CancellationToken would help too.

4. Drop the change to tests/Filee.Core.Tests/MergeImagesTests.cs

It's still there (lines 6 and 205). It isn't part of this fix.

Smaller

A damaged bookmark with "stream" in its title, such as "Upstream plan", stays unrepaired, because the check at PdfRepair.cs:135 also reads inside strings. Please look for stream outside strings only, and add a test with that title.

Before merging

• PR body: every test box is still unticked. Please tick them once you've run dotnet format and dotnet test, and try the #1175 file in the app.
• Commits: I'll squash these when merging, so you don't need to tidy them.

[KnifeLemon]

Thanks for this round, @Marouane-elfadli! You'd fixed nearly everything before I got back to it, and the byte-based rewrite goes further than what I asked for. I ran it again:

Check	Result
The PDF from #1175	opens, pages per sheet works, the source file is unchanged
96 MB, 20,000 objects with no endobj	fails in 0.4 s (was 205 s)
Pages per sheet on a password-protected PDF	gives the same message as the other PDF tools

I pushed one commit on top of yours, so please pull before you push again.

What it changes

• Stream data could be overwritten. IsStreamKeyword also wanted a line end right after stream. With stream (a space before the line end) or data on the same line, the data got counted and a >> inside it was replaced with spaces. It now only looks at the byte before the keyword, and there's a test for both forms.
• Red CI. The new optional CancellationToken made the existing test calls trigger xUnit1051, which stops dotnet format. They pass TestContext.Current.CancellationToken now.
• MergeImagesTests.cs is back to what main has.

There's nothing left for you to do unless CI finds something. I'll squash this when it merges, as I said. I haven't tried it in the app itself, only through the converter and the tests.