From 3ae50861a341a170072e223339dd48ff7211b884 Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Fri, 12 Aug 2022 13:48:47 -0700
Subject: [PATCH] fix(reload): make sure reload() does not pick same-document
 navigaiton (#16504)

We lack `documentId` when doing a reload over browser protocols, so
`reload()` waits for the next navigation to finish. Sometimes, the page
might issue a same-document navigation while reload is in progress,
which confuses the reload command.

To fix the issue, just ignore same-document navigations for reload,
because it is always a new document.
---
 packages/playwright-core/src/server/frames.ts |  4 ++-
 packages/playwright-core/src/server/page.ts   |  7 ++--
 tests/page/page-history.spec.ts               | 32 +++++++++++++++++++
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts
index d351c4a4a2..cf2a8afd83 100644
--- a/packages/playwright-core/src/server/frames.ts
+++ b/packages/playwright-core/src/server/frames.ts
@@ -715,7 +715,7 @@ export class Frame extends SdkObject {
     return response;
   }
 
-  async _waitForNavigation(progress: Progress, options: types.NavigateOptions): Promise<network.Response | null> {
+  async _waitForNavigation(progress: Progress, requiresNewDocument: boolean, options: types.NavigateOptions): Promise<network.Response | null> {
     const waitUntil = verifyLifecycle('waitUntil', options.waitUntil === undefined ? 'load' : options.waitUntil);
     progress.log(`waiting for navigation until "${waitUntil}"`);
 
@@ -723,6 +723,8 @@ export class Frame extends SdkObject {
       // Any failed navigation results in a rejection.
       if (event.error)
         return true;
+      if (requiresNewDocument && !event.newDocument)
+        return false;
       progress.log(`  navigated to "${this._url}"`);
       return true;
     }).promise;
diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts
index f8d6a90344..92a741278d 100644
--- a/packages/playwright-core/src/server/page.ts
+++ b/packages/playwright-core/src/server/page.ts
@@ -372,7 +372,8 @@ export class Page extends SdkObject {
       // Note: waitForNavigation may fail before we get response to reload(),
       // so we should await it immediately.
       const [response] = await Promise.all([
-        this.mainFrame()._waitForNavigation(progress, options),
+        // Reload must be a new document, and should not be confused with a stray pushState.
+        this.mainFrame()._waitForNavigation(progress, true /* requiresNewDocument */, options),
         this._delegate.reload(),
       ]);
       await this._doSlowMo();
@@ -386,7 +387,7 @@ export class Page extends SdkObject {
       // Note: waitForNavigation may fail before we get response to goBack,
       // so we should catch it immediately.
       let error: Error | undefined;
-      const waitPromise = this.mainFrame()._waitForNavigation(progress, options).catch(e => {
+      const waitPromise = this.mainFrame()._waitForNavigation(progress, false /* requiresNewDocument */, options).catch(e => {
         error = e;
         return null;
       });
@@ -407,7 +408,7 @@ export class Page extends SdkObject {
       // Note: waitForNavigation may fail before we get response to goForward,
       // so we should catch it immediately.
       let error: Error | undefined;
-      const waitPromise = this.mainFrame()._waitForNavigation(progress, options).catch(e => {
+      const waitPromise = this.mainFrame()._waitForNavigation(progress, false /* requiresNewDocument */, options).catch(e => {
         error = e;
         return null;
       });
diff --git a/tests/page/page-history.spec.ts b/tests/page/page-history.spec.ts
index b9dad63bf3..85c8ecb781 100644
--- a/tests/page/page-history.spec.ts
+++ b/tests/page/page-history.spec.ts
@@ -114,6 +114,38 @@ it('page.reload during renderer-initiated navigation', async ({ page, server })
   await page.waitForSelector('text=hello');
 });
 
+it('page.reload should not resolve with same-document navigation', async ({ page, server }) => {
+  await page.goto(server.EMPTY_PAGE);
+  // 1. Make sure execution contexts are ready for fast evaluate.
+  await page.evaluate('1');
+
+  // 2. Stall the reload request.
+  let response;
+  server.setRoute('/empty.html', (req, res) => { response = res; });
+  const requestPromise = server.waitForRequest('/empty.html');
+
+  // 3. Trigger push state that could resolve the reload.
+  page.evaluate(() => {
+    window.history.pushState({}, '');
+  }).catch(() => {});
+
+  // 4. Trigger the reload, it should not resolve.
+  const reloadPromise = page.reload();
+
+  // 5. Trigger push state again, for the good measure :)
+  page.evaluate(() => {
+    window.history.pushState({}, '');
+  }).catch(() => {});
+
+  // 5. Serve the request, it should resolve the reload.
+  await requestPromise;
+  response.end('hello');
+
+  // 6. Check the reload response.
+  const gotResponse = await reloadPromise;
+  expect(await gotResponse.text()).toBe('hello');
+});
+
 it('page.goBack during renderer-initiated navigation', async ({ page, server }) => {
   await page.goto(server.PREFIX + '/one-style.html');
   await page.goto(server.EMPTY_PAGE);