Bug 1731556 - [remote] Do not broadcast already existing session data items r=whimboo,jgraham,webdriver-reviewers

Depends on D128284

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

remote/shared/messagehandler/RootMessageHandler.jsm

@@ -85,17 +85,23 @@ class RootMessageHandler extends MessageHandler {
    */
   addSessionData(sessionData = {}) {
     const { moduleName, category, contextDescriptor, values } = sessionData;
-    this._sessionData.addSessionData(
+    const addedValues = this._sessionData.addSessionData(
       moduleName,
       category,
       contextDescriptor,
       values
     );
+
+    if (addedValues.length == 0) {
+      // Avoid unnecessary broadcast if no value was added.
+      return [];
+    }
+
     return this.handleCommand({
       moduleName,
       commandName: "_applySessionData",
       params: {
-        values,
+        values: addedValues,
         category,
       },
       destination: {

remote/shared/messagehandler/sessiondata/SessionData.jsm

@@ -123,8 +123,11 @@ class SessionData {
    *     values.
    * @param {Array<(string|number|boolean)>} values
    *     Array of session data item values.
+   * @return {Array<(string|number|boolean)>}
+   *     The subset of values actually added to the session data.
    */
   addSessionData(moduleName, category, contextDescriptor, values) {
+    const addedValues = [];
     for (const value of values) {
       const item = { moduleName, category, contextDescriptor, value };
 
@@ -132,6 +135,7 @@ class SessionData {
       if (!hasItem) {
         // This is a new data item, create it and add it to the data.
         this._data.push(item);
+        addedValues.push(value);
       } else {
         logger.warn(
           `Duplicated session data item was not added: ${JSON.stringify(item)}`
@@ -141,6 +145,8 @@ class SessionData {
 
     // Persist the sessionDataMap.
     this._persist();
+
+    return addedValues;
   }
 
   destroy() {
@@ -193,8 +199,11 @@ class SessionData {
    *     values.
    * @param {Array<(string|number|boolean)>} values
    *     Array of session data item values.
+   * @return {Array<(string|number|boolean)>}
+   *     The subset of values actually removed from the session data.
    */
   removeSessionData(moduleName, category, contextDescriptor, values) {
+    const removedValues = [];
     // Remove the provided context from the contexts Map of the provided items.
     for (const value of values) {
       const item = { moduleName, category, contextDescriptor, value };
@@ -205,6 +214,7 @@ class SessionData {
       if (itemIndex != -1) {
         // The item was found in the session data, remove it.
         this._data.splice(itemIndex, 1);
+        removedValues.push(value);
       } else {
         logger.warn(
           `Missing session data item was not removed: ${JSON.stringify(item)}`
@@ -214,6 +224,8 @@ class SessionData {
 
     // Persist the sessionDataMap.
     this._persist();
+
+    return removedValues;
   }
 
   _isSameItem(item1, item2) {

remote/shared/messagehandler/test/browser/browser.ini

@@ -12,3 +12,4 @@ prefs =
 [browser_handle_simple_command.js]
 [browser_registry.js]
 [browser_session_data.js]
+[browser_session_data_broadcast.js]

remote/shared/messagehandler/test/browser/browser_session_data_broadcast.js

@@ -0,0 +1,77 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const { RootMessageHandler } = ChromeUtils.import(
+  "chrome://remote/content/shared/messagehandler/RootMessageHandler.jsm"
+);
+
+const TEST_PAGE = "https://example.com/document-builder.sjs?html=tab";
+
+add_task(async function test_session_data_broadcast() {
+  const tab1 = BrowserTestUtils.addTab(gBrowser, TEST_PAGE);
+  await BrowserTestUtils.browserLoaded(tab1.linkedBrowser);
+  const browsingContext1 = tab1.linkedBrowser.browsingContext;
+
+  const root = createRootMessageHandler("session-id-event");
+
+  info("Add a new session data item, expect one return value");
+  const value1 = await sendAddSessionData(root, ["text-1"]);
+  is(value1.length, 1);
+  is(value1[0].newData, "text-1");
+  is(value1[0].sessionData, "text-1");
+  is(value1[0].contextId, browsingContext1.id);
+
+  info("Add two session data items, expect one return value with both items");
+  const value2 = await sendAddSessionData(root, ["text-2", "text-3"]);
+  is(value2.length, 1);
+  is(value2[0].newData, "text-2, text-3");
+  is(value2[0].sessionData, "text-1, text-2, text-3");
+  is(value2[0].contextId, browsingContext1.id);
+
+  info("Try to add an existing data item, expect no return value");
+  const value3 = await sendAddSessionData(root, ["text-1"]);
+  is(value3.length, 0);
+
+  info("Add an existing and a new item, expect only the new item to return");
+  const value4 = await sendAddSessionData(root, ["text-1", "text-4"]);
+  is(value4.length, 1);
+  is(value4[0].newData, "text-4");
+  is(value4[0].sessionData, "text-1, text-2, text-3, text-4");
+  is(value4[0].contextId, browsingContext1.id);
+
+  info("Open a new tab on the same test URL");
+  const tab2 = await addTab(TEST_PAGE);
+  const browsingContext2 = tab2.linkedBrowser.browsingContext;
+
+  info("Add a new session data item, check both contexts have the same data.");
+  const value5 = await sendAddSessionData(root, ["text-5"]);
+  is(value5.length, 2);
+  is(value5[0].newData, "text-5");
+  is(value5[0].sessionData, "text-1, text-2, text-3, text-4, text-5");
+  is(value5[0].contextId, browsingContext1.id);
+  is(value5[1].newData, "text-5");
+  // "text-1, text-2, text-3, text-4" were added as initial session data and
+  // "text-5" was added afterwards.
+  is(value5[1].sessionData, "text-1, text-2, text-3, text-4, text-5");
+  is(value5[1].contextId, browsingContext2.id);
+
+  root.destroy();
+
+  gBrowser.removeTab(tab1);
+  gBrowser.removeTab(tab2);
+});
+
+function sendAddSessionData(rootMessageHandler, values) {
+  return rootMessageHandler.handleCommand({
+    moduleName: "command",
+    commandName: "testAddSessionData",
+    destination: {
+      type: RootMessageHandler.type,
+    },
+    params: {
+      values,
+    },
+  });
+}

remote/shared/messagehandler/test/browser/resources/modules/root/command.jsm

@@ -6,6 +6,10 @@
 
 const EXPORTED_SYMBOLS = ["command"];
 
+const { CONTEXT_DESCRIPTOR_TYPES } = ChromeUtils.import(
+  "chrome://remote/content/shared/messagehandler/MessageHandler.jsm"
+);
+
 const { Module } = ChromeUtils.import(
   "chrome://remote/content/shared/messagehandler/Module.jsm"
 );
@@ -17,6 +21,17 @@ class Command extends Module {
    * Commands
    */
 
+  testAddSessionData(params) {
+    return this.messageHandler.addSessionData({
+      moduleName: "command",
+      category: "testCategory",
+      contextDescriptor: {
+        type: CONTEXT_DESCRIPTOR_TYPES.ALL,
+      },
+      values: params.values,
+    });
+  }
+
   testRootModule() {
     return "root-value";
   }

remote/shared/messagehandler/test/browser/resources/modules/windowglobal/command.jsm

@@ -11,12 +11,31 @@ const { Module } = ChromeUtils.import(
 );
 
 class Command extends Module {
+  constructor(messageHandler) {
+    super(messageHandler);
+    this._testCategorySessionData = [];
+  }
   destroy() {}
 
   /**
    * Commands
    */
 
+  _applySessionData(params) {
+    if (params.category === "testCategory") {
+      this._testCategorySessionData = this._testCategorySessionData.concat(
+        params.values
+      );
+      return {
+        newData: params.values.join(", "),
+        sessionData: this._testCategorySessionData.join(", "),
+        contextId: this.messageHandler.contextId,
+      };
+    }
+
+    return {};
+  }
+
   testWindowGlobalModule() {
     return "windowglobal-value";
   }

remote/shared/messagehandler/test/xpcshell/test_SessionData.js

@@ -22,7 +22,11 @@ add_task(async function test_sessionData() {
   const otherContext = { type: "other-type", id: "some-id" };
 
   info("Add a first event for the global context");
-  sessionData.addSessionData("mod", "event", globalContext, ["first.event"]);
+  let addedValues = sessionData.addSessionData("mod", "event", globalContext, [
+    "first.event",
+  ]);
+  equal(addedValues.length, 1, "One value added");
+  equal(addedValues[0], "first.event", "Expected value was added");
   checkEvents(sessionData.getSessionData("mod", "event"), [
     {
       value: "first.event",
@@ -31,7 +35,10 @@ add_task(async function test_sessionData() {
   ]);
 
   info("Add the exact same data (same module, type, context, value)");
-  sessionData.addSessionData("mod", "event", globalContext, ["first.event"]);
+  addedValues = sessionData.addSessionData("mod", "event", globalContext, [
+    "first.event",
+  ]);
+  equal(addedValues.length, 0, "No new value added");
   checkEvents(sessionData.getSessionData("mod", "event"), [
     {
       value: "first.event",
@@ -40,7 +47,11 @@ add_task(async function test_sessionData() {
   ]);
 
   info("Add another context for the same event");
-  sessionData.addSessionData("mod", "event", otherContext, ["first.event"]);
+  addedValues = sessionData.addSessionData("mod", "event", otherContext, [
+    "first.event",
+  ]);
+  equal(addedValues.length, 1, "One value added");
+  equal(addedValues[0], "first.event", "Expected value was added");
   checkEvents(sessionData.getSessionData("mod", "event"), [
     {
       value: "first.event",
@@ -53,7 +64,11 @@ add_task(async function test_sessionData() {
   ]);
 
   info("Add a second event for the global context");
-  sessionData.addSessionData("mod", "event", globalContext, ["second.event"]);
+  addedValues = sessionData.addSessionData("mod", "event", globalContext, [
+    "second.event",
+  ]);
+  equal(addedValues.length, 1, "One value added");
+  equal(addedValues[0], "second.event", "Expected value was added");
   checkEvents(sessionData.getSessionData("mod", "event"), [
     {
       value: "first.event",
@@ -70,10 +85,13 @@ add_task(async function test_sessionData() {
   ]);
 
   info("Add two events for the global context");
-  sessionData.addSessionData("mod", "event", globalContext, [
+  addedValues = sessionData.addSessionData("mod", "event", globalContext, [
     "third.event",
     "fourth.event",
   ]);
+  equal(addedValues.length, 2, "Two values added");
+  equal(addedValues[0], "third.event", "Expected value was added");
+  equal(addedValues[1], "fourth.event", "Expected value was added");
   checkEvents(sessionData.getSessionData("mod", "event"), [
     {
       value: "first.event",
@@ -98,11 +116,16 @@ add_task(async function test_sessionData() {
   ]);
 
   info("Remove the second, third and fourth events");
-  sessionData.removeSessionData("mod", "event", globalContext, [
-    "second.event",
-    "third.event",
-    "fourth.event",
-  ]);
+  let removedValues = sessionData.removeSessionData(
+    "mod",
+    "event",
+    globalContext,
+    ["second.event", "third.event", "fourth.event"]
+  );
+  equal(removedValues.length, 3, "Three values removed");
+  equal(removedValues[0], "second.event", "Expected value was removed");
+  equal(removedValues[1], "third.event", "Expected value was removed");
+  equal(removedValues[2], "fourth.event", "Expected value was removed");
   checkEvents(sessionData.getSessionData("mod", "event"), [
     {
       value: "first.event",
@@ -115,7 +138,11 @@ add_task(async function test_sessionData() {
   ]);
 
   info("Remove the global context from the first event");
-  sessionData.removeSessionData("mod", "event", globalContext, ["first.event"]);
+  removedValues = sessionData.removeSessionData("mod", "event", globalContext, [
+    "first.event",
+  ]);
+  equal(removedValues.length, 1, "One value removed");
+  equal(removedValues[0], "first.event", "Expected value was removed");
   checkEvents(sessionData.getSessionData("mod", "event"), [
     {
       value: "first.event",
@@ -125,6 +152,8 @@ add_task(async function test_sessionData() {
 
   info("Remove the other context from the first event");
   sessionData.removeSessionData("mod", "event", otherContext, ["first.event"]);
+  equal(removedValues.length, 1, "One value removed");
+  equal(removedValues[0], "first.event", "Expected value was removed");
   checkEvents(sessionData.getSessionData("mod", "event"), []);
 });
 

testing/web-platform/tests/webdriver/tests/bidi/log_entry_added/console.py

@@ -87,3 +87,36 @@ async def test_console_log_new_context(bidi_session,
     current_session.execute_script(f"console.log('text_after_refresh')")
     event_data = await on_entry_added
     assert event_data['text'] == 'text_after_refresh'
+
+
+@pytest.mark.asyncio
+async def test_console_log_subscribe_twice(bidi_session,
+                                           current_session,
+                                           wait_for_event):
+    # Subscribe to log.entryAdded twice and check that events are only received
+    # once.
+    await bidi_session.session.subscribe(events=["log.entryAdded"])
+    await bidi_session.session.subscribe(events=["log.entryAdded"])
+
+    # Track all received log.entryAdded events in the events array
+    events = []
+    async def on_event(method, data):
+        events.append(data)
+
+    remove_listener = bidi_session.add_event_listener("log.entryAdded", on_event)
+
+    on_entry_added = wait_for_event("log.entryAdded")
+    current_session.execute_script(f"console.log('text1')")
+    await on_entry_added
+    assert len(events) == 1;
+    assert events[0]['text'] == 'text1'
+
+    # Wait for another console log so that potential duplicates for the first
+    # log have time to be received.
+    on_entry_added = wait_for_event("log.entryAdded")
+    current_session.execute_script(f"console.log('text2')")
+    await on_entry_added
+    assert len(events) == 2;
+    assert events[1]['text'] == 'text2'
+
+    remove_listener()