[DO NOT MERGE] Race condition in configuration

[kokoc]

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• I have signed the Adobe Open Source CLA.
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[Copilot]

Pull Request Overview

This pull request addresses a race condition in the guest-host communication initialization by implementing a synchronization mechanism using a guest-ready message. The changes ensure the host waits for confirmation that the guest is fully initialized before marking the connection as ready.

• Introduces a new guest-ready postMessage from guest to host to signal completion of guest initialization
• Adds a promise-based wait mechanism in the host to ensure proper sequencing of connection establishment
• Updates the isReady() check to include both isLoaded and the new isGuestReady flag

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
packages/uix-host/src/port.ts	Implements message listener for guest-ready events, adds isGuestReady flag and cleanup logic, updates isReady() condition to require guest confirmation
packages/uix-guest/src/guest.ts	Sends guest-ready postMessage to parent window after completing connection initialization
packages/uix-core/src/types.ts	Adds guestready event type to GuestConnectionEvents union type

Comments suppressed due to low confidence (1)

packages/uix-host/src/port.ts:520

• Memory leak on error: If the connect() method throws an error (either from attachFrame on line 507 or guestReadyPromise on line 510), the message event listener registered on line 501 may not be properly cleaned up. While the timeout rejection on line 493 removes the listener, if attachFrame fails before the timeout, the listener remains registered. Consider wrapping the entire method in a try-catch block and ensuring cleanup of this.guestReadyMessageHandler in the catch block, or restructure to guarantee cleanup in all error scenarios.

  private async connect() {
    const serverFrame =
      this.runtimeContainer.ownerDocument.createElement("iframe");
    normalizeIframe(serverFrame);
    serverFrame.setAttribute("aria-hidden", "true");
    serverFrame.setAttribute("src", this.url.href);
    this.guestServerFrame = serverFrame;
    this.runtimeContainer.appendChild(serverFrame);
    if (this.logger) {
      this.logger.info(
        `Guest ${this.id} attached iframe of ${this.url.href}`,
        this
      );
    }

    // Set up promise to wait for guest-ready message
    const guestReadyPromise = new Promise<void>((resolve, reject) => {
      const handleMessage = (event: MessageEvent) => {
        // Check if this is a guest-ready message from our specific iframe
        if (
          event.data &&
          event.data.type === "guest-ready" &&
          event.source === serverFrame.contentWindow
        ) {
          console.log(
            `[Port ${this.id}] Received guest-ready from our iframe (guest: ${
              event.data.guestId || "unknown"
            })`
          );
          this.isGuestReady = true;
          if (this.logger) {
            this.logger.info(`Guest ${this.id} reported ready status`);
          }
          this.emit("guestready", { guestPort: this });

          // Clean up listener and resolve
          window.removeEventListener("message", handleMessage);
          clearTimeout(timeoutId);
          resolve();
        }
      };

      // Set up timeout in case guest never sends ready message
      const timeoutId = setTimeout(() => {
        window.removeEventListener("message", handleMessage);
        reject(
          new Error(
            `Guest ${this.id} did not send ready message within ${this.timeout}ms`
          )
        );
      }, this.timeout);

      window.addEventListener("message", handleMessage);

      // Store cleanup function
      this.guestReadyMessageHandler = handleMessage;
    });

    this.guestServer = await this.attachFrame<GuestProxyWrapper>(serverFrame);

    // Wait for guest to report ready before marking as loaded
    await guestReadyPromise;

    this.isLoaded = true;
    if (this.logger) {
      this.logger.info(
        `Guest ${this.id} established connection, received methods, and reported ready`,
        this.apis,
        this
      );
    }
  }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Race condition: The guest-ready message listener is set up after the iframe is appended to the DOM and its src is set (line 456). This creates a race condition where the guest iframe could load and send its guest-ready message before the event listener is registered on line 501. The listener should be registered before appending the iframe to the DOM to ensure it's ready to receive the message. Move lines 464-505 (the listener setup) to before line 456 (where the iframe is appended).

