Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: Encode messages in mobile as byte arrays #3077

Merged
merged 4 commits into from
Feb 12, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ describe('WebViewMessageStream', () => {
expect(await responsePromise).toBe(555);

expect(mockWebViewA.injectJavaScript).toHaveBeenCalledWith(
`window.postMessage('eyJ0YXJnZXQiOiJiIiwiZGF0YSI6MTExfQ==')`,
`window.postMessage([123,34,116,97,114,103,101,116,34,58,34,98,34,44,34,100,97,116,97,34,58,34,83,89,78,34,125])`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the size increase not of concern? Does this work with very large Snap bundles etc.?

Copy link
Member Author

@FrederikBolding FrederikBolding Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually posting the message is slightly slower yes, but we save a lot of time encoding/decoding. Seems worth it even with the size increase.

);

// Inject { target: "foo", data: 111 }
mockWebViewA.injectJavaScript(
`window.postMessage('eyJ0YXJnZXQiOiJmb28iLCJkYXRhIjoxMTF9')`,
`window.postMessage([123,34,116,97,114,103,101,116,34,58,34,102,111,111,34,44,34,100,97,116,97,34,58,49,49,49,125])`,
);

const listener = jest.fn();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { PostMessageEvent } from '@metamask/post-message-stream';
import { BasePostMessageStream } from '@metamask/post-message-stream';
import { isValidStreamMessage } from '@metamask/post-message-stream/dist/utils';
import { logError } from '@metamask/snaps-utils';
import { assert, bytesToBase64, stringToBytes } from '@metamask/utils';
import { assert, stringToBytes } from '@metamask/utils';

export type WebViewInterface = {
injectJavaScript(js: string): void;
Expand Down Expand Up @@ -65,12 +65,11 @@ export class WebViewMessageStream extends BasePostMessageStream {
data,
});

// To prevent XSS, we base64 encode the message before injecting it.
// This adds significant performance overhead.
// TODO: Should we use mobile native base64 here?
const bytes = stringToBytes(json);
const base64 = bytesToBase64(bytes);
this.#webView.injectJavaScript(`window.postMessage('${base64}')`);
// To prevent XSS, we encode the message before injecting it.
// This adds significant performance overhead for larger messages.
const bytes = new Uint8Array(stringToBytes(json));

this.#webView.injectJavaScript(`window.postMessage([${bytes.toString()}])`);
}

private _onMessage(event: PostMessageEvent): void {
Expand Down
10 changes: 5 additions & 5 deletions packages/snaps-controllers/src/test-utils/webview.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
import { base64ToBytes, bytesToString } from '@metamask/utils';
import { bytesToString } from '@metamask/utils';

import { WebViewMessageStream } from '../services/webview/WebViewMessageStream';

/**
* Parses the injected JS and returns the decoded base64 payload.
* Parses the injected JS and returns the JSON payload.
*
* @param js - The injected JS.
* @returns The parsed base64 payload as a string.
* @returns The decoded JSON as a string.
*/
export function parseInjectedJS(js: string) {
const base64 = js.slice(20, -2);
const bytes = base64ToBytes(base64);
const byteString = js.slice(19, -1);
const bytes = new Uint8Array(JSON.parse(byteString));
return bytesToString(bytes);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { sleep } from '@metamask/snaps-utils/test-utils';
import { bytesToBase64, stringToBytes } from '@metamask/utils';
import { stringToBytes } from '@metamask/utils';

import { WebViewExecutorStream } from './WebViewExecutorStream';

Expand All @@ -8,9 +8,8 @@ describe('WebViewExecutorStream', () => {
const addEventListener = jest.fn();
const postMessage = jest.fn().mockImplementation((message) => {
const bytes = stringToBytes(message);
const base64 = bytesToBase64(bytes);
addEventListener.mock.calls.forEach(([_type, listener]) => {
setTimeout(() => listener({ data: base64 }));
setTimeout(() => listener({ data: Array.from(bytes) }));
});
});
const mockWindow = {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { PostMessageEvent } from '@metamask/post-message-stream';
import { BasePostMessageStream } from '@metamask/post-message-stream';
import { isValidStreamMessage } from '@metamask/post-message-stream/dist/utils';
import { base64ToBytes, bytesToString } from '@metamask/utils';
import { bytesToString } from '@metamask/utils';

type WebViewExecutorStreamArgs = {
name: string;
Expand Down Expand Up @@ -62,12 +62,12 @@ export class WebViewExecutorStream extends BasePostMessageStream {
}

private _onMessage(event: PostMessageEvent): void {
if (typeof event.data !== 'string') {
if (!Array.isArray(event.data)) {
return;
}

const bytes = base64ToBytes(event.data);
const message = JSON.parse(bytesToString(bytes));
const json = bytesToString(new Uint8Array(event.data as number[]));
const message = JSON.parse(json);

// Notice that we don't check targetWindow or targetOrigin here.
// This doesn't seem possible to do in RN.
Expand Down
Loading