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

don't retain port in content scripts #285

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

turbocrime
Copy link
Collaborator

@turbocrime turbocrime commented Feb 4, 2025

Content scripts no longer retain a MessagePort in violation of object transfer rules.

The injected-penumbra-global.ts content script has no insight to channel state, and as a mainworld script should not be relied upon to manage channel state. So this takes the approach of always requesting a new port.

The injected-listeners.ts content script cannot re-provide a port, since it must retain no reference. So this depends on changes in penumbra-zone/web#2018 so it may launch a new session for each request.

Copy link
Collaborator Author

@turbocrime turbocrime left a comment

Choose a reason for hiding this comment

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

commentary. possibly should minimize extra changes

Comment on lines +22 to +23
import { createPenumbraStateEvent } from '@penumbra-zone/client/event';
import type { PenumbraProvider } from '@penumbra-zone/client/provider';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not strictly necessary, but due to poor webpack tree-shaking, importing from the root package apparently caused the generated script to be very large.

a more specific import significantly reduces the size of the content script.

disconnect: () => this.postDisconnectRequest(),
isConnected: () => Boolean(this.port && this.presentState === PenumbraState.Connected),
isConnected: () => this.presentState === PenumbraState.Connected,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i would like to do some more investigation to identify possibly unknown problems with these changes to state transitions

Comment on lines +98 to +110
private postDisconnectRequest() {
const attempt = this.listenEndMessage();
window.postMessage(disconnectMessage, '/', []);
return attempt;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pulled this out into its own method to match the structure of the connect request logic

Comment on lines +105 to +115
if (this.presentState !== PenumbraState.Connected) {
this.setPending();
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this change needs some closer inspection too

@turbocrime turbocrime marked this pull request as draft February 4, 2025 01:32
@TalDerei TalDerei marked this pull request as ready for review February 4, 2025 19:53
Copy link
Contributor

@TalDerei TalDerei left a comment

Choose a reason for hiding this comment

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

looks like the changeset primarily consolidated content scripts, removed global port object, and better message handling.

Comment on lines +98 to +99
window.addEventListener('message', praxDocumentListener);
chrome.runtime.onMessage.addListener(praxExtensionListener);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add code comments to the two event listeners, praxDocumentListener and praxExtensionListener?

@TalDerei TalDerei changed the base branch from main to connection-lifecycle-branch February 4, 2025 23:07
@TalDerei TalDerei self-requested a review February 4, 2025 23:08
Copy link
Contributor

@TalDerei TalDerei left a comment

Choose a reason for hiding this comment

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

changed base branch to connection-lifecycle-refactor

@turbocrime turbocrime force-pushed the dont-violate-transfer branch from db1ef53 to c9ed017 Compare February 19, 2025 08:09
@turbocrime turbocrime merged commit 987304b into connection-lifecycle-branch Feb 19, 2025
3 checks passed
@turbocrime turbocrime deleted the dont-violate-transfer branch February 19, 2025 08:11
turbocrime added a commit that referenced this pull request Feb 19, 2025
@turbocrime turbocrime mentioned this pull request Feb 19, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants