Fix Encoding Associated Interfaces as Function Args in Lite JS Bindings

Encoding associated interfaces passed as function call arguments relies
on the correct calculation of the dimensions of the paramStructSpec of
the said function call.

This is done in `computeStructDimensions` which internally calls
`computeDimensions` on each struct field.

The issue comes from `computeStructDimensions` assuming all calls to
`computeDimensions` return a numInterfaceIds which is not the case.

This fix makes sure to check if a numInterfaceIds is returned before
updating the `numInterfaceIds` for the paramStructSpec dimensions.

Change-Id: Iee4ae5d84c697450f800cf1c46aaa83bfb5ef5b6
Bug: N/A
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6272375
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Commit-Queue: Ali Hijazi <ahijazi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1421086}

content/test/data/mojo_bindings_web_test.test-mojom

@@ -29,13 +29,15 @@ interface TestMessageTarget {
   Echo(NestedEnum nested) => (NestedEnum nested);
 
   Deconstruct(TestStruct test_struct)
-       => (int32 x, int32 y,
-           // Using TestStruct.StructEnum causes a compile failure. Use
-           // int32 instead.
-           // TODO(crbug.com/40102018): Change |z| to TestStruct.StructEnum.
-           int32 z);
+      => (int32 x,
+          int32 y,
+          // Using TestStruct.StructEnum causes a compile failure. Use
+          // int32 instead.
+          // TODO(crbug.com/40102018): Change |z| to TestStruct.StructEnum.
+          int32 z);
 
   Flatten(array<TestStruct> values) => (array<int32> values);
+
   FlattenUnions(array<TestUnion> unions) => (array<int32> x, array<int32> s);
 
   RequestSubinterface(pending_receiver<Subinterface> receiver,
@@ -44,6 +46,7 @@ interface TestMessageTarget {
 
 interface Subinterface {
   Push(int32 value);
+
   Flush();
 };
 
@@ -54,8 +57,12 @@ interface SubinterfaceClient {
 struct StructVersionTest {
   string a;
   string b;
-  [MinVersion=1] string? c;
-  [MinVersion=2] string? d;
+
+  [MinVersion=1]
+  string? c;
+
+  [MinVersion=2]
+  string? d;
 };
 
 interface InterfaceVersionTest {
@@ -63,15 +70,25 @@ interface InterfaceVersionTest {
 };
 
 interface Counter {
-  // Two different varieties of observer addition to exercise sending remotes
+  // Three different varieties of observer addition to exercise sending remotes
   // and receiving receivers.
-  AddObserver(pending_associated_remote<CounterObserver> observer);
+  AddObserver1(pending_associated_remote<CounterObserver> observer);
+
+  // `unused` is mainly to ensure we correctly encode calls with
+  // associated interfaces alongside other parameters.
+  AddObserver2(
+      pending_associated_remote<CounterObserver> observer, int32 unused);
+
   AddNewObserver() => (pending_associated_receiver<CounterObserver> receiver);
+
   RemoveAllObservers();
 
-  // Two different varieties of cloning to exercise sending receivers and
+  // Three different varieties of cloning to exercise sending receivers and
   // receiving remotes.
-  Clone(pending_associated_receiver<Counter> receiver);
+  Clone1(pending_associated_receiver<Counter> receiver);
+
+  Clone2(pending_associated_receiver<Counter> receiver, int32 unused);
+
   CloneToNewRemote() => (pending_associated_remote<Counter> remote);
 
   // Increments the counter, notifies all observers, then replies. Because
@@ -82,5 +99,6 @@ interface Counter {
 
 interface CounterObserver {
   OnCountChanged(int32 count);
+
   OnCloneDisconnected();
 };

content/web_test/browser/web_test_content_browser_client.cc

@@ -202,11 +202,16 @@ class MojoWebTestCounterImpl : public mojo_bindings_test::mojom::Counter {
   }
 
   // mojo_bindings_test::mojom::Counter:
-  void AddObserver(
+  void AddObserver1(
       mojo::PendingAssociatedRemote<CounterObserver> observer) override {
     observers_.Add(std::move(observer));
   }
 
+  void AddObserver2(mojo::PendingAssociatedRemote<CounterObserver> observer,
+                    int unused) override {
+    observers_.Add(std::move(observer));
+  }
+
   void AddNewObserver(AddNewObserverCallback callback) override {
     mojo::PendingAssociatedRemote<CounterObserver> observer;
     std::move(callback).Run(observer.InitWithNewEndpointAndPassReceiver());
@@ -215,7 +220,12 @@ class MojoWebTestCounterImpl : public mojo_bindings_test::mojom::Counter {
 
   void RemoveAllObservers() override { observers_.Clear(); }
 
-  void Clone(mojo::PendingAssociatedReceiver<Counter> receiver) override {
+  void Clone1(mojo::PendingAssociatedReceiver<Counter> receiver) override {
+    additional_receivers_.Add(this, std::move(receiver));
+  }
+
+  void Clone2(mojo::PendingAssociatedReceiver<Counter> receiver,
+              int unused) override {
     additional_receivers_.Add(this, std::move(receiver));
   }
 

mojo/public/js/bindings_lite.js

@@ -220,7 +220,11 @@ mojo.internal.computeStructDimensions = function(structSpec, value) {
       const fieldDimensions =
           field.type.$.computeDimensions(fieldValue, field.nullable);
       size += mojo.internal.align(fieldDimensions.size, 8);
-      numInterfaceIds += fieldDimensions.numInterfaceIds;
+      // Only update numInterfaceIds if field.type.$.computeDimensions returns a
+      // numInterfaceIds
+      if (fieldDimensions.numInterfaceIds) {
+        numInterfaceIds += fieldDimensions.numInterfaceIds;
+      }
     } else if (field.type.$.hasInterfaceId) {
       numInterfaceIds++;
     }
@@ -260,7 +264,11 @@ mojo.internal.computeUnionDimensions = function(unionSpec, nullable, value) {
       const fieldDimensions =
           field['type'].$.computeDimensions(fieldValue, nullable);
       size += mojo.internal.align(fieldDimensions.size, 8);
-      numInterfaceIds += fieldDimensions.numInterfaceIds;
+      // Only update numInterfaceIds if field['type'].$.computeDimensions
+      // returns a numInterfaceIds
+      if (fieldDimensions.numInterfaceIds) {
+        numInterfaceIds += fieldDimensions.numInterfaceIds;
+      }
     } else if (field['type'].$.hasInterfaceId) {
       numInterfaceIds++;
     }

third_party/blink/web_tests/http/tests/mojo/associated-interfaces.html

@@ -57,8 +57,10 @@ promise_test(async () => {
   const observerB = new ObserverImpl;
   const receiverB = new CounterObserverReceiver(observerB);
 
-  counter.addObserver(receiverA.$.associateAndPassRemote());
-  counter.addObserver(receiverB.$.associateAndPassRemote());
+  counter.addObserver1(receiverA.$.associateAndPassRemote());
+  // This ensures we are able to encode associated interfaces
+  // alongside other arguments.
+  counter.addObserver2(receiverB.$.associateAndPassRemote(), 1234);
 
   counter.increment();
   const {count} = await counter.increment();
@@ -94,9 +96,11 @@ promise_test(async () => {
   const counterB = new CounterRemote;
   const counterC = new CounterRemote;
   const counterD = new CounterRemote;
-  counterA.clone(counterB.$.associateAndPassReceiver());
-  counterA.clone(counterC.$.associateAndPassReceiver());
-  counterB.clone(counterD.$.associateAndPassReceiver());
+  counterA.clone1(counterB.$.associateAndPassReceiver());
+  counterA.clone1(counterC.$.associateAndPassReceiver());
+  // This ensures we are able to encode associated interfaces
+  // alongside other arguments.
+  counterB.clone2(counterD.$.associateAndPassReceiver(), 1234);
 
   // Increment operations among the three interfaces should be strictly ordered.
   const increments = [
@@ -139,7 +143,7 @@ promise_test(async () => {
   const counter = getCounterRemote();
   const observer = new ObserverImpl;
   const receiver = new CounterObserverReceiver(observer);
-  counter.addObserver(receiver.$.associateAndPassRemote());
+  counter.addObserver1(receiver.$.associateAndPassRemote());
   counter.increment();
   counter.increment();
   counter.increment();
@@ -152,7 +156,7 @@ promise_test(async () => {
   const {remote: clone} = await counter.cloneToNewRemote();
   const observer = new ObserverImpl;
   const receiver = new CounterObserverReceiver(observer);
-  counter.addObserver(receiver.$.associateAndPassRemote());
+  counter.addObserver1(receiver.$.associateAndPassRemote());
   clone.$.close();
   observer.nextCloneDisconnect();
 }, 'closing an associated endpoint from JavaScript will signal its peer');
@@ -161,7 +165,7 @@ promise_test(async () => {
   const counter = getCounterRemote();
   const observer = new ObserverImpl;
   const receiver = new CounterObserverReceiver(observer);
-  counter.addObserver(receiver.$.associateAndPassRemote());
+  counter.addObserver1(receiver.$.associateAndPassRemote());
   counter.removeAllObservers();
   await waitForDisconnect(receiver);
 }, 'JavaScript associated endpoints are notified when their peers close');