FakeAsyncZone and RootZone used in conjunction can clobber interleaved timers of each other

[hilts-vaughan]

Which @angular/* package(s) are the source of the bug?

zone.js

Is this a regression?

Yes

Description

Currently, Zone.js patches setTimeout and keeps a tasksByHandleId map to keep a timerId <-> ZoneTask
relationship. This map appears to be global to each method per window (browser use case) / global instance and not per Zone as it's defined on patchTimer.

Tasks then use the return value from their schedule methods (such as setTimeout) to store this ID in tasksByHandleId. For example, in the case of a root zone with no wrapping it normally just calls
setNative which may invoke setTimeout and we store the timer ID in the map. These IDs are monotonically increasing and therefore are never reused. However, in the case of fake-async-test it starts from 1 and increments it's own counters per task.

If you have a Zone that is using setNative and scheduling using the real browser scheduler and using fake-async-zone it is possible to generate two tasks that are both pending with the same ID because they don't know of each other. For example:

let oldTimerId = 
runInFakeAsyncZone(() => {
  // Run a timer in such a way that it also returns the same timer ID
  // but does not resolve, thus overwriting the taskMap
  // Maybe it wasn't fired from an even and has to be flushed or something
  oldTimerId = ...
});

// Run in <root>, running later
// timerId == oldTimerId now
// Therefore, `tasksByHandleId` is overwritten to use this ID
let timerId = setTimeout(() => console.log('I should run but never do'), 30000);


runInFakeAsyncZone(() => {
  // We changed our mind, cancel the timer 
  clearInterval(oldTimerId)

  // Actually, they shared an ID and `tasksByHandleId` was updated
  // to point to the <root> task so `task.zone.cancelTask(task)` is invoked
  // but the implementation uses `clearNative` and clears the <root> task
  // meaning that the setTimeout never runs
});

In the above psuedo-code, the task that runs in some other zone is overwritten with one that runs the root later because of the shared map. In contexts outside of fake-async-test, it's likely never going to happen
because the IDs are strictly monotonically increasing. But in fake-async-test, this is troublesome because they can overlap which is bad news when using a single global tasksByHandleId

Some thoughts:

1. I don't know if this use case is that common but it's at least common in a couple of Google tests that trigger this behaviour
2. If overlap is not expected, the task map should probably assert and break early because this was a huge pain to track down as not only is timing sensitive but it's also a read / write race
3. If overlap is expected in some cases, we need to somehow handle this

I've not included a full reproduction yet because the exact specifics are kind of hard to encode but I will try and get something up.

Please provide a link to a minimal reproduction of the bug

N/A; coming soon maybe

Please provide the exception or error you saw

No response

Please provide the environment you discovered this bug in (run ng version)

Angular: HEAD

Anything else?

No response

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}