aglinxinyuan commented on code in PR #7999:
URL: https://github.com/apache/texera/pull/7999#discussion_r3866919028
##########
frontend/src/jsdom-svg-polyfill.ts:
##########
@@ -192,6 +192,45 @@ G.ResizeObserver ??= class {
disconnect(): void {}
};
+// `window.getComputedStyle(elt, pseudoElt)` and `window.scrollTo` — jsdom
+// implements neither. Both route through its `notImplemented` helper, which
+// emits a `jsdomError` on the virtual console; vitest's jsdom environment
+// forwards that to `console.error`, one full stack trace per call. html2canvas
+// calls both on every render — a `:before` and an `:after` lookup for each
+// cloned node, plus one `scrollTo` per document clone — so the
+// report-generation spec, which drives the real renderer on purpose (`vi.mock`
+// can't reach it under the builder's `isolate: false`), buries the run in
+// traces while all of its tests pass.
+// Dropping `pseudoElt` changes no behaviour: jsdom complains and then ignores
+// the argument, returning the element's own declaration either way. `scrollTo`
+// has nothing to move — jsdom has no layout.
+const jsdomGetComputedStyle = G.getComputedStyle as ((elt: Element,
pseudoElt?: string | null) => unknown) | undefined;
+if (typeof jsdomGetComputedStyle === "function") {
+ const withoutPseudoElement = ((elt: Element) => jsdomGetComputedStyle(elt))
as AnyFn;
+ G.getComputedStyle = withoutPseudoElement;
+ if (G.window) G.window.getComputedStyle = withoutPseudoElement;
+}
+const inertScrollTo: AnyFn = () => undefined;
+G.scrollTo = inertScrollTo;
Review Comment:
These two lines override a call that can't happen. html2canvas has two
`scrollTo` sites: `cloneWindow.scrollTo` (html2canvas.js:5235), which is the
iframe's and is handled by the `contentWindow` patch just below, and
`restoreOwnerScroll` (html2canvas.js:5652), guarded by `x !==
ownerDocument.defaultView.pageXOffset` — under jsdom the saved value and
`pageXOffset` are both 0, so it never fires. Nothing in `src` calls
`window.scrollTo` either.
Deleting 214-215 and running the full suite: 201/201 files pass, zero `Not
implemented: window.scrollTo` traces.
The cost is mostly to the comment's accuracy — "plus one `scrollTo` per
document clone" describes the clone iframe, not this window, so the next reader
has to work out why both patches are here. Dropping these two lines and
pointing the comment at `contentWindow` would settle it.
##########
frontend/src/app/workspace/service/report-generation/report-generation.service.spec.ts:
##########
@@ -330,6 +330,28 @@ describe("ReportGenerationService", () => {
});
describe("generateWorkflowSnapshot", () => {
+ /**
+ * html2canvas clones the whole document, not just the element it is
pointed at, and the
+ * unit-test builder runs spec files with `isolate: false` — so one jsdom
document is shared
+ * by every spec file in a worker, and the renders below drag in whatever
DOM the files
+ * before this one left behind. That is what failed the macOS leg: a clone
that costs ~60ms
+ * against this file's own DOM was measured at 12–37s there, past the 20s
test timeout,
+ * while ubuntu and windows passed. Park the foreign nodes for the
duration of the file and
+ * put them back after, so the render's cost depends only on what these
tests build. The
+ * renders started here outlive the tests that start them, so this has to
span the file
+ * rather than each test.
+ */
+ let parkedNodes: ChildNode[];
+
+ beforeAll(() => {
+ parkedNodes = Array.from(document.body.childNodes);
Review Comment:
This parks `document.body`, but html2canvas clones from `documentElement`
(`this.cloneNode(element.ownerDocument.documentElement, false)`,
html2canvas.js:5212) and `cloneChildNodes` walks every child of `<html>` —
`<head>` included, at three `getComputedStyle` calls per element.
Head accumulates under `isolate: false` the same way body does. Measuring
both at this hook across a full run: body peaks at 463 descendants (which
corroborates your ~475 estimate), head at 185, with 46 of 201 runs above 76. So
parking body takes the larger share but leaves up to ~185 elements — roughly
29% of the worst case — in every clone. Which of the two a given worker carries
depends on file scheduling, which is the shape of an OS-specific flake.
Parking `document.head.childNodes` the same way would close the gap. If
you'd rather leave head alone, the comment above should say the residual is
there, since it currently attributes the whole cost to body.
##########
frontend/src/jsdom-svg-polyfill.ts:
##########
@@ -192,6 +192,45 @@ G.ResizeObserver ??= class {
disconnect(): void {}
};
+// `window.getComputedStyle(elt, pseudoElt)` and `window.scrollTo` — jsdom
+// implements neither. Both route through its `notImplemented` helper, which
+// emits a `jsdomError` on the virtual console; vitest's jsdom environment
+// forwards that to `console.error`, one full stack trace per call. html2canvas
+// calls both on every render — a `:before` and an `:after` lookup for each
+// cloned node, plus one `scrollTo` per document clone — so the
+// report-generation spec, which drives the real renderer on purpose (`vi.mock`
+// can't reach it under the builder's `isolate: false`), buries the run in
+// traces while all of its tests pass.
+// Dropping `pseudoElt` changes no behaviour: jsdom complains and then ignores
+// the argument, returning the element's own declaration either way. `scrollTo`
+// has nothing to move — jsdom has no layout.
+const jsdomGetComputedStyle = G.getComputedStyle as ((elt: Element,
pseudoElt?: string | null) => unknown) | undefined;
Review Comment:
Both patches in this block wrap whatever is already installed rather than
replacing it, so they nest a layer deeper per spec file. Line 207 reads back
the previous run's `withoutPseudoElement`; line 221's
`getOwnPropertyDescriptor` reads back the previous run's patched getter.
`setupFiles` re-runs per spec file and, under `isolate: false`, the window
survives — I tagged each wrapper with its run number and the chain reaches 41
layers on an 8-core box, one per spec file in the worker. Every
`getComputedStyle` call in the suite then traverses 41 nested frames, and every
`iframe.contentWindow` access runs 41 getter bodies, each reassigning
`scrollTo` to a different closure. Fewer cores means fewer workers and more
files each, so the macOS leg is deeper still: the run gets slower the longer it
goes, which cuts against what this PR is for.
This is the accumulation the file already guards against twice for the same
reason — line 33 ("`setupFiles` re-runs per spec file and `module.register`
chains") and lines 292-295 ("attaching fresh `process.on(...)` handlers each
time grows the listener chain") — and Angular's own generated setup file uses
`Symbol.for('@angular/cli/testbed-setup')` for it. A third flag alongside
`CSS_HOOK_FLAG` / `PROCESS_HANDLERS_FLAG` around 207-232 covers both patches at
once.
##########
frontend/src/app/workspace/service/report-generation/report-generation.service.spec.ts:
##########
@@ -330,6 +330,28 @@ describe("ReportGenerationService", () => {
});
describe("generateWorkflowSnapshot", () => {
+ /**
+ * html2canvas clones the whole document, not just the element it is
pointed at, and the
+ * unit-test builder runs spec files with `isolate: false` — so one jsdom
document is shared
+ * by every spec file in a worker, and the renders below drag in whatever
DOM the files
+ * before this one left behind. That is what failed the macOS leg: a clone
that costs ~60ms
+ * against this file's own DOM was measured at 12–37s there, past the 20s
test timeout,
+ * while ubuntu and windows passed. Park the foreign nodes for the
duration of the file and
+ * put them back after, so the render's cost depends only on what these
tests build. The
+ * renders started here outlive the tests that start them, so this has to
span the file
+ * rather than each test.
+ */
+ let parkedNodes: ChildNode[];
+
+ beforeAll(() => {
+ parkedNodes = Array.from(document.body.childNodes);
+ parkedNodes.forEach(node => node.remove());
+ });
+
+ afterAll(() => {
+ parkedNodes.forEach(node => document.body.appendChild(node));
Review Comment:
The foreign nodes go back, but not this file's own debris. html2canvas
appends its clone container to `ownerDocument.body` (html2canvas.js:5579) and
calls `DocumentCloner.destroy(container)` only on the success path
(html2canvas.js:7794 — no try/finally), so every render that throws leaks its
iframe, and five of this file's six renders fail deliberately.
Measured after this hook, both isolated and in a full run: three
`iframe.html2canvas-container` elements left in body, each a live jsdom
browsing context holding a cloned document, inherited by every later spec file
in the worker. Body also ends up dirtier than the hook found it, since the
restored nodes land after the leaked iframes.
The leak is pre-existing, but this is now the hook where cleaning it belongs:
```ts
document.body.querySelectorAll("iframe.html2canvas-container").forEach(node
=> node.remove());
```
##########
frontend/src/app/workspace/service/report-generation/report-generation.service.spec.ts:
##########
@@ -330,6 +330,28 @@ describe("ReportGenerationService", () => {
});
describe("generateWorkflowSnapshot", () => {
+ /**
+ * html2canvas clones the whole document, not just the element it is
pointed at, and the
+ * unit-test builder runs spec files with `isolate: false` — so one jsdom
document is shared
+ * by every spec file in a worker, and the renders below drag in whatever
DOM the files
+ * before this one left behind. That is what failed the macOS leg: a clone
that costs ~60ms
+ * against this file's own DOM was measured at 12–37s there, past the 20s
test timeout,
+ * while ubuntu and windows passed. Park the foreign nodes for the
duration of the file and
+ * put them back after, so the render's cost depends only on what these
tests build. The
+ * renders started here outlive the tests that start them, so this has to
span the file
Review Comment:
`beforeAll`/`afterAll` here scope to `generateWorkflowSnapshot`, not the
file. The earlier describes run before this and aren't covered — harmless,
since only this suite renders — but the justification doesn't survive its own
boundary: if renders outlive the tests that start them, they can outlive
`afterAll` too, and then the parked nodes are back. In practice the clone
starts synchronously once `Promise.all` resolves, so it lands inside the suite;
the problem is that a reader following this comment to `afterAll` gets the
wrong idea of what's guaranteed. Suggest "for the duration of this suite", plus
a note that the guarantee ends at `afterAll`.
##########
frontend/src/jsdom-svg-polyfill.ts:
##########
@@ -192,6 +192,45 @@ G.ResizeObserver ??= class {
disconnect(): void {}
};
+// `window.getComputedStyle(elt, pseudoElt)` and `window.scrollTo` — jsdom
+// implements neither. Both route through its `notImplemented` helper, which
+// emits a `jsdomError` on the virtual console; vitest's jsdom environment
+// forwards that to `console.error`, one full stack trace per call. html2canvas
+// calls both on every render — a `:before` and an `:after` lookup for each
+// cloned node, plus one `scrollTo` per document clone — so the
+// report-generation spec, which drives the real renderer on purpose (`vi.mock`
+// can't reach it under the builder's `isolate: false`), buries the run in
+// traces while all of its tests pass.
+// Dropping `pseudoElt` changes no behaviour: jsdom complains and then ignores
+// the argument, returning the element's own declaration either way. `scrollTo`
+// has nothing to move — jsdom has no layout.
+const jsdomGetComputedStyle = G.getComputedStyle as ((elt: Element,
pseudoElt?: string | null) => unknown) | undefined;
+if (typeof jsdomGetComputedStyle === "function") {
+ const withoutPseudoElement = ((elt: Element) => jsdomGetComputedStyle(elt))
as AnyFn;
Review Comment:
"Dropping `pseudoElt` changes no behaviour" holds for the `notImplemented`
path but not for shadow-DOM pseudo-elements: jsdom 25.0.1 tests
`SHADOW_DOM_PSEUDO_REGEXP` and throws `TypeError("Tried to get the computed
style of a Shadow DOM pseudo-element.")` (Window.js:890-892) *before*
`notImplemented` runs. Since the argument is discarded here,
`getComputedStyle(el, "::part(x)")` now returns a declaration instead of
throwing.
Latent — the only `getComputedStyle` caller in `src` is
`menu.component.ts:287` and it passes no pseudo-element — so this is really
about the comment overstating the equivalence, unless you'd rather keep the
regexp check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
