fix(animationFrameScheduler): some tasks are never flushed and sometimes it breaks completely

[pmoleri]

animationFrameScheduler has a few issues that makes it miss tasks and even start accumulating tasks that never get flushed.

• It's clearing the _scheduled in every flush, but some of the flushes are not for the next scheduled frame, they're regular async timeouts.
• because it might be picking the incorrect flushId it may flush tasks before the next frame
• after flushing and recycling the id it's clearing the _scheduled even though current id might be a different one.

Related issues:

• animationFrameScheduler stops working unexpectedly #7018
• animationFrameScheduler with delay causes incorrect behavior (and exception) #6891
• asapScheduler: Scheduling inside of an executing action only works once #7196
  already fixed but similar cause

Failing tests are included in a independent commit and then fixed.

Note:
The changes could also be ported to AsapScheduler for consistency (also I think it's more clear).
However, I couldn't reproduce the issues there because:

• AsapScheduler already contains one of the fixes
• In AsapScheduler I couldn't schedule a task to happen before the _scheduled one.

[pmoleri]

Hi @benlesh. I noticed that you fixed #7196 for asapScheduler, but animationFrameScheduler has similar issue, and worse even (reason explained in this PR description).
Any chance that you can review this PR? It also includes tests that make it fail without the fix.

[BlueCat0]

any update yet?

[pmoleri]

@BlueCat0 Unfortunately it seems that no one had the time to review it.
Once you understand the code (which takes a while) the bug is pretty clear and the resolution too.

To avoid the bug altogether, never use animationFrameScheduler with a delay, you can use the asyncScheduler for that purpose. In fact they would behave exactly the same, as animationFrameScheduler doesn't really schedule into a frame when delay is used.

Note that using animationFrameScheduler with a delay in one place may impact in a different place.

[ChristophWieske]

@pmoleri Your PR is open for quite some time. Has there been another PR which made yours obsolete? Or is this still waiting for a review?

I encounter issues with animationFrameScheduler in 7.8.1 which are maybe related.

So, is the bug you are trying to fix here still unfixed?

[pmoleri]

Hi @ChristophWieske

The issue is actually quite serious, it can affect any Angular app combining animationFrameScheduler with a delay other than 0.
It's very hard to troubleshoot, it leads to anomalous behavior or crashes depending on the case.

In the 7.x branch the files have not been touched in 3 years, so I'm positive that the issue is still there.
I've included tests to make it fail (if you omit the fix).

In master the files have been moved around and the imports changed but the code is exactly the same.
So it must happen too.

[pmoleri]

Hi @benlesh. This PR fixes real bugs and it has tests to confirm the bug and the fix.
The changes are small and I consider them sound.

I made the PR to 7.x, but I could re-target to master and backport later.
I can make the changes that you consider best. Thanks.

[trxcllnt]

I'm curious the order in which these actions are expected to execute.

Is runFirst expected to be true when runDelayed is set to true? e.g.:

Suggested change

	animationFrame.schedule(() => { runDelayed = true; }, 1);
	animationFrame.schedule(() => {
	if (!runFirst) {
	done(new Error('First action did not run'));
	} else {
	runDelayed = true;
	}
	}, 1);

[pmoleri]

No, it's the opposite. The action queued with a delay smaller than animationFrame runs first.
That's how animationFrameScheduler is implemented, actions with delay other than zero are scheduled purely based on time and not actual animation frames.
I'll push a change to make it more clear.

[pmoleri]

There. I pushed the change. Now each scheduled action asserts the previous one. Instead of having everything in the last one.