Fix a hole in a sparse array being skipped when serialising

[MahathirMohammadShuvo]

Narrowed to the convert change, per review. lib/tree.js is untouched — I
checked, and tree output is byte-identical to master. The
addNamedEmbeddedFile throw will be a separate PR.

The bug

.map does not call back for holes, so a sparse array lost the entry entirely
and every later index shifted down. A destination array is
[page /XYZ left top zoom], and building one by index is an ordinary thing to
write when left and top are optional:

const dest = new Array(5);
dest[0] = page;
dest[1] = 'XYZ';
dest[4] = zoom;          // left and top deliberately unset

master:      [7 0 R /XYZ   3]     <- three entries; zoom has slid into left
this branch: [7 0 R /XYZ null null 3]

const dest = [page, 'XYZ']; dest[4] = zoom; does the same thing.

pdf.js reads the damage rather than rejecting it, so the reader follows a
destination the caller never asked for:

	master	this branch
getAnnotations()[0].dest	[ref, /XYZ, 72, 3] — four entries	[ref, /XYZ, 72, null, 3]

null is legal in exactly those positions — Table 151 defines it as "retain the
current value" for left, top and zoom, and lib/mixins/annotations.js
already writes [page, 'XYZ', null, null, null] for goTo.

The fix

An indexed loop rather than Array.from, which also visits holes but defers to a
subclass's custom iterator. A hole becomes null, which is what #1769 already
does for an explicit undefined entry — this just makes the two agree.

3 tests added, 2 of which fail against master. The third covers the dictionary
guard from #1769 and is a regression guard, not evidence for this change.

Full unit suite passes, 394/394.

[blikblum]

The entry is now dropped. Not written as null either - the file would parse,
but a reader would still surface an entry pointing at nothing. An explicit null
a caller passed is left unchanged, so documents that already work keep their
bytes.

Not sure the best behavior here:

• Do not add file at all silently
• Throw ?
• This PR approach?

[MahathirMohammadShuvo]

Throw, I think - and there is a precedent in the same feature area that settles it for me:

doc.file(undefined, { name: 'x.txt' });    // Error: No src specified
doc.addNamedEmbeddedFile('x', undefined);  // silently accepted

attachments.js:31 already refuses a missing src. addNamedEmbeddedFile is the lower-level route to the same thing and does the opposite.

Of the three, silent drop is the one I would argue against hardest. The caller asked for an attachment under a name and gets back a valid PDF with nothing attached and no signal - harder to debug than a stack trace, and there is no legitimate reason to name an attachment with no file behind it.

If you want the throw I would put it in addNamedEmbeddedFile, not PDFTree.add - add is shared with /Dests, /JavaScript and ParentTree, and those only ever receive values the library builds itself.

I would keep the toString() filter either way, but as a base-class invariant rather than as the answer to this question: whatever a future caller does, a tree should never emit something that is not a PDF object. With the throw in place it becomes unreachable from the public API, which is the right shape for it.

Technically breaking, though Unreleased already carries three [BREAKING CHANGE] entries and anyone hitting this today is shipping a PDF with a phantom attachment.

Happy to push that if you want it - small change on top of this.

[blikblum]

py to push that if you want it - small change on top of this.

It should be a different PR

The convert change is worth anyway

The PDFTree.toString change AFAIK is triggered only synthetically so i am less inclined to merge

[MahathirMohammadShuvo]

Done — narrowed to the convert change and retitled. lib/tree.js is back to
master and I verified the tree output is byte-identical, so nothing from the part
you declined is left in here. The throw will be its own PR.

On "triggered only synthetically" — fair for the tree case, and I should have led
with a better example for the array one. [1, , 2] is contrived, but this is not:

const dest = new Array(5);
dest[0] = page;
dest[1] = 'XYZ';
dest[4] = zoom;          // left and top deliberately unset

master:      [7 0 R /XYZ   3]     <- three entries; zoom has slid into left
this branch: [7 0 R /XYZ null null 3]

const dest = [page, 'XYZ']; dest[4] = zoom; gives the same thing. Pre-allocating
a fixed five-slot destination array and filling the slots you have is an ordinary
way to write it, and pdf.js reads the shifted array without complaint, so the
reader just follows the wrong destination.

Down to 3 tests, 2 red on master, 394/394.

[blikblum]

Many thanks