From fe472aae2e9d7290ecb6e0545d44e7bfadd2fc51 Mon Sep 17 00:00:00 2001
From: Nicolas Chevobbe <nchevobbe@mozilla.com>
Date: Fri, 10 Jun 2022 15:55:07 +0000
Subject: [PATCH] Bug 1771428 - [devtools] Remove last statement
 AwaitExpression in mapTopLevelAwait. r=bomsy.

We use to transform expression like `await obj` into
```
(async () => {
  return await obj;
})()
``

So we were basically returning the expression, wrapped in an async iife, adding
a `return` statement before the last statement.
But in the case the last statement is an `AwaitExpression`, we can simply return
the argument, stripping the `await` keyword.
This fixes an issue when awaiting for an object with a `then` getter would execute
the getter twice.
A test case is added in mochitest, and unit test are updated to reflect the new
output.

Differential Revision: https://phabricator.services.mozilla.com/D148806
---
 .../client/debugger/bin/module-manifest.json    |  4 ++--
 devtools/client/debugger/dist/parser-worker.js  | 12 ++++++++++--
 .../src/workers/parser/mapAwaitExpression.js    | 14 ++++++++++----
 .../workers/parser/tests/mapExpression.spec.js  |  4 ++--
 .../test/browser/browser_jsterm_await.js        | 17 +++++++++++++++++
 5 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/devtools/client/debugger/bin/module-manifest.json b/devtools/client/debugger/bin/module-manifest.json
index 0d21f2b264c5..9b83e9ec986b 100644
--- a/devtools/client/debugger/bin/module-manifest.json
+++ b/devtools/client/debugger/bin/module-manifest.json
@@ -2445,7 +2445,7 @@
         "byName": {},
         "byBlocks": {},
         "usedIds": {
-          "1": 1
+          "0": 0
         }
       }
     }
@@ -2466,7 +2466,7 @@
         "byName": {},
         "byBlocks": {},
         "usedIds": {
-          "1": 1
+          "0": 0
         }
       }
     }
diff --git a/devtools/client/debugger/dist/parser-worker.js b/devtools/client/debugger/dist/parser-worker.js
index cef78d0104dd..d6fc98240af6 100644
--- a/devtools/client/debugger/dist/parser-worker.js
+++ b/devtools/client/debugger/dist/parser-worker.js
@@ -47826,8 +47826,16 @@ function translateDeclarationIntoAssignment(node) {
 
 function addReturnNode(ast) {
   const statements = ast.program.body;
-  const lastStatement = statements[statements.length - 1];
-  return statements.slice(0, -1).concat(t.returnStatement(lastStatement.expression));
+  const lastStatement = statements.pop(); // if the last expression is an awaitExpression, strip the `await` part and directly
+  // return the argument to avoid calling the argument's `then` function twice when the
+  // mapped expression gets evaluated (See Bug 1771428)
+
+  if (t.isAwaitExpression(lastStatement.expression)) {
+    lastStatement.expression = lastStatement.expression.argument;
+  }
+
+  statements.push(t.returnStatement(lastStatement.expression));
+  return statements;
 }
 
 function getDeclarations(node) {
diff --git a/devtools/client/debugger/src/workers/parser/mapAwaitExpression.js b/devtools/client/debugger/src/workers/parser/mapAwaitExpression.js
index 8f750cdb1f8e..fca97c8a32f0 100644
--- a/devtools/client/debugger/src/workers/parser/mapAwaitExpression.js
+++ b/devtools/client/debugger/src/workers/parser/mapAwaitExpression.js
@@ -39,10 +39,16 @@ function translateDeclarationIntoAssignment(node) {
  */
 function addReturnNode(ast) {
   const statements = ast.program.body;
-  const lastStatement = statements[statements.length - 1];
-  return statements
-    .slice(0, -1)
-    .concat(t.returnStatement(lastStatement.expression));
+  const lastStatement = statements.pop();
+
+  // if the last expression is an awaitExpression, strip the `await` part and directly
+  // return the argument to avoid calling the argument's `then` function twice when the
+  // mapped expression gets evaluated (See Bug 1771428)
+  if (t.isAwaitExpression(lastStatement.expression)) {
+    lastStatement.expression = lastStatement.expression.argument;
+  }
+  statements.push(t.returnStatement(lastStatement.expression));
+  return statements;
 }
 
 function getDeclarations(node) {
diff --git a/devtools/client/debugger/src/workers/parser/tests/mapExpression.spec.js b/devtools/client/debugger/src/workers/parser/tests/mapExpression.spec.js
index bbb845d2ecfa..839b366469a0 100644
--- a/devtools/client/debugger/src/workers/parser/tests/mapExpression.spec.js
+++ b/devtools/client/debugger/src/workers/parser/tests/mapExpression.spec.js
@@ -39,7 +39,7 @@ describe("mapExpression", () => {
     {
       name: "await",
       expression: "await a()",
-      newExpression: formatAwait("return await a()"),
+      newExpression: formatAwait("return a()"),
       bindings: [],
       mappings: {},
       shouldMapBindings: true,
@@ -78,7 +78,7 @@ describe("mapExpression", () => {
     {
       name: "await (multiple awaits)",
       expression: "const x = await a(); await b(x)",
-      newExpression: formatAwait("self.x = await a(); return await b(x);"),
+      newExpression: formatAwait("self.x = await a(); return b(x);"),
       bindings: [],
       mappings: {},
       shouldMapBindings: true,
diff --git a/devtools/client/webconsole/test/browser/browser_jsterm_await.js b/devtools/client/webconsole/test/browser/browser_jsterm_await.js
index eb55a7994b2d..c90fe6de7d13 100644
--- a/devtools/client/webconsole/test/browser/browser_jsterm_await.js
+++ b/devtools/client/webconsole/test/browser/browser_jsterm_await.js
@@ -64,4 +64,21 @@ add_task(async function() {
     `Promise {`
   );
   ok(message, "Promise are displayed as expected");
+
+  info("Check that then getters aren't called twice");
+  message = await executeAndWaitForResultMessage(
+    hud,
+    // It's important to keep the last statement of the expression as it covers the original issue.
+    // We could execute another expression to get `object.called`, but since we get a preview
+    // of the object with an accurate `called` value, this is enough.
+    `
+    var obj = {
+      called: 0,
+      get then(){
+        this.called++
+      }
+    };
+    await obj`,
+    `Object { called: 1, then: Getter }`
+  );
 });