From 15a8ba515826aab095fc48f4cba2a2bb0c2182fa Mon Sep 17 00:00:00 2001
From: Yury Semikhatsky <yurys@chromium.org>
Date: Tue, 28 Nov 2023 17:52:16 -0800
Subject: [PATCH] fix(route): correctly remove expired handlers (#28385)

* Check if handler is still in the route list before calling it
* Check if the handler is still in the list before removing it after
`times` expiration
---
 .../src/client/browserContext.ts              |  5 ++++-
 packages/playwright-core/src/client/page.ts   |  5 ++++-
 tests/library/browsercontext-route.spec.ts    | 19 +++++++++++++++++++
 tests/page/page-route.spec.ts                 | 19 +++++++++++++++++++
 4 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/packages/playwright-core/src/client/browserContext.ts b/packages/playwright-core/src/client/browserContext.ts
index a74996bbe2c68..8c6023a70358e 100644
--- a/packages/playwright-core/src/client/browserContext.ts
+++ b/packages/playwright-core/src/client/browserContext.ts
@@ -197,8 +197,11 @@ export class BrowserContext extends ChannelOwner<channels.BrowserContextChannel>
     for (const routeHandler of routeHandlers) {
       if (!routeHandler.matches(route.request().url()))
         continue;
+      const index = this._routes.indexOf(routeHandler);
+      if (index === -1)
+        continue;
       if (routeHandler.willExpire())
-        this._routes.splice(this._routes.indexOf(routeHandler), 1);
+        this._routes.splice(index, 1);
       const handled = await routeHandler.handle(route);
       if (!this._routes.length)
         this._wrapApiCall(() => this._updateInterceptionPatterns(), true).catch(() => {});
diff --git a/packages/playwright-core/src/client/page.ts b/packages/playwright-core/src/client/page.ts
index 0430e3c0bc49a..ac15fb4ffe7d6 100644
--- a/packages/playwright-core/src/client/page.ts
+++ b/packages/playwright-core/src/client/page.ts
@@ -177,8 +177,11 @@ export class Page extends ChannelOwner<channels.PageChannel> implements api.Page
     for (const routeHandler of routeHandlers) {
       if (!routeHandler.matches(route.request().url()))
         continue;
+      const index = this._routes.indexOf(routeHandler);
+      if (index === -1)
+        continue;
       if (routeHandler.willExpire())
-        this._routes.splice(this._routes.indexOf(routeHandler), 1);
+        this._routes.splice(index, 1);
       const handled = await routeHandler.handle(route);
       if (!this._routes.length)
         this._wrapApiCall(() => this._updateInterceptionPatterns(), true).catch(() => {});
diff --git a/tests/library/browsercontext-route.spec.ts b/tests/library/browsercontext-route.spec.ts
index e600948216234..a597cf1157e2a 100644
--- a/tests/library/browsercontext-route.spec.ts
+++ b/tests/library/browsercontext-route.spec.ts
@@ -208,6 +208,25 @@ it('should support the times parameter with route matching', async ({ context, p
   expect(intercepted).toHaveLength(1);
 });
 
+it('should work if handler with times parameter was removed from another handler', async ({ context, page, server }) => {
+  const intercepted = [];
+  const handler = async route => {
+    intercepted.push('first');
+    void route.continue();
+  };
+  await context.route('**/*', handler, { times: 1 });
+  await context.route('**/*', async route => {
+    intercepted.push('second');
+    await context.unroute('**/*', handler);
+    await route.fallback();
+  });
+  await page.goto(server.EMPTY_PAGE);
+  expect(intercepted).toEqual(['second']);
+  intercepted.length = 0;
+  await page.goto(server.EMPTY_PAGE);
+  expect(intercepted).toEqual(['second']);
+});
+
 it('should support async handler w/ times', async ({ context, page, server }) => {
   await context.route('**/empty.html', async route => {
     await new Promise(f => setTimeout(f, 100));
diff --git a/tests/page/page-route.spec.ts b/tests/page/page-route.spec.ts
index 8c0a1bc80c438..b33054b8bbebd 100644
--- a/tests/page/page-route.spec.ts
+++ b/tests/page/page-route.spec.ts
@@ -902,6 +902,25 @@ it('should support the times parameter with route matching', async ({ page, serv
   expect(intercepted).toHaveLength(1);
 });
 
+it('should work if handler with times parameter was removed from another handler', async ({ page, server }) => {
+  const intercepted = [];
+  const handler = async route => {
+    intercepted.push('first');
+    void route.continue();
+  };
+  await page.route('**/*', handler, { times: 1 });
+  await page.route('**/*', async route => {
+    intercepted.push('second');
+    await page.unroute('**/*', handler);
+    await route.fallback();
+  });
+  await page.goto(server.EMPTY_PAGE);
+  expect(intercepted).toEqual(['second']);
+  intercepted.length = 0;
+  await page.goto(server.EMPTY_PAGE);
+  expect(intercepted).toEqual(['second']);
+});
+
 it('should support async handler w/ times', async ({ page, server }) => {
   await page.route('**/empty.html', async route => {
     await new Promise(f => setTimeout(f, 100));