FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(core): prevent orphaned requestIdleCallback handle from re-entran… · angular/angular@7bcce26 · GitHub

/ angular Public

Commit 7bcce26

Browse files
arturovt
authored and
thePunderWoman
committed
fix(core): prevent orphaned requestIdleCallback handle from re-entrant scheduling
Defer blocks that ask to run during browser idle time with the same options get grouped into one batch ("bucket") and processed together the next time the browser is idle. While that batch runs, if one callback (or the change-detection check right after it) asks to schedule more idle work for that same batch, Angular used to register a second idle-callback handle instead of reusing the one already in flight. That second handle became orphaned. By the time the batch finished running, its queue was empty, so Angular threw away the bookkeeping for it — including the only reference that could have cancelled the handle. If the app was destroyed before the browser got around to calling it, it fired anyway, against a scheduler that no longer existed. The root cause was a marker (idleId) that IdleScheduler uses to know "I already have a browser callback pending for this batch, don't request another one." That marker was being cleared to null right at the start of processing a batch, before any of its callbacks had actually run. So a callback that re-entrantly asked to schedule more work mid-batch saw "nothing pending" and requested a redundant handle. The fix: leave the marker set for the entire batch instead of clearing it up front. Only clear it once every callback in the batch has run — at that point, if there's leftover work, it's safe to request a fresh handle for it. Added a test for the basic re-entrant scenario, plus five more for edge cases: a re-entrant add() into a different batch, and re-entrant remove() of a sibling callback that hasn't run yet versus one that already has. (cherry picked from commit c2b14b7)
1 parent ef2ce9a commit 7bcce26

2 files changed

Lines changed: 157 additions & 2 deletions

File tree

