Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MV3: Update service worker restart logic and keep-alive logic for dapp support #16075
MV3: Update service worker restart logic and keep-alive logic for dapp support #16075
Changes from 15 commits
ea21786
e860244
a8f86ad
9d2da57
0eb8501
169722c
dd2146e
f60ba5f
d09dfca
9e910b5
09396a8
69caf1a
e5ef11b
c00fb30
c6f042e
b9fdffa
eff5234
8efb02c
073f05e
ae3c2df
ea21f94
db5a926
da6aa11
0c86b83
3c52f65
665b934
c10bded
9f9aad3
00ed35f
acefe68
dcd4284
1ccb9d7
2ab6234
3b66c2c
0047146
bbd5cff
8074e59
b0dc2a8
66107a9
4b68873
32aa4ef
3bf4dfb
4bc6cfb
a8b6921
38128d0
cb87438
6b334a7
924235b
61ed554
1b9e298
bb5ba6b
0f9ff27
de862d4
3fd3ba3
3d9007b
d1724c7
5461986
b45a920
de1e441
931f511
b07242a
a568420
f410124
bba4dba
e676c71
a9aa850
ac1c32a
b0e5ae0
ba3ed91
206837b
d979dcf
858f5d4
67c5533
76bc0f6
58db96f
db28cf8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We should keep service worker alive even if account is not connected on DAPP I think. There are some queries that user can make even in disconnected state.
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.
Edit: Addressed in linked thread
Slightly similar point to #16075 (comment), something to keep in mind when settling the above:
If the user is interacting with MetaMask on a dapp on one page, this should not result in events/change of behavior readable from an unauthorized (disconnected) tab/page as this would both leak entropy facilitating fingerprinting as well as help in detecting user activity (e.g distinguishing from "visiting browser has MetaMask installed" vs "visiting user has opened MetaMask during this session".
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.
@jpuri
The keep-alive logic starts for any dapp interaction, whether or not accounts are connected. On initial load, we only trigger the keep-alive logic if the dapp connects or if the dapp sends requests. Initially, I don't think there is a way to distinguish a dapp from a non-dapp page aside from connected accounts... unless I'm missing something. I actually prefer to avoid running the keep-alive logic on non-dapps or when the provider is not in use on a page to save CPU and memory on the browser.
@legobeat getting to these comments after I run some more tests sending messages to/from our content script and extension
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.
@legobeat I added a reply here #16075 (comment)
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.
Hey @digiwand : I know it is complex, but could you test phishing stream and legacy stream changes.
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.
Thanks @seaona for taking this head on! As discussed earlier, we will handle the support for the phishing stream logic in a separate PR (see #15987)
@seaona has found that terminating the service worker breaks the phishing warning page logic