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.

Sign up for GitHub

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

Jump to bottom

Use Messenger for executeInTarget #2044

Closed
wants to merge 3 commits into from
Closed

Use Messenger for executeInTarget #2044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e330e55
Use Messenger for `executeInTarget`
fregante Dec 4, 2021
6b3269d
Restore wait.ts
fregante Dec 4, 2021
c667d99
Comment
fregante Dec 4, 2021
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
14 changes: 7 additions & 7 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@
"webext-content-scripts": "^0.10.1",
"webext-detect-page": "^3.0.2",
"webext-dynamic-content-scripts": "^8.0.1",
"webext-messenger": "^0.15.0-5",
"webext-messenger": "^0.15.1-0",
"webext-patterns": "^1.1.1",
"webext-polyfill-kinda": "^0.1.0",
"webextension-polyfill": "^0.8.0",
Expand Down
75 changes: 2 additions & 73 deletions src/background/executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import {

const RUN_BLOCK = `${MESSAGE_PREFIX}RUN_BLOCK`;
const MESSAGE_RUN_BLOCK_OPENER = `${MESSAGE_PREFIX}RUN_BLOCK_OPENER`;
const MESSAGE_RUN_BLOCK_TARGET = `${MESSAGE_PREFIX}RUN_BLOCK_TARGET`;
const MESSAGE_RUN_BLOCK_BROADCAST = `${MESSAGE_PREFIX}RUN_BLOCK_BROADCAST`;
const MESSAGE_RUN_BLOCK_FRAME_NONCE = `${MESSAGE_PREFIX}RUN_BLOCK_FRAME_NONCE`;
Comment on lines 43 to 45
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A note about the other targets:

Copy link
Contributor

@twschiller twschiller Dec 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Broadcast likely similar implementation to Replace notifyContentScripts with Messenger #1329

Unlike notifications, broadcast actually returns the results as the array. You mean in terms of how it notifies all the tabs?

frame_nonce I haven't looked at it yet, but maybe via {tabId: "this", page: "?nonce=ABC"}

I think using the more general page matching capability here to support our nonce based frame approach makes sense here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think using the more general page matching capability here to support our nonce based frame approach makes sense here

I'm starting to think that all of these methods will continue existing in the background page because that's the only context that can do this.


Expand Down Expand Up @@ -104,25 +103,6 @@ async function waitNonceReady(
return true;
}

async function waitReady(
{ tabId, frameId }: Target,
{ maxWaitMillis = 10_000 }: WaitOptions = {}
): Promise<boolean> {
const startTime = Date.now();
while (tabReady[tabId]?.[frameId] == null) {
if (Date.now() - startTime > maxWaitMillis) {
throw new BusinessError(
`Tab ${tabId} was not ready after ${maxWaitMillis}ms`
);
Comment on lines -114 to -116
Copy link
Contributor Author

@fregante fregante Dec 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This specific error no longer exists because the retries are handled automatically by webext-messenger when forwarding it. The max retry time is fixed/global to around 5s, but it only applies "per hop", i.e. the background can take 4s to answer, then the content script can wait 4s more, etc.

I suppose this is an issue on large pages that have document_idle late.

If you think this is an issue, I could add an option¹ for this or increase the default timeout.

1 options are kind of difficult at the moment, 🤭 which is why I split getMethod from getNotifier instead of using isNotification: true

Copy link
Contributor Author

@fregante fregante Dec 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Since I'm bringing the handler back to this context, I will restore this function 🎉

}

// eslint-disable-next-line no-await-in-loop -- retry loop
await sleep(50);
}

return true;
}

const handlers = new HandlerMap();

handlers.set(
Expand Down Expand Up @@ -220,42 +200,18 @@ handlers.set(
}
);

handlers.set(
MESSAGE_RUN_BLOCK_TARGET,
async (request: RunBlockRequestAction, sender) => {
const target = tabToTarget.get(sender.tab.id);

if (!target) {
throw new BusinessError("Sender tab has no target");
}

console.debug(`Waiting for target tab ${target} to be ready`);
// For now, only support top-level frame as target
await waitReady({ tabId: target, frameId: 0 });
console.debug(
`Sending ${RUN_BLOCK} to target tab ${target} (sender=${sender.tab.id})`
);
return runBlockInContentScript(
{ tabId: target, frameId: 0 },
{
sourceTabId: sender.tab.id,
...request.payload,
}
);
}
);

export async function openTab(
this: MessengerMeta,
createProperties: Tabs.CreateCreatePropertiesType
): Promise<void> {
): Promise<number> {
// Natively links the new tab to its opener + opens it right next to it
const openerTabId = this.trace[0].tab.id;
const tab = await browser.tabs.create({ ...createProperties, openerTabId });

// FIXME: include frame information here
tabToTarget.set(openerTabId, tab.id);
tabToOpener.set(tab.id, openerTabId);
return tab.id;
}

export async function markTabAsReady(this: MessengerMeta) {
Expand Down Expand Up @@ -388,33 +344,6 @@ export async function executeForNonce(
);
}

export async function executeInTarget(
blockId: string,
blockArgs: BlockArg,
options: RemoteBlockOptions
): Promise<unknown> {
console.debug(`Running ${blockId} in the target tab`, {
blockId,
blockArgs,
options,
});

const { maxRetries = DEFAULT_MAX_RETRIES } = options;

return retrySend(
async () =>
browser.runtime.sendMessage({
type: MESSAGE_RUN_BLOCK_TARGET,
payload: {
blockId,
blockArgs,
options,
},
}),
maxRetries
);
}

export async function executeInAll(
blockId: string,
blockArgs: BlockArg,
Expand Down
6 changes: 5 additions & 1 deletion src/blocks/effects/redirectPage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ import {
} from "@/blocks/transformers/url";
import { makeURL } from "@/utils";

export const target = {
id: -1,
};
Comment on lines +27 to +29
Copy link
Contributor Author

@fregante fregante Dec 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The target was previous stored in the background page and the messaging solution expected the background page to match "sender" with "target".

With the messenger we can message the tab "directly" (i.e. the messenger takes care of forwarding) so we can store it locally (which is where the tab is created anyway).

This might not be the best solution but it's clean-ish (when messaging the target, you'll have to import this block, which shows the direct dependency between the two)

Copy link
Contributor

@twschiller twschiller Dec 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The target was previous stored in the background page and the messaging solution expected the background page to match "sender" with "target".

This change actually changes the behavior. The old code also tracked if a normal link opened up a new tab. With this new code the "target" relationship will only be tracked through this block?

  • Check impact on tabs opened through 'a'

See the bookkeeping via browser.tabs.onCreated at initExecutor: http://github.com/pixiebrix/pixiebrix-extension/blob/main/src/background/executor.ts#L300-L300

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. In that case we can exclusively send messages via the background page because that's the only context that can know that information. I'll have to restore the previous manual forwarding


export class NavigateURLEffect extends Effect {
constructor() {
super(
Expand Down Expand Up @@ -62,7 +66,7 @@ export class OpenURLEffect extends Effect {
params,
spaceEncoding = URL_INPUT_SPACE_ENCODING_DEFAULT,
}: BlockArg): Promise<void> {
await openTab({
target.id = await openTab({
url: makeURL(url, params, spaceEncoding),
});
}
Expand Down
8 changes: 0 additions & 8 deletions src/contentScript/messenger/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,6 @@

/* Do not use `registerMethod` in this file */
import { getMethod, getNotifier } from "webext-messenger";
import { isContentScript } from "webext-detect-page";

// TODO: This should be a hard error, but due to unknown dependency routes, it can't be enforced yet
if (isContentScript() && process.env.DEBUG) {
console.warn(
"This should not have been imported in the content script. Use the API directly instead."
);
}

export const getFormDefinition = getMethod("FORM_GET_DEFINITION");
export const resolveForm = getMethod("FORM_RESOLVE");
Expand Down
27 changes: 24 additions & 3 deletions src/runtime/reducePipeline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,10 @@ import {
UUID,
} from "@/core";
import { castArray, isPlainObject } from "lodash";
import { BusinessError, ContextError } from "@/errors";
import { BusinessError, ContextError, isErrorObject } from "@/errors";
import {
executeInAll,
executeInOpener,
executeInTarget,
executeOnServer,
} from "@/background/executor";
import { getLoggingConfig } from "@/background/logging";
Expand Down Expand Up @@ -59,6 +58,8 @@ import {
import ConsoleLogger from "@/tests/ConsoleLogger";
import { ResolvedBlockConfig } from "@/runtime/runtimeTypes";
import { UnknownObject } from "@/types";
import { runBlockInContentScript } from "@/contentScript/messenger/api";
import { target } from "@/blocks/effects/redirectPage";

type CommonOptions = ApiVersionOptions & {
/**
Expand Down Expand Up @@ -223,7 +224,27 @@ async function execute(
}

case "target": {
return executeInTarget(config.id, args, commonOptions);
if (target.id === -1) {
throw new BusinessError("Sender tab has no target");
}

return runBlockInContentScript(
{ tabId: target.id },
{
blockId: config.id,
blockArgs: args,
options: commonOptions,
}
).catch((error: unknown) => {
if (
isErrorObject(error) &&
error.message.startsWith("Could not establish connection")
) {
// Either the target isn't receiving messages (due to lack of permissions or other errors)
// or it does not exist anymore
throw new BusinessError("Sender tab has no target");
}
});
}

case "broadcast": {
Expand Down