-
Notifications
You must be signed in to change notification settings - Fork 22
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
Handle closed tabs better while attempting to message them #3572
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,42 +20,45 @@ import { expectContext } from "@/utils/expectContext"; | |
import { asyncForEach } from "@/utils"; | ||
import { getLinkedApiClient } from "@/services/apiClient"; | ||
import { JsonObject } from "type-fest"; | ||
import { MessengerMeta } from "webext-messenger"; | ||
import { | ||
MessengerMeta, | ||
errorTargetClosedEarly, | ||
errorTabDoesntExist, | ||
} from "webext-messenger"; | ||
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"; | ||
import { getErrorMessage } from "@/errors/errorHelpers"; | ||
// eslint-disable-next-line import/no-restricted-paths -- Type only | ||
import type { RunBlock } from "@/contentScript/runBlockTypes"; | ||
import { BusinessError } from "@/errors/businessErrors"; | ||
|
||
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"); | ||
} | ||
try { | ||
return await runBrick({ tabId }, request); | ||
} catch (error) { | ||
const errorMessage = getErrorMessage(error); | ||
|
||
// Repackage tab-lifecycle-related errors as BusinessErrors | ||
if ([errorTargetClosedEarly, errorTabDoesntExist].includes(errorMessage)) { | ||
throw new BusinessError(errorMessage); | ||
} | ||
|
||
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), | ||
]); | ||
Comment on lines
-48
to
-52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there was a race condition (heh) right here. If You can see the two error message alternating here. alternate.movNoteThis alternating of message hasn't really been fixed by this PR. Before
After
Both are "tab doesn't exist" errors but sometimes we can track why it doesn't exist, so we can throw "tab was closed" instead |
||
// This must follow the tab existence checks or else it returns false even if the tab simply doesn't exist | ||
if (!(await canAccessTab(tabId))) { | ||
throw new BusinessError("PixieBrix doesn't have access to the tab"); | ||
} | ||
|
||
if (result === TYPE_WAS_CLOSED) { | ||
throw new BusinessError("The tab was closed"); | ||
throw error; | ||
} | ||
|
||
return result; | ||
} | ||
|
||
export async function waitForTargetByUrl(url: string): Promise<Target> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this was being run first, it was reporting "no access" when the tab just didn't exist (was closed way too early).
Here you can see different error messages depending on how long I wait before closing the tab.
Screen.Recording.42.mov