-
Notifications
You must be signed in to change notification settings - Fork 47
Clarify that the scheduling state is supposed to be per event loop #114
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
Comments
The specific worry I had when reviewing the implementation is that if there are same site (not same origin) iframes, they share the same event loop because with .domain one can make those same origin. |
I think of the scheduling state as a property of the currently running task or microtask (accessed through the event loop), which should be null at the start of each event loop task (cleared after postTask callback and microtasks). A task can call into other windows/contexts, but the per-task state remains consistent (i.e. priority has not changed), which I think is desirable in cases where frames (intentionally/are mutually expecting to) script each other. For most scenarios I was thinking about, scripting is intentional and requires A to call B for B to learn about A, e.g.: // Frame B (child)
window.doSomethingInOtherFrame = async() => {
let taskFirst = false;
await Promise.race([scheduler.postTask(() => { taskFirst = true; }), scheduler.yield()]);
if (taskFirst) {
console.log('The yield had lower priority');
}
}
// Frame A (parent)
scheduler.postTask(async () => {
// Call into frame B, otherwise B's code doesn't run in this task.
await child.contentWindow.doSomethingInOtherFrame();
}, {priority: 'background'}); But I guess frame A could unintentionally run frame B code via events, e.g. if frame B calls A's Are there other scenarios I'm missing, or does that still seem concerning? |
I'm worried about the cases when something happens synchronously in a cross origin iframe when parent does something for example in a promise callback. |
@shaseley Here the updated testcase that Olli and I came up https://mozilla.seanfeng.dev/files/outer2.html. It sets the If you click the
What looks suspicious is the |
Might be worth to test also on Android without Origin-Agent-Cluster: ?0. Anyhow, the cross-origin leak is rather serious and could easily break sites in very unexpected way. |
Thanks for the example, agreed this case is problematic. Making the state per-Scheduler/global might be fine -- I'll think this over and talk to a few folks, and update this soon. |
Update: I patched Blink to check "same origin-domain" where the scheduling state is used (as a temp fix), and I added a WPT test for the blur() case based on the example, along with some other cross-frame tests documenting the current behavior. web-platform-tests/wpt#51023. As far as resolving this, I think there are several options:
I'm leaning towards (2) since asynchronous callbacks/events that occur within a task/microtask aren't clearly a continuation of that task, as the blur() example illustrates. Treating nested callbacks/events as new "task roots" with no scheduling state seems reasonable, and I think it would eliminate some unexpected behavior. I'm not sure about (3) though. I think it could lead to surprising behavior if doing intentional cross-frame scripting, like frameA calls postTask(frameB_Task); or frameA_Task calls a frameB_Function, which would behave differently if it were in a library. |
"Nested" events are very much part of the ongoing task. That is the model microtasks have had for 13 years now. (microtask checkpoint is at the end of outermost script execution or innermost task). I think (3) would make most sense. That way caller from page A can't affect (and thus break) scheduling of something in page B so easily. |
I just meant that code observing behavior (event listener) might be conceptually a different "task" / "thread of execution" (or whatever, these are all overloaded) from the developer's perspective than the code causing behavior -- even though they are literally part of the same task: addEventListener("done", async () => {
// Get out of the way.
await scheduler.yield();
doDependentOperation1();
});
addEventListener("done", async () => {
// Get out of the way.
await scheduler.yield();
doDependentOperation2();
});
function task() {
// ...
// Inform observers:
dispatchEvent(new Event("done"));
} But it's easy enough to schedule things as separate tasks rather than continuations if that's indeed what's intended. |
Currently the scheduling state is set on the event loop, which means mulitple globals (multiple iframes) that share the same event loop can potentially interfere with each other.
It feels unusual to have this behaviour, or at least it'd be safer to make it per global.
@shaseley Scott, do you mind confirm that the intention was to make it per event loop?
The text was updated successfully, but these errors were encountered: