-
Notifications
You must be signed in to change notification settings - Fork 56
Upgrade to upstream WebKit c119008088 #541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0839f93
1392437
5e2ef4f
04390f8
96aed03
e4856c6
d4a150e
e903a14
c119008
2bb7c6a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,52 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||
| //@ runDefault("--useDollarVM=1", "--warmUpMarkedBlockCount=8", "--warmUpMarkedBlockIdleTimeout=0.2") | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| // Drives the warm-up supply through its whole state machine: fill, release on idle, restart, | ||||||||||||||||||||||||||||||||||||||||||||||||
| // stand-down on allocation failure, and recovery once allocation works again. Every assertion is | ||||||||||||||||||||||||||||||||||||||||||||||||
| // "reaches this state eventually", since a helper thread drives the transitions. | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| const pollSeconds = 0.05; | ||||||||||||||||||||||||||||||||||||||||||||||||
| const timeoutSeconds = 20; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| const retained = []; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| function allocateBlocks() { | ||||||||||||||||||||||||||||||||||||||||||||||||
| // Retaining these is the point: a heap that can recycle stops asking the allocator for blocks, | ||||||||||||||||||||||||||||||||||||||||||||||||
| // and sustained block demand is what the stand-down and restart paths are driven by. The count | ||||||||||||||||||||||||||||||||||||||||||||||||
| // is kept small so that a run which ends up timing out still reports rather than exhausting | ||||||||||||||||||||||||||||||||||||||||||||||||
| // memory first. | ||||||||||||||||||||||||||||||||||||||||||||||||
| for (let i = 0; i < 2000; ++i) | ||||||||||||||||||||||||||||||||||||||||||||||||
| retained.push({ a: i, b: i, c: i }); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+10
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Cap the retained set so a timing-out run reports instead of exhausting memory.
Add a ceiling on 🧪 Proposed fix to bound retention const retained = [];
+const retainedLimit = 200000;
function allocateBlocks() {
// Retaining these is the point: a heap that can recycle stops asking the allocator for blocks,
// and sustained block demand is what the stand-down and restart paths are driven by. The count
// is kept small so that a run which ends up timing out still reports rather than exhausting
// memory first.
for (let i = 0; i < 2000; ++i)
retained.push({ a: i, b: i, c: i });
+ if (retained.length > retainedLimit)
+ retained.splice(0, retained.length - retainedLimit);
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| function waitFor(description, predicate, betweenAttempts = () => { }) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| for (let attempt = 0; attempt < timeoutSeconds / pollSeconds; ++attempt) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| if (predicate($vm.warmUpMarkedBlockState())) | ||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||
| betweenAttempts(); | ||||||||||||||||||||||||||||||||||||||||||||||||
| sleepSeconds(pollSeconds); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| const state = $vm.warmUpMarkedBlockState(); | ||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(`Timed out waiting for ${description}; blocks=${state.blocks} phase=${state.phase}`); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| const isFilled = state => state.phase === "armed" && state.blocks > 0; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| // Demand starts the helper, which arms to the configured depth and fills. | ||||||||||||||||||||||||||||||||||||||||||||||||
| allocateBlocks(); | ||||||||||||||||||||||||||||||||||||||||||||||||
| waitFor("the supply to fill", isFilled, allocateBlocks); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| // With no demand at all, the helper hands everything back and shuts down. | ||||||||||||||||||||||||||||||||||||||||||||||||
| waitFor("the supply to be released when idle", state => state.phase === "stopped"); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| // Fresh demand brings it back. | ||||||||||||||||||||||||||||||||||||||||||||||||
| allocateBlocks(); | ||||||||||||||||||||||||||||||||||||||||||||||||
| waitFor("the helper to restart", isFilled, allocateBlocks); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| // An allocation failure makes it stand down rather than spin against an exhausted heap. | ||||||||||||||||||||||||||||||||||||||||||||||||
| $vm.setWarmUpMarkedBlockAllocationShouldFail(true); | ||||||||||||||||||||||||||||||||||||||||||||||||
| waitFor("the helper to stand down", state => state.phase === "standingDown", allocateBlocks); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| // Standing down must not be permanent: the idle timeout still fires while demand continues, and | ||||||||||||||||||||||||||||||||||||||||||||||||
| // lifts it once allocation works again. | ||||||||||||||||||||||||||||||||||||||||||||||||
| $vm.setWarmUpMarkedBlockAllocationShouldFail(false); | ||||||||||||||||||||||||||||||||||||||||||||||||
| waitFor("the stand-down to lift", isFilled, allocateBlocks); | ||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| //@ runDefault("--useWarmUpMarkedBlocks=1", "--warmUpMarkedBlockCount=64") | ||
| //@ runDefault("--useWarmUpMarkedBlocks=1", "--warmUpMarkedBlockCount=1") | ||
| //@ runDefault("--useWarmUpMarkedBlocks=1", "--warmUpMarkedBlockCount=0") | ||
| //@ runDefault("--useWarmUpMarkedBlocks=0") | ||
| //@ runDefault("--useWarmUpMarkedBlocks=1", "--warmUpMarkedBlockCount=8", "--scribbleFreeCells=1") | ||
| //@ runDefault("--useWarmUpMarkedBlocks=1", "--forceMiniVMMode=1") | ||
|
|
||
| function shouldBe(actual, expected) { | ||
| if (actual !== expected) | ||
| throw new Error(`Expected ${expected} but got ${actual}`); | ||
| } | ||
|
|
||
| let chain = null; | ||
| let expectedLength = 0; | ||
| for (let i = 0; i < 300000; ++i) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): This new stress test hardcodes Extended reasoning...JSTests/CLAUDE.md imports JSTests/README.md, whose rule 2 requires new stress tests to derive their iteration count from Verification: nit — The new file /home/claude/webkit/JSTests/stress/warm-up-marked-blocks.js line 15 hardcodes |
||
| const garbage = { index: i, payload: [i, i + 1, i + 2] }; | ||
| shouldBe(garbage.payload[2], i + 2); | ||
| if (!(i % 1000)) { | ||
| chain = { index: i, next: chain }; | ||
| ++expectedLength; | ||
| } | ||
| } | ||
|
|
||
| let length = 0; | ||
| for (let node = chain; node; node = node.next) { | ||
| ++length; | ||
| shouldBe(node.index, (expectedLength - length) * 1000); | ||
| } | ||
| shouldBe(length, expectedLength); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| Under site isolation, a remote-control command received when no media session is NowPlaying-eligible is still delivered to the current session, matching the non-site-isolated behavior. The video is loaded but never played, so it is not NowPlaying-eligible and the GPU process has no elected owner; a 'play' command injected at the GPU process must reach the video and start playback. | ||
|
|
||
| On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". | ||
|
|
||
|
|
||
| PASS videoIsCommandTarget is true | ||
| PASS videoIsElectedOwner is false | ||
| PASS videoStartedPlaying is true | ||
| PASS successfullyParsed is true | ||
|
|
||
| TEST COMPLETE | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| <!-- webkit-test-runner [ SiteIsolationEnabled=true allowTestOnlyIPC=true ] --> | ||
| <!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <script src="/js-test-resources/js-test.js"></script> | ||
| <script src="/media-resources/utilities.js"></script> | ||
| </head> | ||
| <body> | ||
| <video id="video" loop src="/media-resources/content/test.mp4" style="width: 640px; height: 480px"></video> | ||
| <script> | ||
| description("Under site isolation, a remote-control command received when no media session is NowPlaying-eligible is still delivered to the current session, matching the non-site-isolated behavior. The video is loaded but never played, so it is not NowPlaying-eligible and the GPU process has no elected owner; a 'play' command injected at the GPU process must reach the video and start playback."); | ||
| jsTestIsAsync = true; | ||
|
|
||
| let video = document.getElementById("video"); | ||
| let videoIsCommandTarget = false; | ||
| let videoIsElectedOwner = true; | ||
| let videoStartedPlaying = false; | ||
|
|
||
| onload = async () => { | ||
| if (!window.internals) { | ||
| testFailed("This test requires the Internals API"); | ||
| finishJSTest(); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await new Promise(resolve => { | ||
| video.addEventListener("canplaythrough", resolve, { once: true }); | ||
| video.load(); | ||
| }); | ||
| video.volume = 0.001; | ||
|
|
||
| await waitUntil(async () => { | ||
| videoIsCommandTarget = await internals.elementIsRemoteCommandTargetInGPUProcess(video); | ||
| return videoIsCommandTarget; | ||
| }, { tries: 200, intervalMs: 10 }); | ||
| shouldBeTrue("videoIsCommandTarget"); | ||
|
|
||
| // The premise of the test: the video was never played, so no session is NowPlaying-eligible and the GPU | ||
| // process elected nobody. Without this the test could pass through the ordinary elected path instead. | ||
| videoIsElectedOwner = await internals.elementIsActiveNowPlayingSessionInGPUProcess(video); | ||
| shouldBeFalse("videoIsElectedOwner"); | ||
|
|
||
| const playing = new Promise(resolve => video.addEventListener("playing", resolve, { once: true })); | ||
| internals.postSystemRemoteControlCommand("play"); | ||
|
|
||
| await Promise.race([playing, delay(5000)]); | ||
|
|
||
| videoStartedPlaying = !video.paused; | ||
| shouldBeTrue("videoStartedPlaying"); | ||
| } catch (e) { | ||
| testFailed(e.message); | ||
| } | ||
|
|
||
| video.pause(); | ||
| finishJSTest(); | ||
| }; | ||
| </script> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| Under site isolation, a system remote-control command received by the GPU process is delivered to the GPU-elected NowPlaying session, even when that session lives in a different web process than the one that injects the command. The main-frame video plays first, then the cross-site subframe video, which becomes the elected session; a pause command injected from the main frame's process must reach and pause the subframe's session. (The main frame's own playback state is not asserted: platforms that forbid concurrent playback, e.g. iOS, pause it when the subframe starts.) | ||
|
|
||
| On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". | ||
|
|
||
|
|
||
| PASS subframeElectedInitially is true | ||
| PASS subframePausedAfterCommand is true | ||
| PASS successfullyParsed is true | ||
|
|
||
| TEST COMPLETE | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 nit (optional): This new stress test cannot satisfy JSTests/README.md rule 1 ("Tests must run in under 200ms in all configurations", imported via JSTests/CLAUDE.md): it waits on the helper thread's 0.2 s idle timeout at least twice (once to observe
phase === "stopped", once to lift the stand-down) and polls with 50 mssleepSecondsbetween checks, so a successful run is ~600 ms–1 s and thetimeoutSeconds = 20ceiling allows a 20 s hang on a slow bot. Fix: mark it slow/skip for the stress runner (e.g.//@ slow!or move it out ofstress/), or shortenwarmUpMarkedBlockIdleTimeoutandpollSecondsenough that the whole state-machine walk fits in 200 ms.Extended reasoning...
waitFor("the supply to be released when idle", state => state.phase === "stopped")must wait for AutomaticThread's timeout (--warmUpMarkedBlockIdleTimeout=0.2) to fire with no demand beforethreadIsStoppingsetsPhase::Stopped; that is 200 ms plus at least one 50 ms poll. Later,waitFor("the stand-down to lift", ...)requires another idle-timeout cycle: whilem_phase == StandingDown,tryTakedeliberately does not notify the condition, soshouldSleeponly runs after the 0.2 s timeout and only then flips the phase back toArmed. Those two mandatory 200 ms waits plus polling put the best-case runtime well over the fork's 200 ms budget forJSTests/stress/. On the base branch this file does not exist, so merging adds a test that regularly exceeds the suite's per-test time budget and can stall for up to 20 s under load, slowingrun-jsc-stress-tests.Verification: nit — The timing analysis is correct and the test structurally cannot meet JSTests/README.md rule 1 ("Tests must run in under 200ms in all configurations", line 19). Line 1 sets
--warmUpMarkedBlockIdleTimeout=0.2, and the AutomaticThread is constructed with that as its timeout: `AutomaticThread(locker, provider.m_lock, provider.m_condition.copyRef(),… | nit — The timing analysis is correct:…