Repository navigation
fix(markdown): serialize the current iframe document in place - #1592
lanyue-llk wants to merge 6 commits into
Conversation
Spider Bench A/B✅ All benchmark browser service runs completed; results are informational. Public HEAD: 184 / 240 rows; 38 / 48 sites produced rows; 10 unexpected empty sites. ✅ Deterministic fixture contract is clean. Common ancestor Public 48-site run · informational
Public-site content, timing, and memory are noisy. This single A/B run reports evidence only; site outcome counts explain missing rows without treating them as a deterministic regression. CPU and memory timelinesThese are bounded 40-point views of the same complete process-tree samples used by the HTML report. CPU uses 100% per occupied logical core; memory lines are RSS first and PSS second when complete PSS samples are available. Basexychart-beta
title "Base CPU"
x-axis "Elapsed seconds" [1.035, 2.036, 2.536, 3.537, 4.539, 5.541, 7.043, 8.045, 9.047, 10.55, 11.552, 12.552, 13.554, 15.057, 16.059, 17.061, 18.063, 18.563, 19.564, 20.564, 21.566, 22.066, 23.067, 24.069, 25.069, 26.07, 27.573, 28.574, 29.575, 31.078, 32.078, 33.079, 34.08, 35.582, 36.583, 37.585, 38.585, 40.087]
y-axis "CPU percent" 0 --> 200
line [16, 103.84, 113.85, 81.98, 105.75, 127.78, 13.99, 109.83, 129.75, 69.86, 125.77, 139.92, 5.99, 77.85, 69.88, 3.99, 37.94, 148.01, 69.84, 109.98, 125.75, 101.87, 117.86, 133.81, 134.02, 83.95, 109.88, 115.88, 101.95, 4, 55.95, 135.79, 79.91, 9.99, 137.6, 143.8, 53.94, 117.85]
xychart-beta
title "Base memory: RSS then PSS"
x-axis "Elapsed seconds" [0.035, 1.035, 2.036, 2.536, 3.537, 4.539, 5.541, 7.043, 8.045, 9.047, 10.55, 11.552, 12.552, 13.554, 15.057, 16.059, 17.061, 18.063, 18.563, 19.564, 20.564, 21.566, 22.066, 23.067, 24.069, 25.069, 26.07, 27.573, 28.574, 29.575, 31.078, 32.078, 33.079, 34.08, 35.582, 36.583, 37.585, 38.585, 40.087, 40.74]
y-axis "Memory MiB" 0 --> 500
line [13.28, 74.7, 118.57, 106.07, 107.93, 125.94, 140.85, 145.31, 145.03, 148.67, 135.72, 143.61, 127.48, 115.21, 130.32, 144.98, 117.51, 129.53, 160.42, 143.89, 141.27, 178.77, 247.76, 177.48, 182.02, 201.69, 133.96, 172.77, 140.95, 142.21, 138.19, 143.61, 152.99, 155.28, 125.49, 149.25, 149.49, 130.65, 155.94, 0]
line [9.94, 71.19, 114.84, 102.31, 104.19, 122.19, 138.04, 140.84, 141.27, 145.01, 131.94, 139.88, 123.69, 111.42, 126.49, 141.14, 113.63, 125.64, 156.58, 138.58, 137.55, 174.96, 243.95, 173.61, 178.19, 197.71, 129.96, 168.86, 137.07, 138.47, 134.35, 139.76, 149.13, 151.42, 121.67, 145.39, 147.31, 126.82, 152.36, 0]
HEADxychart-beta
title "HEAD CPU"
x-axis "Elapsed seconds" [1.043, 2.046, 2.547, 3.548, 4.548, 5.549, 6.05, 7.052, 8.555, 9.556, 10.556, 12.059, 13.061, 14.062, 15.563, 16.563, 17.564, 19.066, 20.066, 21.568, 22.57, 23.572, 25.074, 26.074, 27.076, 28.579, 29.58, 30.581, 32.083, 33.084, 34.086, 34.587, 35.588, 36.589, 38.092, 39.093, 40.094, 41.596]
y-axis "CPU percent" 0 --> 200
line [17.95, 91.58, 93.83, 113.96, 137.94, 135.95, 129.73, 115.79, 127.65, 59.98, 83.87, 129.74, 67.87, 155.86, 18, 18, 0, 61.96, 124.01, 99.85, 107.78, 121.81, 129.95, 57.95, 119.86, 119.82, 87.91, 79.93, 51.93, 57.94, 111.75, 103.88, 79.93, 45.99, 53.84, 113.95, 85.93, 109.68]
xychart-beta
title "HEAD memory: RSS then PSS"
x-axis "Elapsed seconds" [0.041, 1.043, 2.046, 2.547, 3.548, 4.548, 5.549, 6.05, 7.052, 8.555, 9.556, 10.556, 12.059, 13.061, 14.062, 15.563, 16.563, 17.564, 19.066, 20.066, 21.568, 22.57, 23.572, 25.074, 26.074, 27.076, 28.579, 29.58, 30.581, 32.083, 33.084, 34.086, 34.587, 35.588, 36.589, 38.092, 39.093, 40.094, 41.596, 42.387]
y-axis "Memory MiB" 0 --> 500
line [13.39, 75.21, 116.64, 102.49, 112.36, 126.14, 145.88, 147.81, 145.49, 139.14, 119.66, 134.98, 138.49, 124.6, 141.37, 132.6, 131.02, 120.09, 118.98, 135.9, 209.04, 176.55, 197.1, 213.77, 133.51, 162.02, 153.56, 135.2, 142.8, 135.72, 153.93, 247.25, 252.27, 226.72, 149.62, 145.92, 172.9, 155.38, 168.11, 0]
line [9.81, 71.27, 112.75, 98.56, 108.4, 122.2, 142.01, 143.81, 141.55, 135.05, 115.65, 130.99, 134.67, 120.6, 137.36, 128.59, 127.04, 116.1, 115.09, 131.92, 205.15, 172.57, 193.11, 209.78, 129.52, 158.27, 154.25, 131.29, 138.83, 131.76, 151.24, 239.08, 248.3, 222.39, 145.65, 141.87, 168.97, 151.41, 164.66, 0]
Deterministic fixture · required contractThe fixture exercises 15 routes: 8 are expected to emit rows and 7 intentionally exercise empty, loading, or timeout behavior.
Full HTML, JSON, CSV, logs, and page snapshots: workflow run and full The public-web A/B is informational. The exact HEAD fixture contract runs as a separate required CI check; timing and public-site content deltas remain non-blocking. |
Sequential Navigation Soak A/B✅ HEAD completed the 200-navigation resilience and memory observation. One browser process, one target, and one CDP session navigate CSDN → SegmentFault → Huaban → example.com repeatedly. This run issued 200 Common ancestor Session resilience
A public-page failure is reported separately from an unrecoverable session. The soak requires all navigation attempts, zero failed recovery, zero lifecycle/network ordering violations, and complete resource evidence. Process-tree memory
HEAD memory by 50-navigation quarter
Raw reports and per-navigation boundary samples: workflow run and full Public-site timing and memory are observational. Use the A/B deltas and trend shape as evidence, not as a deterministic performance threshold. |
CI Regression ReportSource CI run · source state at render:
Release regression — ✅ HEAD/base failures 0/0; raw binary +0.024655%Package and image size
Startup latency and PSS
Concurrency matrix
Frontend differential — ❌ 1,017/1,020 cases matched; 3 issues; 1 Chromium reference recoveries
Recovered Chromium reference infrastructure failures
First failing cases
Agent episodes · Moli vs Chromium — ✅ 8/8 Moli episodes passed; 0 failures
Runtime and CDP session contracts — ✅ 26 contract cases; 0 failures
CDP smoke — ✅ 52/52 groups passed; 621 scenariosWorkers: 4 · cumulative group time: 139.24 s · failed groups: 0
WebMainBench · 545 pages — ✅ 545/545 completed; 0 unexpected failures
Frozen HTML · Linux · Diagnostics: All artifact fields are parsed by the trusted default-branch renderer; missing or invalid inputs remain visible as unavailable. |
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
复核提交 bef729588c74，发现一项 HTML 导出回退，建议修正后再合入，具体位置见行内评论。
按 iframe 的 DOM 位置读取当前文档、按各文档 base URL 解析资源地址，修复方向有必要。但该 PR 同时替换了 HTML 快照的序列化路径，原有 Shadow DOM 保留能力随之丢失。最小修正应保留快照原来的 shadow-root 和 scripting-policy 语义，同时让 strip 和嵌套 srcdoc 更新作用于导出副本。
已对照原快照调用链与 NativeDom 序列化实现，并用 Chrome 实测显式包含 shadow root 与普通 innerHTML 的输出差异。本机未运行 Moli 的原生 iframe 集成测试。该 PR 仍依赖 #1591。
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
复审 5e2f2376deb3：上次指出的 iframe Shadow DOM 丢失问题已在代码中修正，本轮未发现新的明确阻断问题。
导出副本恢复了 shadow-root bindings，子文档重新通过 DomHost::get_html 序列化，并沿原文档读取 scripting policy；strip 也覆盖 shadow root。新增真实页面测试覆盖 open/closed root、脚本和样式剥离、live DOM 保持原样，以及 sandbox 下 noscript 的转义差异，保护了上次遗漏的功能范围。嵌套 iframe 仍按逆序写入导出副本，Markdown 的位置和文档 base URL 路径也没有被这次补丁改坏。
本轮检查了最新补丁、完整导出路径和新增测试断言。本机仍缺少原生开发工具，未重新运行 Moli/V8 集成测试；此结论不将作者报告的测试通过写成本机实测。
2661174 to
829f198
Compare
53722d3 to
8e017b8
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
This PR makes frame-aware Markdown walk each live child document at its <iframe>/<frame> position, using a composed moli_html2md::Dom view. It resolves child URLs against the child document's base and keeps the root document's Markdown unchanged. Strip options now apply at every document root, and HTML export inlines strip-applied child markup from an export DomHost that has the shadow-root bindings restored.
The core approach is sound. Frame order comes from collect_child_browsing_context_handles_in_document_order, which is a pre-order walk with visited-document and visited-handle cycle guards. That makes the .rev() deepest-first passes correct, and frame cycles cannot occur. html2md's iterative machine with MAX_DOM_OUTPUT_TREE_DEPTH bounds the composed tree, including deeply nested frames, so there is no stack-recursion risk. Cross-origin, nested and srcdoc frames are covered by tests, and the srcdoc base inherits from the parent as the spec requires.
Assessment: request changes, mainly because of the architectural collision with #1337 described below. The remaining findings are minor.
Overlap with #1337 (major, architecture). #1337 (refs/review/pr-1337) rewrites the same render_page_dump Markdown path to go through its own visibility::MarkdownDom adapter. That adapter does computed-style visibility filtering, resolves href/src/srcset/data-src/data-lazy-src/poster against the document base, and adds control_value. This PR adds a second, independent Dom adapter (FrameMarkdownDom) over raw NativeDom, with its own narrower URL overlay (href/src/poster only). Whichever merges second will conflict, and a naive merge leaves two problems:
with_frames=trueMarkdown would bypass #1337's visibility filter and URL resolution entirely, becauserender_markdown_with_frameswrapsNativeDomdirectly. Hidden navigation and stale controls would then reappear only when frames are enabled. This contradicts this PR's stated invariant that the root Markdown is identical with and without frames.- The two PRs take opposite root-URL contracts. #1337 resolves root links against the document base. This PR asserts
[Parent link](./parent-details)stays relative in both modes (markdown_with_frames_preserves_document_positions_nested_and_cross_origin_content,iframe_media_urls_use_their_own_document_base). One of these tests must change.
Child frames would also need their own per-document ExportStyle collection, since #1337 only collects styles from the root document_node_id(), and lazy data-src/srcset resolution. Suggested fix: make frame composition a layer over the single shared Markdown adapter. For example, give #1337's MarkdownDom an optional owner -> child root map and a per-document base URL, populated for each frame document. Then there is one adapter, one URL-resolution table and one visibility policy. Agree on the root-URL contract before either merges.
Tests. The new per-frame light-DOM strip loop (for (_, _, document) in &frames) is not exercised. The HTML test strips only shadow-root content, which the separate roots loop handles, and no Markdown test uses non-default strip options. Add a Markdown and an HTML case with <script>/style=/<nav> in a child document's light DOM, with the strip assertions flipping on the option.
CI. All checks pass. The Frontend differential ❌ is 3 Chromium reference infrastructure timeouts (reference_error), with 0 Moli or DOM mismatches, so it is unrelated.
Verdict: changes requested (posted as a comment because GitHub does not allow requesting changes on your own PR).
Automated review by Claude Code.
8e017b8 to
b94b661
Compare
f5e6630 to
f04b0b1
Compare
b94b661 to
7dbeedb
Compare
9272068 to
6d0a254
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Re-review of the updated version (86ed30b95)
This PR makes frame-aware Markdown and HTML export walk each live child document at its <iframe>/<frame> position. Composition is now a layer on #1337's shared visibility::MarkdownDom: frame_roots maps each owner to its child body, and URLs resolve through a per-document base map. HTML export serializes every child once, from the strip-applied export copy, and keeps the shadow-root bindings. Head 86ed30b95; reviewed by reading code only, with no build.
Verdict: request changes. The design now matches what we asked for, and all four round-1 inline findings are fixed. However, the PR is based on #1337's round-3 head f04b0b151, while #1337 is now at 92720683b. GitHub reports the PR as CONFLICTING. Replaying only this PR's five commits onto 92720683b (git merge-tree --merge-base f04b0b151 92720683b 86ed30b95) conflicts in page_dump.rs and page_dump/visibility.rs. There is also one new hidden-content leak that only occurs with frames enabled.
Previous findings
| # | Finding (round 1) | Status | Evidence at 86ed30b95 |
|---|---|---|---|
| I1 | HTML export serializes each frame twice to read its URL | FIXED | page_dump.rs:133-140 now reads child_browsing_context_current_url(owner) and calls get_html once on the export DomHost. 86ed30b95 deletes document_lookup.rs (198 lines) and ChildBrowsingContextDocumentSnapshot, and no callers remain. |
| I2 | Embedded content noise for empty or about:blank frames |
FIXED | A frame without content is no longer in frame_roots, so html2md's own media() handles it. With no src it returns nothing (machine.rs:556). about: fails safe_url (media.rs:247-262). The test (fetch_dump/tests.rs, !framed.contains("Embedded content") with srcdoc="") would have failed on the old code. The fix introduces the predicate problem in N3. |
| I3 | Second, private URL overlay (href/src/poster only) |
FIXED | The single MarkdownDom resolves through resource_url_attributes using document_bases[owner_document] (visibility.rs:246-270). FrameMarkdownDom is gone. |
| I4 | Emptiness probe re-converts nested frames, O(d·size) | FIXED | markdown_frame_roots (page_dump.rs:305-321) runs deepest-first, and frame_document_has_content stops at nested owners already in the map. That is one raw-DOM scan per document. |
| S1 | Architectural overlap with #1337 (two adapters, two policies) | FIXED | There is one adapter. The per-document targets key (document, id) and the per-document root veil (visibility.rs:62-103,184-197) are correct generalizations. |
| S2 | Opposite root-URL contracts | RESOLVED (author adopted #1337's) | The tests now assert [Parent link]({parent_url}parent-details) both with and without frames. |
| S3 | Child documents need their own ExportStyle collection |
FIXED, but it conflicts with v4 | page_dump.rs:21-36 collects every frame document. See N1. |
| S4 | Child srcset/data-src not resolved |
FIXED | This comes from the shared attribute inventory. |
| S5 | Per-frame light-DOM strip loop is untested | NOT FIXED | See N4. The new HTML test strips only shadow-root content (handled by the roots loop at :108-110), and no Markdown test uses non-default strip options. |
| (earlier P2) | Shadow DOM lost in iframe HTML snapshot | still fixed | Covered by html_with_frames_preserves_shadow_content_and_strips_only_the_export. |
New findings
| # | Severity | Location | Summary |
|---|---|---|---|
| N1 | major | page_dump.rs:21-36, visibility.rs:246-251 |
The PR must be rebased onto #1337 92720683b. It conflicts with v4's body-only style collection and the names→tag rename. |
| N2 | minor | visibility.rs:364-399 |
Content of a visibility:hidden iframe (or one under a hidden ancestor) is emitted in frame mode. |
| N3 | minor | page_dump.rs:324-366 |
The content predicate disagrees with the converter. Its about: guard does nothing, and script, style or hidden content counts as content, so the embed link fallback disappears. |
| N4 | minor | page_dump.rs:95-97 |
The per-frame light-DOM strip loop is still untested for both HTML and Markdown. There is no <frame> test either. |
| N5 | nit | visibility.rs:80-82 |
Fragment ids from all composed documents share one namespace. |
| N6 | nit | page_dump.rs:71-83,117 |
Frame-mode Markdown always clones the whole DOM, including every frame document. |
Inherited stack findings. The stack-v4 findings N2–N6 and #10 reach child documents unchanged through the shared classifier:
- N5 (blockified flex/grid/float/abspos) applies to every frame body, because child styles are computed from the child's own stylist (
styles.rs:701-709). - N3 (range/color defaults) and N6 (per-character
inline-block) also apply to child documents.
None of these needs a frame-specific fix; they are fixed once in #1337/#1336. The frame-specific addition: every composed owner is reported as div (visibility.rs:387-391), so an inline iframe always breaks the surrounding paragraph (<p>See <iframe srcdoc="<b>x</b>"></iframe> here</p> gives See\n\n**x**\n\nhere). That is acceptable for embedded documents, but the README/PR description should state it.
Hygiene and CI.
- Commit
e1e746100still introduces arender_markdown_with_framespath that ignores styles, and7dbeedb5dthen replaces it. Squash those two when rebasing so that no commit has frames bypass visibility. - The author's thread replies cite
b94b6614b, which is not in the branch. - CI on
86ed30b95: Clippy, Format, CDP smoke, Python/Node harness and Windows storage pass. Nextest and both release builds are still pending. The bot A/B comments (Spider Bench, Soak, CI regression report) are for the old head8e017b87f, so there is no A/B for this version.
Verdict: changes requested (posted as a comment because GitHub does not allow requesting changes on your own PR).
Automated review by Claude Code.
6d0a254 to
4e58ef8
Compare
a89fb18 to
8a423e7
Compare
4e58ef8 to
19bfc78
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Final re-review (P0/P1 only) (8a423e752)
Verdict: request changes, for a mechanical reason only. Head 8a423e752. The frame code looks correct, and every round-2 finding is fixed and covered by a test. The one blocker is the stack: #1337 was force-pushed again after this rebase (4e58ef889 → 19bfc785f, rebased onto a newer lower stack), so GitHub reports this PR as CONFLICTING/DIRTY. Replaying this PR's six commits onto 19bfc785f is conflict-free (git merge-tree --merge-base 4e58ef889 19bfc785f 8a423e752). The page_dump/html2md files are byte-identical between 4e58ef889 and 19bfc785f, so a plain rebase should resolve it. I reviewed by reading the code only; nothing was built.
Previous findings
| # | Finding | Status | Evidence at 8a423e752 |
|---|---|---|---|
| R2-N1 | Conflict with #1337 (style collection, base-URL source) | Code FIXED; branch conflicting again | page_dump.rs:28-44: for each document it collects <html> plus the <body> subtree, falling back to the whole document when there is no body. Bases come from the single source dom_host.document_base_url_for_handle(doc), which document_base_url() itself delegates to. v6's tag naming is kept (visibility.rs:307-311). It needs a re-rebase onto 19bfc785f (see above). |
| R2-N2 | Hidden iframe (or hidden ancestor) leaks child content | FIXED | visibility.rs:338-341 puts hidden owners into hidden_frame_owners, and first_child returns None for them (:506). An owner set back to visibility:visible still descends, which matches what a browser paints. Test: fetch_dump/tests.rs:705 (Hidden frame body absent). |
| R2-N3 | Content predicate disagrees with the converter (script-only embeds lose the link) | FIXED | The hand-written predicate is gone. markdown_frame_roots (page_dump.rs:323-354) runs the same MarkdownDom + converter on the child root, so script/style (suppressed through display:none), hidden and about:blank-only children render empty and fall back to html2md's media link. Tests: [Map fallback](https://maps.example/embed) and [Empty fallback](...) at tests.rs:727-734. |
| R2-N4 | Per-frame light-DOM strip untested | FIXED | frame_light_dom_obeys_strip_options_in_html_and_markdown (tests.rs:751). |
| R2-N5/N6 | Fragment namespace; clone on default path | FIXED | Child #frag resolves against the child base, and child id/name are hidden (visibility.rs:520; html2md reads these only in anchors.rs). The default Markdown fast path no longer clones (page_dump.rs:89-100). |
Checks requested for this round
- Blockification across the frame boundary is correct.
item_blocksandoriginal_displayuse rawparent_node. The owner is classified by its own parent's flex/grid, and the child<body>by the child<html>, never by the owner. This matches CSS, where the child document is a separate formatting root. Inherited visibility likewise starts atNormalfor the child<html>, because its parent is a Document node. - The reverse hidden-structure pass works per frame.
node_ordercontains every document's nodes, so a wholly invisible child table or list is suppressed inside its own document. A hidden owner no longer descends, so no child content can keep a hidden parent table alive. - HTML export is safe. Child markup goes through
set_attribute("srcdoc")and normal attribute serialization, so there is no raw injection.
New findings
No P0/P1 issues in the code. The only blocking item is the mechanical rebase above.
CI
On 8a423e752, Clippy, Format, CDP smoke, Python/Node harness and Windows storage pass. Nextest, Build release · HEAD and Build release · common ancestor are still queued (run 39770570147), so the new frame tests have not run in CI yet. Approve once the branch is rebased onto 19bfc785f and Nextest is green.
Verdict: changes requested (posted as a comment because GitHub does not allow requesting changes on your own PR).
Automated review by Claude Code.
8a423e7 to
152871b
Compare
1 participant
Problem
Frame-aware Markdown could read a stale iframe document, append frame text outside its DOM position, resolve child resources against the wrong document, and run a second conversion policy that disagreed with ordinary Markdown. HTML export also serialized each child twice just to recover its URL.
Change
Serialize the current child document at its iframe position. Root and child documents use the same visibility, accessible-content, media, strip, and URL rules from #1334–#1337. Each child still resolves relative destinations against its own base URL. Hidden, empty, script-only, and
about:blankframes add no placeholder text; a failed external frame remains a source link when it has a useful URL. Fragment links inside a child stay in that child's URL namespace.HTML export reads the live frame URL directly, preserves open and closed shadow roots, and applies strip options only to the export copy. This exports already-loaded DOM; it does not add PDF or DOCX parsing.
Focused verification
The Moli fixtures cover:
about:blankframes omitted without losing the owner's useful fallback link;cargo nextest run --locked -p moli --lib \ -E 'test(markdown_with_frames_) | test(iframe_media_urls_) | test(html_with_frames_) | test(frame_light_dom_)' \ --no-fail-fastBrowser Eval reproduction
Build this head, register the exact executable and SHA-256 as
moli-candidatein an instance provider file, then run the frozen Browser Eval cases through the certified native-fetch entry point:dynamic-019-selenium-dev-seleniumanddynamic-iframeexercise frame serialization directly.dynamic-015-015-testpages-iframe-listalso depends on external-child load completion, so it is a cross-PR integration case rather than part of this PR-only command. That observation creates no Git dependency on #1584. Moli contains no case-ID or site-specific branch.Stack
This PR is stacked only on #1337 (
codex/content-rendered-state), which completes the shared Markdown policy introduced by #1334–#1337. It does not depend on #1584, #1591, or #1593.