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

Fix Chrome death, by creating both cross-frame ports in the background #259

Merged
merged 4 commits into from
Oct 1, 2023

Conversation

praschke
Copy link
Collaborator

on Chrome (currently 117), the port created in the content script with runtime.connect does not properly receive an onDisconnect event when the service worker sleeps. the port created in the background with tabs.connect does receive the event, so create both ports with tabs.connect.

fixes #241.

@praschke praschke requested a review from a team as a code owner September 30, 2023 16:19
@github-actions
Copy link

github-actions bot commented Sep 30, 2023

⚠️ Visual differences introduced by this PR; please validate if they are desirable.

View Playwright Report (note: open the "playwright-report" artifact)

on Chrome (currently 117), the port created in the content script with
runtime.connect does not properly receive an onDisconnect event when
the service worker sleeps. the port created in the background with
tabs.connect does receive the event, so create both ports with
tabs.connect.

fixes yomidevs#241.
@djahandarie djahandarie added kind/bug The issue or PR is regarding a bug priority/high High priority issue/PR labels Oct 1, 2023
@djahandarie djahandarie changed the title create both cross-frame ports in the background Fix Chrome death, by creating both cross-frame ports in the background Oct 1, 2023
djahandarie
djahandarie previously approved these changes Oct 1, 2023
Copy link
Collaborator

@djahandarie djahandarie left a comment

Choose a reason for hiding this comment

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

Thank you so much, this looks like it will be the key PR for getting yomitan released. Left some minor comments, feel free to resolve them however you see fit.

@djahandarie djahandarie enabled auto-merge October 1, 2023 02:17
it was only used in the cross-frame api.

additionally, this comment on triggerExtensionUnloaded is incorrect.
not sure if API messages should be sent in prepare(), but we should
probably wait for the ready signal before doing so
auto-merge was automatically disabled October 1, 2023 08:51

Head branch was pushed to by a user without write access

Copy link
Collaborator

@djahandarie djahandarie left a comment

Choose a reason for hiding this comment

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

🎉

@djahandarie djahandarie added this pull request to the merge queue Oct 1, 2023
Merged via the queue into yomidevs:master with commit 8430fce Oct 1, 2023
@praschke praschke deleted the chrome-death-fix branch October 1, 2023 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug The issue or PR is regarding a bug priority/high High priority issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yomitan popup window fails to update, repeatedly showing the same dictionary entry
2 participants