‎packages/core/src/defer/idle_scheduler.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,7 @@ export class IdleScheduler implements OnDestroy {
102102

103103
const key = getIdleRequestKey(options);
104104
const callback = (deadline?: IdleDeadline) => {
105-
this.cancelBucket(bucket);
106-
105+
// Keep idleId set during the drain to prevent re-entrant add() from scheduling redundant callbacks.
107106
for (const cb of bucket.queue) {
108107
cb();
109108
// _tick here is an optimized change detection check and is safe to call here.
@@ -118,6 +117,8 @@ export class IdleScheduler implements OnDestroy {
118117
}
119118
}
120119

120+
bucket.idleId = null;
121+
121122
if (bucket.queue.size > 0) {
122123
this.scheduleBucket(bucket, options);
123124
} else {

‎packages/core/test/acceptance/defer_spec.ts‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5502,4 +5502,158 @@ describe('IdleScheduler', () => {
55025502
capturedCbs[0]({didTimeout: false, timeRemaining: () => 10});
55035503
expect(cb1).toHaveBeenCalledTimes(1);
55045504
});
5505+
5506+
it('should not register a spurious requestOnIdle when a callback re-entrantly adds to the same bucket', () => {
5507+
let capturedCb: ((deadline: any) => void) | null = null;
5508+
let ricCount = 0;
5509+
5510+
customIdleService.requestOnIdleSpy.and.callFake((cb: any) => {
5511+
ricCount++;
5512+
capturedCb = cb;
5513+
return 100 + ricCount;
5514+
});
5515+
5516+
const cbB = jasmine.createSpy('cbB');
5517+
const cbA = jasmine.createSpy('cbA').and.callFake(() => {
5518+
scheduler.add(cbB);
5519+
});
5520+
5521+
scheduler.add(cbA);
5522+
expect(ricCount).toBe(1);
5523+
5524+
capturedCb!({didTimeout: false, timeRemaining: () => 9999});
5525+
5526+
expect(cbA).toHaveBeenCalledTimes(1);
5527+
expect(cbB).toHaveBeenCalledTimes(1);
5528+
expect(ricCount).toBe(1);
5529+
});
5530+
5531+
it('should defer a re-entrantly-added same-bucket callback to the next idle period when the deadline expires', () => {
5532+
let capturedCbs: Array<(deadline: any) => void> = [];
5533+
let ricCount = 0;
5534+
5535+
customIdleService.requestOnIdleSpy.and.callFake((cb: any) => {
5536+
ricCount++;
5537+
capturedCbs.push(cb);
5538+
return 100 + ricCount;
5539+
});
5540+
5541+
const cbB = jasmine.createSpy('cbB');
5542+
const cbA = jasmine.createSpy('cbA').and.callFake(() => {
5543+
scheduler.add(cbB);
5544+
});
5545+
5546+
scheduler.add(cbA);
5547+
capturedCbs[0]({didTimeout: false, timeRemaining: () => 0});
5548+
5549+
expect(cbA).toHaveBeenCalledTimes(1);
5550+
expect(cbB).toHaveBeenCalledTimes(0);
5551+
expect(ricCount).toBe(2);
5552+
capturedCbs[1]({didTimeout: false, timeRemaining: () => 10});
5553+
expect(cbB).toHaveBeenCalledTimes(1);
5554+
});
5555+
5556+
it('should isolate a re-entrantly-added callback in a different bucket from the current drain', () => {
5557+
let capturedCbs: Array<(deadline: any) => void> = [];
5558+
let ricCount = 0;
5559+
5560+
customIdleService.requestOnIdleSpy.and.callFake((cb: any) => {
5561+
ricCount++;
5562+
capturedCbs.push(cb);
5563+
return 100 + ricCount;
5564+
});
5565+
5566+
const cbB = jasmine.createSpy('cbB');
5567+
const cbA = jasmine.createSpy('cbA').and.callFake(() => {
5568+
scheduler.add(cbB, {timeout: 500});
5569+
});
5570+
5571+
scheduler.add(cbA);
5572+
capturedCbs[0]({didTimeout: false, timeRemaining: () => 10});
5573+
5574+
expect(cbA).toHaveBeenCalledTimes(1);
5575+
expect(cbB).toHaveBeenCalledTimes(0);
5576+
expect(ricCount).toBe(2);
5577+
5578+
capturedCbs[1]({didTimeout: false, timeRemaining: () => 10});
5579+
expect(cbB).toHaveBeenCalledTimes(1);
5580+
});
5581+
5582+
it('should skip a same-bucket sibling that is removed during a drain', () => {
5583+
let capturedCb: ((deadline: any) => void) | null = null;
5584+
let ricCount = 0;
5585+
5586+
customIdleService.requestOnIdleSpy.and.callFake((cb: any) => {
5587+
ricCount++;
5588+
capturedCb = cb;
5589+
return 100 + ricCount;
5590+
});
5591+
5592+
const cbC = jasmine.createSpy('cbC');
5593+
const cbB = jasmine.createSpy('cbB');
5594+
const cbA = jasmine.createSpy('cbA').and.callFake(() => {
5595+
scheduler.remove(cbC);
5596+
});
5597+
5598+
scheduler.add(cbA);
5599+
scheduler.add(cbB);
5600+
scheduler.add(cbC);
5601+
5602+
capturedCb!({didTimeout: false, timeRemaining: () => 10});
5603+
5604+
expect(cbA).toHaveBeenCalledTimes(1);
5605+
expect(cbB).toHaveBeenCalledTimes(1);
5606+
expect(cbC).toHaveBeenCalledTimes(0);
5607+
expect(ricCount).toBe(1);
5608+
});
5609+
5610+
it('should skip the immediately-next callback when it is removed during a drain', () => {
5611+
let capturedCb: ((deadline: any) => void) | null = null;
5612+
let ricCount = 0;
5613+
5614+
customIdleService.requestOnIdleSpy.and.callFake((cb: any) => {
5615+
ricCount++;
5616+
capturedCb = cb;
5617+
return 100 + ricCount;
5618+
});
5619+
5620+
const cbB = jasmine.createSpy('cbB');
5621+
const cbA = jasmine.createSpy('cbA').and.callFake(() => {
5622+
scheduler.remove(cbB);
5623+
});
5624+
5625+
scheduler.add(cbA);
5626+
scheduler.add(cbB);
5627+
5628+
capturedCb!({didTimeout: false, timeRemaining: () => 10});
5629+
5630+
expect(cbA).toHaveBeenCalledTimes(1);
5631+
expect(cbB).toHaveBeenCalledTimes(0);
5632+
expect(ricCount).toBe(1);
5633+
});
5634+
5635+
it('should ignore re-entrant remove() calls for callbacks already processed in the current drain', () => {
5636+
let capturedCb: ((deadline: any) => void) | null = null;
5637+
let ricCount = 0;
5638+
5639+
customIdleService.requestOnIdleSpy.and.callFake((cb: any) => {
5640+
ricCount++;
5641+
capturedCb = cb;
5642+
return 100 + ricCount;
5643+
});
5644+
5645+
const cbA = jasmine.createSpy('cbA');
5646+
const cbB = jasmine.createSpy('cbB').and.callFake(() => {
5647+
scheduler.remove(cbA);
5648+
});
5649+
5650+
scheduler.add(cbA);
5651+
scheduler.add(cbB);
5652+
5653+
expect(() => capturedCb!({didTimeout: false, timeRemaining: () => 10})).not.toThrow();
5654+
5655+
expect(cbA).toHaveBeenCalledTimes(1);
5656+
expect(cbB).toHaveBeenCalledTimes(1);
5657+
expect(ricCount).toBe(1);
5658+
});
55055659
});

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL