From 7dbc543b13c632acf9d18f322c44908ef94123c9 Mon Sep 17 00:00:00 2001
From: Shane Caraveo <scaraveo@mozilla.com>
Date: Tue, 17 Mar 2020 22:01:43 +0000
Subject: [PATCH] Bug 1621491 fix activityLog api by passing alreadyLogged
 where necessary r=rpl

Differential Revision: https://phabricator.services.mozilla.com/D66349

--HG--
extra : moz-landing-system : lando
---
 .../components/extensions/ExtensionChild.jsm  | 11 ++-
 .../components/extensions/ExtensionCommon.jsm |  3 +
 .../components/extensions/ExtensionParent.jsm | 29 ++++--
 .../test/mochitest/test_ext_activityLog.html  | 91 +++++++++++++++----
 4 files changed, 107 insertions(+), 27 deletions(-)

diff --git a/toolkit/components/extensions/ExtensionChild.jsm b/toolkit/components/extensions/ExtensionChild.jsm
index 9fab72bec7df..4e62980f67df 100644
--- a/toolkit/components/extensions/ExtensionChild.jsm
+++ b/toolkit/components/extensions/ExtensionChild.jsm
@@ -1064,11 +1064,13 @@ class ProxyAPIImplementation extends SchemaAPIInterface {
    *     `name` member. This may contain dots, e.g. "storage.local".
    * @param {string} name The name of the method or property.
    * @param {ChildAPIManager} childApiManager The owner of this implementation.
+   * @param {boolean} alreadyLogged Whether the child already logged the event.
    */
-  constructor(namespace, name, childApiManager) {
+  constructor(namespace, name, childApiManager, alreadyLogged = false) {
     super();
     this.path = `${namespace}.${name}`;
     this.childApiManager = childApiManager;
+    this.alreadyLogged = alreadyLogged;
   }
 
   revoke() {
@@ -1098,7 +1100,8 @@ class ProxyAPIImplementation extends SchemaAPIInterface {
     return this.childApiManager.callParentAsyncFunction(
       this.path,
       args,
-      callback
+      callback,
+      { alreadyLogged: this.alreadyLogged }
     );
   }
 
@@ -1120,6 +1123,7 @@ class ProxyAPIImplementation extends SchemaAPIInterface {
       listenerId: id,
       path: this.path,
       args,
+      alreadyLogged: this.alreadyLogged,
     });
   }
 
@@ -1139,6 +1143,7 @@ class ProxyAPIImplementation extends SchemaAPIInterface {
       childId: this.childApiManager.id,
       listenerId: id,
       path: this.path,
+      alreadyLogged: this.alreadyLogged,
     });
   }
 
@@ -1353,7 +1358,7 @@ class ChildAPIManager {
     let name = path.pop();
     let namespace = path.join(".");
 
-    let impl = new ProxyAPIImplementation(namespace, name, this);
+    let impl = new ProxyAPIImplementation(namespace, name, this, true);
     return {
       addListener: (listener, ...args) => impl.addListener(listener, args),
       removeListener: listener => impl.removeListener(listener),
diff --git a/toolkit/components/extensions/ExtensionCommon.jsm b/toolkit/components/extensions/ExtensionCommon.jsm
index 7b664063023f..de6c46e4d4f2 100644
--- a/toolkit/components/extensions/ExtensionCommon.jsm
+++ b/toolkit/components/extensions/ExtensionCommon.jsm
@@ -2535,6 +2535,9 @@ class EventManager {
     if (!this.unregister.has(callback)) {
       return;
     }
+    this.context.logActivity("api_call", `${this.name}.removeListener`, {
+      args: [],
+    });
 
     let unregister = this.unregister.get(callback);
     this.unregister.delete(callback);
diff --git a/toolkit/components/extensions/ExtensionParent.jsm b/toolkit/components/extensions/ExtensionParent.jsm
index 37b3cde3ce31..00024d406b6f 100644
--- a/toolkit/components/extensions/ExtensionParent.jsm
+++ b/toolkit/components/extensions/ExtensionParent.jsm
@@ -1142,7 +1142,7 @@ ParentAPIManager = {
       throw new Error("Got message on unexpected message manager");
     }
 
-    let { childId } = data;
+    let { childId, alreadyLogged = false } = data;
     let handlingUserInput = false;
 
     let listener = async (...listenerArgs) => {
@@ -1187,13 +1187,15 @@ ParentAPIManager = {
       handlingUserInput = true;
     }
     handler.addListener(listener, ...args);
-    ExtensionActivityLog.log(
-      context.extension.id,
-      context.viewType,
-      "api_call",
-      `${data.path}.addListener`,
-      { args }
-    );
+    if (!alreadyLogged) {
+      ExtensionActivityLog.log(
+        context.extension.id,
+        context.viewType,
+        "api_call",
+        `${data.path}.addListener`,
+        { args }
+      );
+    }
   },
 
   async recvRemoveListener(data) {
@@ -1202,6 +1204,17 @@ ParentAPIManager = {
 
     let handler = await context.apiCan.asyncFindAPIPath(data.path);
     handler.removeListener(listener);
+
+    let { alreadyLogged = false } = data;
+    if (!alreadyLogged) {
+      ExtensionActivityLog.log(
+        context.extension.id,
+        context.viewType,
+        "api_call",
+        `${data.path}.removeListener`,
+        { args: [] }
+      );
+    }
   },
 
   getContextById(childId) {
diff --git a/toolkit/components/extensions/test/mochitest/test_ext_activityLog.html b/toolkit/components/extensions/test/mochitest/test_ext_activityLog.html
index fd661094b8d5..c9039381e9ae 100644
--- a/toolkit/components/extensions/test/mochitest/test_ext_activityLog.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_activityLog.html
@@ -12,9 +12,9 @@
 <script type="text/javascript">
 "use strict";
 
-
 add_task(async function test_api() {
-  let URL = "http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_sample.html";
+  let URL =
+    "http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_sample.html";
 
   // Test that an unspecified extension is not logged by the watcher extension.
   let unlogged = ExtensionTestUtils.loadExtension({
@@ -40,7 +40,14 @@ add_task(async function test_api() {
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       applications: { gecko: { id: "watched@tests.mozilla.org" } },
-      permissions: ["webRequest", "webRequestBlocking", "<all_urls>"],
+      permissions: [
+        "tabs",
+        "tabHide",
+        "storage",
+        "webRequest",
+        "webRequestBlocking",
+        "<all_urls>",
+      ],
       content_scripts: [
         {
           matches: ["http://mochi.test/*/file_sample.html"],
@@ -58,6 +65,7 @@ add_task(async function test_api() {
       },
     },
     async background() {
+      let listen = () => {};
       async function runTest() {
         // Test activity for a child function call.
         browser.test.assertEq(
@@ -67,13 +75,16 @@ add_task(async function test_api() {
         );
 
         // Test a child event manager.
-        browser.test.onMessage.addListener(async msg => {});
+        browser.storage.onChanged.addListener(listen);
+        browser.storage.onChanged.removeListener(listen);
 
         // Test a parent event manager.
+        let webRequestListener = details => {
+          browser.webRequest.onBeforeRequest.removeListener(webRequestListener);
+          return { cancel: false };
+        };
         browser.webRequest.onBeforeRequest.addListener(
-          details => {
-            return { cancel: false };
-          },
+          webRequestListener,
           { urls: ["http://mochi.test/*/file_sample.html"] },
           ["blocking"]
         );
@@ -87,7 +98,7 @@ add_task(async function test_api() {
         });
         browser.test.sendMessage("ready");
       }
-      browser.test.onMessage.addListener(msg => {
+      browser.test.onMessage.addListener((msg, data) => {
         // Logging has started here so this listener is logged, but the
         // call adding it was not.  We do an additional onMessage.addListener
         // call in the test function to validate child based event managers.
@@ -95,6 +106,9 @@ add_task(async function test_api() {
           browser.test.assertTrue(true, msg);
           runTest();
         }
+        if (msg == "hideTab") {
+          browser.tabs.hide(data);
+        }
       });
       browser.test.sendMessage("url", browser.extension.getURL(""));
     },
@@ -120,7 +134,14 @@ add_task(async function test_api() {
       // Test child addListener calls.
       {
         type: "api_call",
-        name: "test.onMessage.addListener",
+        name: "storage.onChanged.addListener",
+        data: {
+          args: [],
+        },
+      },
+      {
+        type: "api_call",
+        name: "storage.onChanged.removeListener",
         data: {
           args: [],
         },
@@ -177,7 +198,24 @@ add_task(async function test_api() {
         name: "test.sendMessage",
         data: { args: ["ready"] },
       },
+      {
+        type: "api_call",
+        name: "tabs.hide",
+        data: { args: ["__TAB_ID"] },
+      },
+      {
+        type: "api_event",
+        name: "test.onMessage",
+        data: { args: ["hideTab", "__TAB_ID"] },
+      },
       // Test parent api_event calls.
+      {
+        type: "api_call",
+        name: "webRequest.onBeforeRequest.removeListener",
+        data: {
+          args: [],
+        },
+      },
       {
         type: "api_event",
         name: "webRequest.onBeforeRequest",
@@ -229,18 +267,24 @@ add_task(async function test_api() {
     browser.test.assertTrue(browser.activityLog, "activityLog is privileged");
 
     let tab;
-    browser.activityLog.onExtensionActivity.addListener(async details => {
+    let handler = async details => {
       browser.test.log(`onExtensionActivity ${JSON.stringify(details)}`);
       let test = expecting.shift();
       if (!test) {
         browser.test.notifyFail(`no test for ${details.name}`);
       }
       delete details.timeStamp;
-      // On multiple runs, tabId will be different.  Get the current tabId and
-      // use that.
+
+      // On multiple runs, tabId will be different.  Set the current
+      // tabId where we need it.
       if (test.data.tabId !== undefined) {
         test.data.tabId = tab.id;
       }
+      if (test.data.args !== undefined) {
+        test.data.args = test.data.args.map(value =>
+          value === "__TAB_ID" ? tab.id : value
+        );
+      }
 
       // hack for webRequest test
       if (details.name === "webRequest.onBeforeRequest") {
@@ -272,11 +316,22 @@ add_task(async function test_api() {
         await browser.tabs.remove(tab.id);
         browser.test.notifyPass("activity");
       }
-    }, "watched@tests.mozilla.org");
+    };
+    browser.activityLog.onExtensionActivity.addListener(
+      handler,
+      "watched@tests.mozilla.org"
+    );
 
     browser.test.onMessage.addListener(async msg => {
       if (msg === "opentab") {
-        tab = await browser.tabs.create({url: expectedUrl});
+        tab = await browser.tabs.create({ url: expectedUrl });
+        browser.test.sendMessage("tabid", tab.id);
+      }
+      if (msg === "done") {
+        browser.activityLog.onExtensionActivity.removeListener(
+          handler,
+          "watched@tests.mozilla.org"
+        );
       }
     });
   }
@@ -296,6 +351,8 @@ add_task(async function test_api() {
   extension.sendMessage("runtest");
   await extension.awaitMessage("ready");
   logger.sendMessage("opentab");
+  let id = await logger.awaitMessage("tabid");
+  extension.sendMessage("hideTab", id);
 
   await Promise.all([
     extension.awaitMessage("content_script"),
@@ -303,12 +360,14 @@ add_task(async function test_api() {
     logger.awaitFinish("activity"),
   ]);
 
+  // Stop watching because we get extra calls on extension shutdown
+  // such as listener removal.
+  logger.sendMessage("done");
+
   await extension.unload();
   await unlogged.unload();
   await logger.unload();
 });
-
-
 </script>
 
 </body>