From 557ec24cfc084440197da67581bf9782f10eb346 Mon Sep 17 00:00:00 2001
From: Nikolay Vitkov <34244704+Lightning00Blade@users.noreply.github.com>
Date: Wed, 24 May 2023 14:42:08 +0200
Subject: [PATCH] fix: stacktraces should not throw errors (#10231)

---
 packages/puppeteer-core/src/common/util.ts | 10 ++++---
 test/TestExpectations.json                 | 28 ++++++------------
 test/src/stacktrace.spec.ts                | 33 ++++++++++++++++------
 3 files changed, 38 insertions(+), 33 deletions(-)

diff --git a/packages/puppeteer-core/src/common/util.ts b/packages/puppeteer-core/src/common/util.ts
index bde06680e4a..c6390026cf9 100644
--- a/packages/puppeteer-core/src/common/util.ts
+++ b/packages/puppeteer-core/src/common/util.ts
@@ -50,8 +50,9 @@ export function createEvaluationError(
     name = 'Error';
     message = details.text;
   } else if (
-    details.exception.type !== 'object' ||
-    details.exception.subtype !== 'error'
+    (details.exception.type !== 'object' ||
+      details.exception.subtype !== 'error') &&
+    !details.exception.objectId
   ) {
     return valueFromRemoteObject(details.exception);
   } else {
@@ -110,8 +111,9 @@ export function createClientError(
     name = 'Error';
     message = details.text;
   } else if (
-    details.exception.type !== 'object' ||
-    details.exception.subtype !== 'error'
+    (details.exception.type !== 'object' ||
+      details.exception.subtype !== 'error') &&
+    !details.exception.objectId
   ) {
     return valueFromRemoteObject(details.exception);
   } else {
diff --git a/test/TestExpectations.json b/test/TestExpectations.json
index 12abd918aff..85445556b06 100644
--- a/test/TestExpectations.json
+++ b/test/TestExpectations.json
@@ -587,6 +587,12 @@
     "parameters": ["chrome", "webDriverBiDi"],
     "expectations": ["FAIL"]
   },
+  {
+    "testIdPattern": "[stacktrace.spec] Stack trace *",
+    "platforms": ["darwin", "linux", "win32"],
+    "parameters": ["cdp", "firefox"],
+    "expectations": ["FAIL"]
+  },
   {
     "testIdPattern": "[TargetManager.spec] *",
     "platforms": ["darwin", "linux", "win32"],
@@ -2220,28 +2226,10 @@
     "expectations": ["SKIP"]
   },
   {
-    "testIdPattern": "[stacktrace.spec] Stack trace should work",
+    "testIdPattern": "[stacktrace.spec] Stack trace should work for none error objects",
     "platforms": ["darwin", "linux", "win32"],
     "parameters": ["cdp", "firefox"],
-    "expectations": ["FAIL"]
-  },
-  {
-    "testIdPattern": "[stacktrace.spec] Stack trace should work with contiguous evaluation",
-    "platforms": ["darwin", "linux", "win32"],
-    "parameters": ["cdp", "firefox"],
-    "expectations": ["FAIL"]
-  },
-  {
-    "testIdPattern": "[stacktrace.spec] Stack trace should work with handles",
-    "platforms": ["darwin", "linux", "win32"],
-    "parameters": ["cdp", "firefox"],
-    "expectations": ["FAIL"]
-  },
-  {
-    "testIdPattern": "[stacktrace.spec] Stack trace should work with nested function calls",
-    "platforms": ["darwin", "linux", "win32"],
-    "parameters": ["cdp", "firefox"],
-    "expectations": ["FAIL"]
+    "expectations": ["PASS"]
   },
   {
     "testIdPattern": "[target.spec] Target Browser.waitForTarget should wait for a target",
diff --git a/test/src/stacktrace.spec.ts b/test/src/stacktrace.spec.ts
index 288f29b9131..c26ebf1a002 100644
--- a/test/src/stacktrace.spec.ts
+++ b/test/src/stacktrace.spec.ts
@@ -23,6 +23,7 @@ import {
   setupTestBrowserHooks,
   setupTestPageAndContextHooks,
 } from './mocha-utils.js';
+import {waitEvent} from './utils.js';
 
 const FILENAME = __filename.replace(/[/\-\\^$*+?.()|[\]{}]/g, '\\$&');
 
@@ -48,7 +49,7 @@ describe('Stack trace', function () {
     expect(error.stack.split('\n    at ').slice(0, 2)).toMatchObject({
       ...[
         'Error: Test',
-        'evaluate (evaluate at Context.<anonymous> (<filename>:31:14), <anonymous>:1:18)',
+        'evaluate (evaluate at Context.<anonymous> (<filename>:32:14), <anonymous>:1:18)',
       ],
     });
   });
@@ -71,7 +72,7 @@ describe('Stack trace', function () {
     expect(error.stack.split('\n    at ').slice(0, 2)).toMatchObject({
       ...[
         'Error: Test',
-        'evaluateHandle (evaluateHandle at Context.<anonymous> (<filename>:51:14), <anonymous>:1:18)',
+        'evaluateHandle (evaluateHandle at Context.<anonymous> (<filename>:52:14), <anonymous>:1:18)',
       ],
     });
   });
@@ -99,8 +100,8 @@ describe('Stack trace', function () {
     expect(error.stack.split('\n    at ').slice(0, 3)).toMatchObject({
       ...[
         'Error: Test',
-        'evaluateHandle (evaluateHandle at Context.<anonymous> (<filename>:70:36), <anonymous>:2:22)',
-        'evaluate (evaluate at Context.<anonymous> (<filename>:76:14), <anonymous>:1:12)',
+        'evaluateHandle (evaluateHandle at Context.<anonymous> (<filename>:71:36), <anonymous>:2:22)',
+        'evaluate (evaluate at Context.<anonymous> (<filename>:77:14), <anonymous>:1:12)',
       ],
     });
   });
@@ -135,12 +136,26 @@ describe('Stack trace', function () {
     expect(error.stack.split('\n    at ').slice(0, 6)).toMatchObject({
       ...[
         'Error: Test',
-        'a (evaluate at Context.<anonymous> (<filename>:97:14), <anonymous>:2:22)',
-        'b (evaluate at Context.<anonymous> (<filename>:97:14), <anonymous>:5:16)',
-        'c (evaluate at Context.<anonymous> (<filename>:97:14), <anonymous>:8:16)',
-        'd (evaluate at Context.<anonymous> (<filename>:97:14), <anonymous>:11:16)',
-        'evaluate (evaluate at Context.<anonymous> (<filename>:97:14), <anonymous>:13:12)',
+        'a (evaluate at Context.<anonymous> (<filename>:98:14), <anonymous>:2:22)',
+        'b (evaluate at Context.<anonymous> (<filename>:98:14), <anonymous>:5:16)',
+        'c (evaluate at Context.<anonymous> (<filename>:98:14), <anonymous>:8:16)',
+        'd (evaluate at Context.<anonymous> (<filename>:98:14), <anonymous>:11:16)',
+        'evaluate (evaluate at Context.<anonymous> (<filename>:98:14), <anonymous>:13:12)',
       ],
     });
   });
+
+  it('should work for none error objects', async () => {
+    const {page} = getTestState();
+
+    const [error] = await Promise.all([
+      waitEvent<Error>(page, 'pageerror'),
+      page.evaluate(() => {
+        // This can happen when a 404 with HTML is returned
+        void Promise.reject(new Response());
+      }),
+    ]);
+
+    expect(error).toBeTruthy();
+  });
 });