[AmyJZhao]

Regarding whether await guestReadyPromise should be moved - No, it should stay where it is (after attachFrame). Here's why:
The guest sends the "guest-ready" message at the end of its _connect() method (line 328 in guest.ts)
This happens after the Penpal connection is established
So logically: attachFrame completes first, then the guest sends "guest-ready"
The correct fix is to move only the promise creation (which registers the listener) before appending the iframe:

this.guestServerFrame = serverFrame;
    
// Set up promise BEFORE appending iframe
const guestReadyPromise = new Promise<void>((resolve, reject) => {
  const handleMessage = (event: MessageEvent) => {
    // ... handler code ...
  };
  
  const timeoutId = setTimeout(() => {
    // ... timeout code ...
  }, this.timeout);
  
  window.addEventListener("message", handleMessage);
  this.guestReadyMessageHandler = handleMessage;
});

// NOW append the iframe - listener is ready
this.runtimeContainer.appendChild(serverFrame);

if (this.logger) {
  this.logger.info(`Guest ${this.id} attached iframe of ${this.url.href}`, this);
}

// Wait for Penpal connection
this.guestServer = await this.attachFrame<GuestProxyWrapper>(serverFrame);

// Then wait for guest-ready message (stays here!)
await guestReadyPromise;

this.isLoaded = true;

[Copilot]

This console.log statement should use the existing logger instance (this.logger) for consistency with the rest of the codebase. Replace with this.logger?.log(...) to maintain consistent logging behavior.

[Copilot]

Potential resource leak: If guestReadyPromise is rejected due to timeout but this.guestServer = await this.attachFrame<GuestProxyWrapper>(serverFrame) (line 507) also rejects with an error, the timeout will not be cleared. The timeout should be cleared in a finally block or the promise should be structured to handle all error paths. Consider wrapping the entire connection logic in a try-catch and clearing the timeout in the error handler.

[Copilot]

The cleanup in unload() doesn't reset the isGuestReady flag. If the port is reloaded after being unloaded, the isGuestReady flag will remain true, causing incorrect behavior. Add this.isGuestReady = false; after line 350 to properly reset the state.

Suggested change

	this.guestReadyMessageHandler = null;
	this.guestReadyMessageHandler = null;
	this.isGuestReady = false;

[Copilot]

Missing origin validation: The message event handler should validate that event.origin matches the expected guest origin (this.url.origin) to prevent malicious iframes from other origins sending fake guest-ready messages. Add a check: event.origin === this.url.origin in the condition on line 468.

Suggested change

	event.source === serverFrame.contentWindow
	event.source === serverFrame.contentWindow &&
	event.origin === this.url.origin

[Copilot]

Spelling error: "al" should be "all".

Suggested change

	* True when al extensions have loaded.
	* True when all extensions have loaded.

[Copilot]

Security issue: Using "*" as the targetOrigin allows any origin to receive the message, which could leak sensitive information (the guestId). Instead, use the parent's origin or a specific trusted origin. Consider using window.parent.location.origin or getting the expected origin from the configuration.

Suggested change

	window.parent.postMessage({ type: "guest-ready", guestId: this.id }, "*");
	// Use configuration parentOrigin if available, otherwise use window.parent.location.origin
	const parentOrigin =
	(this.configuration && this.configuration.parentOrigin) ||
	(window.parent.location && window.parent.location.origin);
	if (parentOrigin) {
	window.parent.postMessage({ type: "guest-ready", guestId: this.id }, parentOrigin);
	} else {
	this.logger.warn("Unable to determine parent origin for postMessage. Message not sent.");
	}

[lukem235]

Connection is already established like that:
const hostConnectionPromise = connectParentWindow<HostConnection>( { targetOrigin: "*", timeout: this.timeout, logger: this.logger, }, this.getLocalMethods() );

[AmyJZhao]

I'll test these review comments on my own branch test-fixes