Skip to content

Commit e9fdbb6

Browse files
committed
fix: address multiple critical issues and refactor appearance generator
- Fix quadraticCurveTo to properly convert quadratic to cubic Bezier curves with current-point tracking - Make ContentTokenizer lenient on unexpected delimiters (warns instead of throwing) - Split appearance-generator.ts (1600+ lines) into smaller modules: appearance-utils, text-appearance, button-appearance, choice-appearance - Fix memory leak in PdfName/PdfRef static caches using LRU cache with bounded size (10k/20k entries) - Add validation for XRef stream field widths to prevent integer overflow - Use multiplication instead of bitwise shift for large field values - Cache TextDecoder instance in token-reader.ts hot paths - Document PdfStream data state machine and getEncodedData() behavior
1 parent 6a5a988 commit e9fdbb6

22 files changed

+2637
-1622
lines changed

src/api/drawing/path-builder.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,36 @@ describe("PathBuilder", () => {
4444
expect(content).toContain("10 20 30 40 50 60 c");
4545
});
4646

47+
it("quadraticCurveTo converts to cubic Bezier correctly", () => {
48+
const { builder, appendContent } = createBuilder();
49+
50+
// Start at (0, 0), control point at (50, 100), end at (100, 0)
51+
builder.moveTo(0, 0).quadraticCurveTo(50, 100, 100, 0).stroke();
52+
53+
const content = appendContent.mock.calls[0][0];
54+
// Quadratic to cubic conversion:
55+
// CP1 = P0 + 2/3 * (QCP - P0) = (0,0) + 2/3 * (50,100) = (33.333, 66.667)
56+
// CP2 = P + 2/3 * (QCP - P) = (100,0) + 2/3 * (50-100, 100-0) = (100,0) + 2/3 * (-50, 100) = (66.667, 66.667)
57+
expect(content).toContain("0 0 m");
58+
// Check for curve operator with approximately correct values
59+
expect(content).toMatch(/33\.333.*66\.666.*66\.666.*66\.666.*100 0 c/);
60+
});
61+
62+
it("quadraticCurveTo tracks current point correctly", () => {
63+
const { builder, appendContent } = createBuilder();
64+
65+
// Chain two quadratic curves - the second should use end of first as start
66+
builder
67+
.moveTo(0, 0)
68+
.quadraticCurveTo(25, 50, 50, 0) // First curve: (0,0) -> (50,0)
69+
.quadraticCurveTo(75, 50, 100, 0) // Second curve: (50,0) -> (100,0)
70+
.stroke();
71+
72+
const content = appendContent.mock.calls[0][0];
73+
// Should have two curve operators
74+
expect((content.match(/ c\n/g) || []).length).toBe(2);
75+
});
76+
4777
it("close adds close-path operator", () => {
4878
const { builder, appendContent } = createBuilder();
4979

src/api/drawing/path-builder.ts

Lines changed: 57 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,14 @@ export class PathBuilder {
4747
private readonly appendContent: ContentAppender;
4848
private readonly registerGraphicsState: GraphicsStateRegistrar;
4949

50+
/** Current point for quadratic-to-cubic conversion */
51+
private currentX = 0;
52+
private currentY = 0;
53+
54+
/** Start point of current subpath (for close operations) */
55+
private subpathStartX = 0;
56+
private subpathStartY = 0;
57+
5058
constructor(appendContent: ContentAppender, registerGraphicsState: GraphicsStateRegistrar) {
5159
this.appendContent = appendContent;
5260
this.registerGraphicsState = registerGraphicsState;
@@ -62,6 +70,11 @@ export class PathBuilder {
6270
moveTo(x: number, y: number): this {
6371
this.pathOps.push(moveTo(x, y));
6472

73+
this.currentX = x;
74+
this.currentY = y;
75+
this.subpathStartX = x;
76+
this.subpathStartY = y;
77+
6578
return this;
6679
}
6780

@@ -71,6 +84,9 @@ export class PathBuilder {
7184
lineTo(x: number, y: number): this {
7285
this.pathOps.push(lineTo(x, y));
7386

87+
this.currentX = x;
88+
this.currentY = y;
89+
7490
return this;
7591
}
7692

@@ -87,27 +103,41 @@ export class PathBuilder {
87103
curveTo(cp1x: number, cp1y: number, cp2x: number, cp2y: number, x: number, y: number): this {
88104
this.pathOps.push(curveTo(cp1x, cp1y, cp2x, cp2y, x, y));
89105

106+
this.currentX = x;
107+
this.currentY = y;
108+
90109
return this;
91110
}
92111

93112
/**
94113
* Draw a quadratic Bezier curve from the current point to (x, y).
95114
*
96-
* Converted internally to a cubic Bezier curve.
115+
* Converted internally to a cubic Bezier curve using the standard
116+
* quadratic-to-cubic conversion formula:
117+
* - CP1 = P0 + 2/3 * (QCP - P0)
118+
* - CP2 = P + 2/3 * (QCP - P)
119+
*
120+
* Where P0 is the current point, QCP is the quadratic control point,
121+
* and P is the end point.
97122
*
98123
* @param cpx - Control point X
99124
* @param cpy - Control point Y
100125
* @param x - End point X
101126
* @param y - End point Y
102127
*/
103128
quadraticCurveTo(cpx: number, cpy: number, x: number, y: number): this {
104-
// Quadratic to cubic conversion requires current point
105-
// For simplicity, we approximate by using the control point for both control points
106-
// This is a rough approximation - proper conversion needs current point tracking
107-
// cp1 = p0 + 2/3 * (cp - p0)
108-
// cp2 = p + 2/3 * (cp - p)
109-
// For now, use the control point as both (less accurate but functional)
110-
this.pathOps.push(curveTo(cpx, cpy, cpx, cpy, x, y));
129+
// Quadratic to cubic conversion:
130+
// CP1 = P0 + 2/3 * (QCP - P0) = P0 * 1/3 + QCP * 2/3
131+
// CP2 = P + 2/3 * (QCP - P) = P * 1/3 + QCP * 2/3
132+
const cp1x = this.currentX + (2 / 3) * (cpx - this.currentX);
133+
const cp1y = this.currentY + (2 / 3) * (cpy - this.currentY);
134+
const cp2x = x + (2 / 3) * (cpx - x);
135+
const cp2y = y + (2 / 3) * (cpy - y);
136+
137+
this.pathOps.push(curveTo(cp1x, cp1y, cp2x, cp2y, x, y));
138+
139+
this.currentX = x;
140+
this.currentY = y;
111141

112142
return this;
113143
}
@@ -117,6 +147,9 @@ export class PathBuilder {
117147
*/
118148
close(): this {
119149
this.pathOps.push(closePath());
150+
// After close, the current point returns to the subpath start
151+
this.currentX = this.subpathStartX;
152+
this.currentY = this.subpathStartY;
120153

121154
return this;
122155
}
@@ -129,13 +162,12 @@ export class PathBuilder {
129162
* Add a rectangle to the current path.
130163
*/
131164
rectangle(x: number, y: number, width: number, height: number): this {
132-
this.pathOps.push(moveTo(x, y));
133-
this.pathOps.push(lineTo(x + width, y));
134-
this.pathOps.push(lineTo(x + width, y + height));
135-
this.pathOps.push(lineTo(x, y + height));
136-
this.pathOps.push(closePath());
137-
138-
return this;
165+
// Use fluent methods to ensure current point is tracked
166+
return this.moveTo(x, y)
167+
.lineTo(x + width, y)
168+
.lineTo(x + width, y + height)
169+
.lineTo(x, y + height)
170+
.close();
139171
}
140172

141173
/**
@@ -152,14 +184,13 @@ export class PathBuilder {
152184
const kx = rx * KAPPA;
153185
const ky = ry * KAPPA;
154186

155-
this.pathOps.push(moveTo(cx - rx, cy));
156-
this.pathOps.push(curveTo(cx - rx, cy + ky, cx - kx, cy + ry, cx, cy + ry));
157-
this.pathOps.push(curveTo(cx + kx, cy + ry, cx + rx, cy + ky, cx + rx, cy));
158-
this.pathOps.push(curveTo(cx + rx, cy - ky, cx + kx, cy - ry, cx, cy - ry));
159-
this.pathOps.push(curveTo(cx - kx, cy - ry, cx - rx, cy - ky, cx - rx, cy));
160-
this.pathOps.push(closePath());
161-
162-
return this;
187+
// Use fluent methods to ensure current point is tracked
188+
return this.moveTo(cx - rx, cy)
189+
.curveTo(cx - rx, cy + ky, cx - kx, cy + ry, cx, cy + ry)
190+
.curveTo(cx + kx, cy + ry, cx + rx, cy + ky, cx + rx, cy)
191+
.curveTo(cx + rx, cy - ky, cx + kx, cy - ry, cx, cy - ry)
192+
.curveTo(cx - kx, cy - ry, cx - rx, cy - ky, cx - rx, cy)
193+
.close();
163194
}
164195

165196
// ─────────────────────────────────────────────────────────────────────────────
@@ -175,6 +206,7 @@ export class PathBuilder {
175206
borderColor: options.borderColor ?? options.color,
176207
color: undefined, // Don't fill
177208
};
209+
178210
this.paint(effectiveOptions);
179211
}
180212

@@ -186,6 +218,7 @@ export class PathBuilder {
186218
...options,
187219
borderColor: undefined, // Don't stroke
188220
};
221+
189222
this.paint(effectiveOptions);
190223
}
191224

@@ -253,6 +286,7 @@ export class PathBuilder {
253286
*/
254287
private emitOps(ops: Operator[]): void {
255288
const content = ops.map(op => op.toString()).join("\n");
289+
256290
this.appendContent(content);
257291
}
258292
}

src/content/parsing/content-tokenizer.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,24 @@ describe("ContentTokenizer", () => {
216216
});
217217
});
218218

219+
describe("lenient parsing", () => {
220+
it("skips unexpected closing bracket and continues", () => {
221+
const tokenizer = new ContentTokenizer(encode("] q Q"));
222+
223+
// Should skip the ] and return q
224+
expect(tokenizer.nextToken()).toEqual({ type: "operator", name: "q" });
225+
expect(tokenizer.nextToken()).toEqual({ type: "operator", name: "Q" });
226+
});
227+
228+
it("skips unexpected >> and continues", () => {
229+
const tokenizer = new ContentTokenizer(encode(">> 42 m"));
230+
231+
// Should skip the >> and return 42
232+
expect(tokenizer.nextToken()).toEqual({ type: "number", value: 42 });
233+
expect(tokenizer.nextToken()).toEqual({ type: "operator", name: "m" });
234+
});
235+
});
236+
219237
describe("peek and eof", () => {
220238
it("peek returns same token until consumed", () => {
221239
const tokenizer = new ContentTokenizer(encode("q Q"));

src/content/parsing/content-tokenizer.ts

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -90,16 +90,23 @@ export class ContentTokenizer {
9090
}
9191

9292
private readNext(): ContentOrOperatorToken | null {
93-
const token = this.reader.nextToken();
93+
while (true) {
94+
const token = this.reader.nextToken();
9495

95-
if (token.type === "eof") {
96-
return null;
97-
}
96+
if (token.type === "eof") {
97+
return null;
98+
}
9899

99-
return this.convertToken(token);
100+
const result = this.convertToken(token);
101+
102+
// If convertToken returns null (skipped token), try the next one
103+
if (result !== null) {
104+
return result;
105+
}
106+
}
100107
}
101108

102-
private convertToken(token: Token): ContentOrOperatorToken {
109+
private convertToken(token: Token): ContentOrOperatorToken | null {
103110
switch (token.type) {
104111
case "number":
105112
return { type: "number", value: token.value };
@@ -142,7 +149,7 @@ export class ContentTokenizer {
142149
return { type: "operator", name: value };
143150
}
144151

145-
private handleDelimiter(value: string): ContentToken {
152+
private handleDelimiter(value: string): ContentToken | null {
146153
if (value === "[") {
147154
return this.readArray();
148155
}
@@ -152,7 +159,11 @@ export class ContentTokenizer {
152159
}
153160

154161
// Unexpected delimiters (], >>) - shouldn't happen in valid content stream
155-
throw new Error(`Unexpected delimiter: ${value}`);
162+
// Be lenient: warn and skip the token rather than throwing
163+
console.warn(
164+
`ContentTokenizer: Unexpected delimiter '${value}' at position ${this.scanner.position} - skipping`,
165+
);
166+
return null;
156167
}
157168

158169
private readArray(): ArrayToken {

0 commit comments

Comments
 (0)