-
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
#579: Make content script injector aware of content script itself #661
Conversation
d3ec4a5
to
482f559
Compare
console.debug( | ||
`Skipping injectDevtoolsContentScript because no activeTab permissions for tab: ${tabId}` | ||
`Skipping injectDevtoolsContentScript because no activeTab permissions`, | ||
{ tabId, frameId, url } |
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.
The interesting part here is that activeTab
is preserved for the top frame’s origin, but not for the iframe’s origins. This means that if a cross-origin iframe reloads, we lose access to it.
Tested on https://iframe-test-page.vercel.app/cross-origin-parent.html
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.
You mean if the devtools are connected to another frame on the tab? (We haven't really though through the page editor functionality/handling for frames)
Please leave this note in the code comments, as it's important context for future work
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.
I was talking about its behavior in general. I haven't really looked into how/why it deals with frames here other than to ensure that it throws errors correctly.
Will add the comment 👍 (Edit: added)
Ideally, probably in a future PR, I'll want to improve this part as well, like avoiding injection altogether if the tab never had activeTab
in the first place, or if its URL was off-limits (chrome://)
src/background/util.ts
Outdated
export async function onReadyNotification(signal: AbortSignal): Promise<true> { | ||
return new Promise((resolve) => { | ||
const onMessage = (message: unknown) => { | ||
if (message === "pbReady") { |
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.
Likewise here I'm using a "raw" message because it's a temporary listener and MESSAGE_CONTENT_SCRIPT_READY
is already fully handled in a different file. Let me know if you'd to follow the standard type: SOMETHING, payload: []
format instead and where I should put that constant.
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.
For consistency, I have the preference for using {type: PIXIEBRIX_READY}
. Even if we keep the string, I have the preference for using a shared constant for sending/handling the message
Maybe throw it in background/protocol for now? As part of the messaging overhaul in the future, we'll be figuring how where/how to refactor the action constants/payload typings
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.
background/protocol
has a lot of dependencies and code, I'll probably use messaging/constants.ts
for now. Related:
@@ -308,7 +323,7 @@ if (isBackgroundPage()) { | |||
}); | |||
|
|||
browser.webNavigation.onDOMContentLoaded.addListener((details) => { | |||
void injectTemporaryAccess(details); | |||
emitDevtools("DOMContentLoaded", details); |
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.
Could you add a comment here on why this is the correct sequence of calls? It seems weird that we'd want the devtools to handle DOMContentLoaded before it has access
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.
injectTemporaryAccess
was not awaited, so I'd ask you the same question 😃
I moved it lower simply because the injector was logging before and after emitDevtools (because some code was execute before, and some after)
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.
I think the order doesn't matter. As with everything involving communication, there's a lot of layers and it's hard to follow...
Eventually it gets to here: http://github.com/pixiebrix/pixiebrix-extension/blob/bc8c32d237056630d459b60bb445da5d373500b0/src/devTools/context.ts#L213-L213
Which I think ends up handling the race condition through a combination of injectScript and waitReady
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.
waitReady
only exists for executeInTarget
-> MESSAGE_RUN_BLOCK_TARGET
now.
I haven't looked into emitDevtools("DOMContentLoaded")
specifically, but if you expect it to run when the CS is ready, then it should await attemptTemporaryAccess
.
Overall this looks good. With complicated items like this, I have a strong preference for ensuring there's inline documentation of considerations, which sequencing is important, etc. In general, reviewing this PR has affirmed that we need to have testing infrastructure in place prior to re-architecting the messaging infrastructure Let me know when this is ready for testing and I'll help with manual QA. Please merge/rebase this PR, so we know what the impact is |
Review followed. If you see 🚀 on your review messages it's because I followed them. I do this only when I also leave a comment, because if I "resolve" the conversation you might miss it. I added some comments in the code, but in some places |
@@ -220,7 +220,7 @@ | |||
"^.+\\.txt$": "jest-raw-loader" | |||
}, | |||
"transformIgnorePatterns": [ | |||
"node_modules/(?!@cfworker|webext-detect-page|idb|webext-patterns|webext-polyfill-kinda|p-timeout|p-defer)" | |||
"node_modules/(?!@cfworker|webext-detect-page|idb|webext-patterns|webext-polyfill-kinda|webext-additional-permissions|p-timeout|p-defer)" |
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.
I'll look into enabling native ESM support in Jest at some point 😅 https://stackoverflow.com/a/61653104
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.
OK to merge - will address any connection corner cases as they arise
To avoid sending huge PRs, I'm splitting this work over a few PRs. In this one I'm trying to preserve the general behavior while setting it up for further potential improvements. This PR:
webext-dynamic-content-script
)webext-dynamic-content-script
, attempting to inject on a forbidden page, or manually injecting itReady except:
pbReady
messagepbReady
globalGeneral direction for future PRs:
We'll see how practical that will be though. The intent is avoid/limit calling the function itself and also to review the injection and communication overall.