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

#2902: Throw BusinessError on runBrick if tab is closed or has no access #2917

Merged
merged 6 commits into from
Mar 12, 2022
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
29 changes: 26 additions & 3 deletions src/background/executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,36 @@ import { runBrick } from "@/contentScript/messenger/api";
import { Target } from "@/types";
import { RemoteExecutionError } from "@/blocks/errors";
import pDefer from "p-defer";
import { canAccessTab } from "webext-tools";
import { onTabClose } from "@/chrome";

type TabId = number;

// Used to determine which promise was resolved in a race
const TYPE_WAS_CLOSED = Symbol("Tab was closed");

const tabToOpener = new Map<TabId, TabId>();
const tabToTarget = new Map<TabId, TabId>();
// TODO: One tab could have multiple targets, but `tabToTarget` currenly only supports one at a time

async function safelyRunBrick({ tabId }: { tabId: number }, request: RunBlock) {
if (!(await canAccessTab(tabId))) {
throw new BusinessError("PixieBrix doesn't have access to the tab");
}

const result = await Promise.race([
// If https://github.com/pixiebrix/webext-messenger/issues/67 is resolved, we don't need the listener
onTabClose(tabId).then(() => TYPE_WAS_CLOSED),
runBrick({ tabId }, request),
]);

if (result === TYPE_WAS_CLOSED) {
throw new BusinessError("The tab was closed");
}

return result;
}

export async function waitForTargetByUrl(url: string): Promise<Target> {
const { promise, resolve } = pDefer<Target>();

Expand Down Expand Up @@ -65,7 +88,7 @@ export async function requestRunInOpener(
tabId: tabToOpener.get(sourceTabId),
};
const subRequest = { ...request, sourceTabId };
return runBrick(opener, subRequest);
return safelyRunBrick(opener, subRequest);
}

export async function requestRunInBroadcast(
Expand All @@ -87,7 +110,7 @@ export async function requestRunInBroadcast(
}

try {
const response = runBrick({ tabId: tab.id }, subRequest);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't matter as much here, but I suppose wrapping the error still makes sense.

const response = safelyRunBrick({ tabId: tab.id }, subRequest);
fulfilled.set(tab.id, await response);
} catch (error) {
rejected.set(tab.id, error);
Expand All @@ -113,7 +136,7 @@ export async function requestRunInTarget(
}

const subRequest = { ...request, sourceTabId };
return runBrick({ tabId: target }, subRequest);
return safelyRunBrick({ tabId: target }, subRequest);
}

export async function openTab(
Expand Down
13 changes: 13 additions & 0 deletions src/chrome.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,16 @@ export async function setReduxStorage<T extends JsonValue = JsonValue>(
): Promise<void> {
await browser.storage.local.set({ [storageKey]: JSON.stringify(value) });
}

export async function onTabClose(watchedTabId: number): Promise<void> {
await new Promise<void>((resolve) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to unsubscribe the listener somehow after?

const listener = (closedTabId: number) => {
if (closedTabId === watchedTabId) {
resolve();
browser.tabs.onRemoved.removeListener(listener);
}
};

browser.tabs.onRemoved.addListener(listener);
});
}
4 changes: 3 additions & 1 deletion src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,11 @@ export class ContextError extends Error {
}
}

export const NO_TARGET_FOUND_CONNECTION_ERROR =
"Could not establish connection. Receiving end does not exist.";
/** Browser Messenger API error message patterns */
export const CONNECTION_ERROR_MESSAGES = [
"Could not establish connection. Receiving end does not exist.",
NO_TARGET_FOUND_CONNECTION_ERROR,
"Extension context invalidated.",
];

Expand Down