From 9fe6c0430466c093faeb41d9d6f229bb8ce85a5d Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Thu, 10 Mar 2022 17:47:53 +0800 Subject: [PATCH 1/6] Throw BusinessError when the target tab is closed --- src/background/executor.ts | 24 ++++++++++++++++++++---- src/errors.ts | 4 +++- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/background/executor.ts b/src/background/executor.ts index 51e55b5923..d7607ecd10 100644 --- a/src/background/executor.ts +++ b/src/background/executor.ts @@ -18,7 +18,11 @@ // eslint-disable-next-line import/no-restricted-paths -- Type only import type { RunBlock } from "@/contentScript/executor"; import browser, { Runtime, Tabs } from "webextension-polyfill"; -import { BusinessError } from "@/errors"; +import { + BusinessError, + getErrorMessage, + NO_TARGET_FOUND_CONNECTION_ERROR, +} from "@/errors"; import { expectContext } from "@/utils/expectContext"; import { asyncForEach } from "@/utils"; import { getLinkedApiClient } from "@/services/apiClient"; @@ -35,6 +39,18 @@ const tabToOpener = new Map(); const tabToTarget = new Map(); // TODO: One tab could have multiple targets, but `tabToTarget` currenly only supports one at a time +async function safelyRunBrick(...args: Parameters) { + try { + return await runBrick(...args); + } catch (error) { + if (getErrorMessage(error) === NO_TARGET_FOUND_CONNECTION_ERROR) { + throw new BusinessError("The tab doesn't exist or it was closed"); + } + + throw error; + } +} + export async function waitForTargetByUrl(url: string): Promise { const { promise, resolve } = pDefer(); @@ -65,7 +81,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( @@ -87,7 +103,7 @@ export async function requestRunInBroadcast( } try { - const response = runBrick({ tabId: tab.id }, subRequest); + const response = safelyRunBrick({ tabId: tab.id }, subRequest); fulfilled.set(tab.id, await response); } catch (error) { rejected.set(tab.id, error); @@ -113,7 +129,7 @@ export async function requestRunInTarget( } const subRequest = { ...request, sourceTabId }; - return runBrick({ tabId: target }, subRequest); + return safelyRunBrick({ tabId: target }, subRequest); } export async function openTab( diff --git a/src/errors.ts b/src/errors.ts index 5ff0e987ba..1ad538e92a 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -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.", ]; From 26ae1cf37f3f1a24837019498ce6800f9d6250a7 Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Thu, 10 Mar 2022 18:04:46 +0800 Subject: [PATCH 2/6] Update error message and update comment --- src/background/executor.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/background/executor.ts b/src/background/executor.ts index d7607ecd10..04a7925995 100644 --- a/src/background/executor.ts +++ b/src/background/executor.ts @@ -44,7 +44,12 @@ async function safelyRunBrick(...args: Parameters) { return await runBrick(...args); } catch (error) { if (getErrorMessage(error) === NO_TARGET_FOUND_CONNECTION_ERROR) { - throw new BusinessError("The tab doesn't exist or it was closed"); + // We can only be sure that the tab was closed because `safelyRunBrick` is called on + // specific, existing `tabId`s, and not on named targets that may or may not exist. + + // The caught error isn't "The tab was closed" because of: + // https://github.com/pixiebrix/pixiebrix-extension/issues/2902#issuecomment-1062658248 + throw new BusinessError("The tab was closed"); } throw error; From 4eab7823890cd22ac2dff2481364d7978fc15158 Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Fri, 11 Mar 2022 00:18:37 +0800 Subject: [PATCH 3/6] Update comment and error --- src/background/executor.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/background/executor.ts b/src/background/executor.ts index 04a7925995..c416e7b29b 100644 --- a/src/background/executor.ts +++ b/src/background/executor.ts @@ -43,13 +43,15 @@ async function safelyRunBrick(...args: Parameters) { try { return await runBrick(...args); } catch (error) { + // The caught error isn't "The tab was closed" because of: + // https://github.com/pixiebrix/pixiebrix-extension/issues/2902#issuecomment-1062658248 if (getErrorMessage(error) === NO_TARGET_FOUND_CONNECTION_ERROR) { - // We can only be sure that the tab was closed because `safelyRunBrick` is called on - // specific, existing `tabId`s, and not on named targets that may or may not exist. - - // The caught error isn't "The tab was closed" because of: - // https://github.com/pixiebrix/pixiebrix-extension/issues/2902#issuecomment-1062658248 - throw new BusinessError("The tab was closed"); + // NO_TARGET_FOUND_CONNECTION_ERROR is thrown when: + // - the target has no permissions + // - the target was closed after creation + throw new BusinessError( + "PixieBrix doesn't have access to the tab or it was closed" + ); } throw error; From 899c4dbed8dd47763f168f1ca0d90efbdc8995a4 Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Fri, 11 Mar 2022 00:29:08 +0800 Subject: [PATCH 4/6] Explicitly handle closed tabs and access-less tabs --- src/background/executor.ts | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/background/executor.ts b/src/background/executor.ts index c416e7b29b..538cb9574f 100644 --- a/src/background/executor.ts +++ b/src/background/executor.ts @@ -18,11 +18,7 @@ // eslint-disable-next-line import/no-restricted-paths -- Type only import type { RunBlock } from "@/contentScript/executor"; import browser, { Runtime, Tabs } from "webextension-polyfill"; -import { - BusinessError, - getErrorMessage, - NO_TARGET_FOUND_CONNECTION_ERROR, -} from "@/errors"; +import { BusinessError } from "@/errors"; import { expectContext } from "@/utils/expectContext"; import { asyncForEach } from "@/utils"; import { getLinkedApiClient } from "@/services/apiClient"; @@ -32,6 +28,7 @@ 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"; type TabId = number; @@ -39,19 +36,18 @@ const tabToOpener = new Map(); const tabToTarget = new Map(); // TODO: One tab could have multiple targets, but `tabToTarget` currenly only supports one at a time -async function safelyRunBrick(...args: Parameters) { +async function safelyRunBrick({ tabId }: { tabId: number }, request: RunBlock) { + if (!(await canAccessTab(tabId))) { + throw new BusinessError("PixieBrix doesn't have access to the tab"); + } + try { - return await runBrick(...args); + return await runBrick({ tabId }, request); } catch (error) { - // The caught error isn't "The tab was closed" because of: - // https://github.com/pixiebrix/pixiebrix-extension/issues/2902#issuecomment-1062658248 - if (getErrorMessage(error) === NO_TARGET_FOUND_CONNECTION_ERROR) { - // NO_TARGET_FOUND_CONNECTION_ERROR is thrown when: - // - the target has no permissions - // - the target was closed after creation - throw new BusinessError( - "PixieBrix doesn't have access to the tab or it was closed" - ); + // If https://github.com/pixiebrix/webext-messenger/issues/67 is resolved, we don't need the query + const tabStillExists = await browser.tabs.get(tabId).catch(() => false); + if (!tabStillExists) { + throw new BusinessError("The tab was closed"); } throw error; From 677d586ee31508a29877b1735ea72efd2f99b118 Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Fri, 11 Mar 2022 00:52:07 +0800 Subject: [PATCH 5/6] Faster and doubt-free "closed tab" detection --- src/background/executor.ts | 22 +++++++++++++--------- src/chrome.ts | 10 ++++++++++ 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/background/executor.ts b/src/background/executor.ts index 538cb9574f..a3010599e7 100644 --- a/src/background/executor.ts +++ b/src/background/executor.ts @@ -29,9 +29,13 @@ 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(); const tabToTarget = new Map(); // TODO: One tab could have multiple targets, but `tabToTarget` currenly only supports one at a time @@ -41,17 +45,17 @@ async function safelyRunBrick({ tabId }: { tabId: number }, request: RunBlock) { throw new BusinessError("PixieBrix doesn't have access to the tab"); } - try { - return await runBrick({ tabId }, request); - } catch (error) { - // If https://github.com/pixiebrix/webext-messenger/issues/67 is resolved, we don't need the query - const tabStillExists = await browser.tabs.get(tabId).catch(() => false); - if (!tabStillExists) { - throw new BusinessError("The tab was closed"); - } + 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), + ]); - throw error; + if (result === TYPE_WAS_CLOSED) { + throw new BusinessError("The tab was closed"); } + + return result; } export async function waitForTargetByUrl(url: string): Promise { diff --git a/src/chrome.ts b/src/chrome.ts index b9d9ca9adf..62ffa1104a 100644 --- a/src/chrome.ts +++ b/src/chrome.ts @@ -138,3 +138,13 @@ export async function setReduxStorage( ): Promise { await browser.storage.local.set({ [storageKey]: JSON.stringify(value) }); } + +export async function onTabClose(watchedTabId: number): Promise { + await new Promise((resolve) => { + browser.tabs.onRemoved.addListener((closedTabId) => { + if (closedTabId === watchedTabId) { + resolve(); + } + }); + }); +} From caf188a68ace605350d9f07b20aa5e6b985b6620 Mon Sep 17 00:00:00 2001 From: Federico Brigante Date: Sat, 12 Mar 2022 16:11:44 +0800 Subject: [PATCH 6/6] Remove listener on tab close --- src/chrome.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/chrome.ts b/src/chrome.ts index 62ffa1104a..af8a88fb17 100644 --- a/src/chrome.ts +++ b/src/chrome.ts @@ -141,10 +141,13 @@ export async function setReduxStorage( export async function onTabClose(watchedTabId: number): Promise { await new Promise((resolve) => { - browser.tabs.onRemoved.addListener((closedTabId) => { + const listener = (closedTabId: number) => { if (closedTabId === watchedTabId) { resolve(); + browser.tabs.onRemoved.removeListener(listener); } - }); + }; + + browser.tabs.onRemoved.addListener(listener); }); }