fix: Handle error when offscreen document already exists #25138
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gudahtt
force-pushed
the
handle-offscreen-document-already-exists-error
branch
from
9ce1a35 to
c00e8f7
Compare
|
I can't write unit tests for this file directly, not easily at least, because it has side-effects. I could move this function (and maybe some others) into a separate module and write unit tests for them there. Probably we should do this at some point. But there is minimal value in doing so because the logic here is so simple. Really it's the chrome extension API behavior that would be valuable to test; if we're mocking that, we're not really validating this fix. That means a valuable test would have to be an e2e or manual test. But I don't know how to trigger the race condition where we attempt to create the offscreen document when it already exists. |
danjm
force-pushed
the
handle-offscreen-document-already-exists-error
branch
from
c00e8f7 to
f3d8360
Compare
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
This comment was marked as resolved.
This comment was marked as resolved.
Gudahtt
force-pushed
the
handle-offscreen-document-already-exists-error
branch
from
f3d8360 to
1594c48
Compare
davidmurdoch
previously approved these changes
Jun 7, 2024
danjm
previously approved these changes
Jun 7, 2024
|
Still investigating the remaining e2e test error. It's a pre-existing flaky test, but something about this PR appears to make it fail more often (possibly every time) |
Gudahtt
force-pushed
the
handle-offscreen-document-already-exists-error
branch
from
c1e41a7 to
3397a77
Compare
|
The test failures on this PR were resolved by #25164 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #25138 +/- ##
========================================
Coverage 70.44% 70.44%
========================================
Files 1274 1274
Lines 44080 44080
Branches 12453 12453
========================================
Hits 31051 31051
Misses 13029 13029 ☔ View full report in Codecov by Sentry. |
Builds ready [3397a77]
Page Load Metrics (129 ± 174 ms)
Bundle size diffs
|
Gudahtt
force-pushed
the
handle-offscreen-document-already-exists-error
branch
2 times, most recently
from
6eb8858 to
ac73ecc
Compare
Builds ready [ac73ecc]
Page Load Metrics (53 ± 5 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Gudahtt
added
the
needs-dev-review
Gudahtt
force-pushed
the
handle-offscreen-document-already-exists-error
branch
from
ac73ecc to
b54361b
Compare
Gudahtt
commented
Jun 24, 2024
| // Report unrecongized errors without halting wallet initialization | ||
| // Failures to create the offscreen document does not compromise wallet data integrity or | ||
| // core functionality, it's just needed for specific features. | ||
| captureException(error); |
There was a problem hiding this comment.
Just updated this to catch all platform errors here (as suggested by @FrederikBolding on Slack) to avoid bricking the wallet if something else goes wrong here. We're still capturing the error in Sentry, and this is a non-essential step, so this should be a safe way to approach this for now.
See commit ffb2c3a
Builds ready [ffb2c3a]
Page Load Metrics (49 ± 3 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Gudahtt
force-pushed
the
handle-offscreen-document-already-exists-error
branch
from
ffb2c3a to
429d147
Compare
Builds ready [429d147]
Page Load Metrics (151 ± 170 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
danjm
approved these changes
Jul 15, 2024
Gudahtt
force-pushed
the
handle-offscreen-document-already-exists-error
branch
2 times, most recently
from
08e0378 to
440e581
Compare
vthomas13
approved these changes
Jul 17, 2024
A `try..catch` block has been added to catch errors about the Offscreen document already existing. This circumstance is not a bug, it's expected to happen sometimes due to race conditions between `hasDocument` and the creation step. The code in question is just meant to create the document if it doesn't exist, so it's OK if we get this error. The `hasDocument` call has been removed as well, since it's now redundant. This function wasn't safe to call anyway; it's an undocumented private function. See here for more information: * https://stackoverflow.com/a/7725839 * https://groups.google.com/a/chromium.org/g/chromium-extensions/c/D5Jg2ukyvUc4 Fixes #25118
We now capture unrecognized errors and report them to Sentry, rather than throwing them. This ensures that any new failures would not brick the wallet.
Gudahtt
force-pushed
the
handle-offscreen-document-already-exists-error
branch
from
440e581 to
8341502
Compare
|
Builds ready [8341502]
Page Load Metrics (68 ± 7 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
github-actions
bot
removed
the
needs-dev-review
metamaskbot
added
the
release-12.3.0
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
A
try..catchblock has been added to catch errors about the Offscreen document already existing. This circumstance is not a bug, it's expected to happen sometimes due to race conditions betweenhasDocumentand the creation step. The code in question is just meant to create the document if it doesn't exist, so it's OK if we get this error.The
hasDocumentcall has been removed as well, since it's now redundant. This function wasn't safe to call anyway; it's an undocumented private function.See here for more information:
Related issues
Fixes #25118
Manual testing steps
I do not know how to test this unfortunately.
Screenshots/Recordings
N/A
Pre-merge author checklist
Pre-merge reviewer checklist