Skip to content
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

The order of idle callbacks across windows is inconsistent between spec & UAs #82

Closed
rniwa opened this issue Oct 10, 2019 · 10 comments
Closed

Comments

@rniwa
Copy link

rniwa commented Oct 10, 2019

Consider the following example where requestIdleCallback is requested in two different windows:

requestIdleCallbackIsCalled = false;
const iframe = document.createElement('iframe');
const logs = [];

iframe.onload = () => {
    requestIdleCallback(() => {
        requestIdleCallbackIsCalled = true;
        logs.push('1.A1');
    });

    iframe.contentWindow.requestIdleCallback(() => {
        requestIdleCallbackIsCalled = true;
        logs.push('2.B1');
    });

    iframe.contentWindow.requestIdleCallback(() => {
        requestIdleCallbackIsCalled = true;
        logs.push('3.B2');
    });

    requestIdleCallback(() => {
        requestIdleCallbackIsCalled = true;
        logs.push('4.A2');
    });
}
document.body.appendChild(iframe);

This would result in the log being 1.A1, 2.B1, 3.B2, 4.A2 in Chrome and sometimes in Firefox but my faithful implementation of the specification as well as Firefox would sometimes order them as 1.A1, 2.B1, 4.A2, 3.B2.

My statement assumes that step 3 in the start an idle period algorithm, which states "optionally, if the user agent determines the idle period should be delayed, return from this algorithm", doesn't allow UA to ignore the idle period for one window but not for the other.

My reading of the specification goes as follows:

  1. Adds a callback 1.A1 to the top-level window's idle request callbacks, then schedules a new task to start an idle period algorithm (SIPA-A1). Currently scheduled tasks are: [SIPA-A1].
  2. Adds a callback 2.B1 to iframe's content window's idle request callbacks, then schedules a new task to start an idle period algorithm (SIPA-B1). Now we have scheduled tasks: [SIPA-A1, SIPA-B1].
  3. Adds a callback 3.B2 to iframe's idle request callbacks. No new tasks scheduled.
  4. Adds a callback 4.A2 to iframe's idle request callbacks. No new tasks scheduled.
  5. SIPA-A1 runs. Callbacks 1.A1 and 4.A2 move from the list of idle request callbacks to the list of runnable idle callbacks.
    A new task to invoke idle callbacks (IIC-A1) is scheduled. Currently scheduled tasks are: [SIPA-B1, IIC-A1].
  6. SIPA-B1 runs. Callbacks 2.B1 and 3.B2 move from the list of idle request callbacks to the list of runnable idle callbacks.
    A new task to invoke idle callbacks (IIC-B1) is scheduled. Currently scheduled tasks are: [IIC-A1, IIC-B1].
  7. IIC-A1 runs. The callback 1.A1 is executed. A new task to invoke idle callbacks (IIC-A2) is scheduled.
    Currently scheduled tasks are: [IIC-B1, IIC-A2].
  8. IIC-B1 runs. The callback 2.B1 is executed. A new task to invoke idle callbacks (IIC-B2) is scheduled.
    Currently scheduled tasks are: [IIC-A2, IIC-B2].
  9. IIC-A2 runs. The callback 4.A2 is executed.
  10. IIC-B2 runs. The callback 3.B2 is executed.

What am I missing here? What allows Chrome and (sometimes) Firefox to execute callbacks in other orders? Perhaps step 3 in the start an idle period algorithm? If so, then 2.B1 should be the first to execute.

Perhaps step 1 in the invoke idle callbacks algorithm, which states, "if the user-agent believes it should end the idle period early due to newly scheduled high-priority work, skip to step 4." If this were to always happen at the beginning of (9) perhaps the observed order would make sense.

But then it seems problematic that the ordering of idle callbacks change across content windows. I understand the need to let UA delay work by arbitrary amount but the order of callbacks being completely dependent on UAs isn't ideal to say the least.

@rniwa
Copy link
Author

rniwa commented Oct 10, 2019

@annevk @smaug----

@annevk
Copy link
Member

annevk commented Oct 11, 2019

I suspect like mutation observers this needs to use the similar-origin window agent for guaranteeing consistent ordering.

@annevk
Copy link
Member

annevk commented Oct 11, 2019

cc @farre

@rniwa
Copy link
Author

rniwa commented Oct 11, 2019

Yeah, the fundamental problem here is that the queue is per window. It really ought to be per event loop.

@rmcilroy
Copy link
Contributor

We used to associate the queue with the event loop and not the window (and that's how we implemented it in Chrome). We changed the spec later in 9f072ac to be associated with windows to address #57. I agree though, it should really be associated with the event loop if we can avoid the issues in #57.

@annevk
Copy link
Member

annevk commented Oct 11, 2019

I don't think there's anything there that would prevent specifying a consistent ordering. But also, "don't monkey patch" doesn't mean "don't change". You can (and should if needed) PR whatwg/html if you need a different infrastructure.

rmcilroy added a commit to rmcilroy/requestidlecallback that referenced this issue Oct 30, 2019
The idle callback lists and idle periods should be per-event loop
rather than per-window. This change moves the lists and the starting
of idle periods to be on a per-event loop basis.

Addresses w3c#82
@rmcilroy
Copy link
Contributor

I've created a WIP PR in rmcilroy#1 that moves the idle callback lists to the event loop.

If this landed we would also need to change step 11 of the event loop processing model to only call the start an idle period algorithm once with the event loop as its argument. We might also want to move the definition of the idle period lists and deadline over to the html5 spec rather than defining them here.

Let me know if this seems reasonable?

@rniwa
Copy link
Author

rniwa commented Nov 23, 2019

Seems like a reasonable approach.

@yoavweiss yoavweiss added this to the Level 1 milestone Nov 2, 2020
@noamr
Copy link
Contributor

noamr commented Oct 11, 2021

I think this is solved by #95 and whatwg/html#7166 once they are merged.

The event loop iterates through the windows of that event loop.
Probably tests could reflect that better.

@noamr
Copy link
Contributor

noamr commented Jan 17, 2022

Closing, as the spec is explicity about this now. Feel free to reopen if something is missing.

@noamr noamr closed this as completed Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants