fix: useLinking crash#10262
Merged
satya164 merged 2 commits intoJan 26, 2022
Merged
Conversation
|
Hey roryabraham! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines. |
|
✔️ Deploy Preview for react-navigation-example ready! 🔨 Explore the source changes: 66eca91 🔍 Inspect the deploy log: https://app.netlify.com/sites/react-navigation-example/deploys/61e5f8d2525f4a000743cc37 😎 Browse the preview: https://deploy-preview-10262--react-navigation-example.netlify.app/ |
Codecov Report
@@ Coverage Diff @@
## main #10262 +/- ##
=======================================
Coverage 74.41% 74.41%
=======================================
Files 160 160
Lines 4858 4855 -3
Branches 1838 1836 -2
=======================================
- Hits 3615 3613 -2
+ Misses 1209 1208 -1
Partials 34 34
Continue to review full report at Codecov.
|
satya164
approved these changes
Jan 26, 2022
Contributor
|
Yihaaa! |
Member
|
Thank you for the PR and investigation |
|
Hey! This issue is closed and isn't watched by the core team. You are welcome to discuss the issue with others in this thread, but if you think this issue is still valid and needs to be tracked, please open a new issue with a repro. |
6 tasks
MohamedLamineAllal
pushed a commit
to MohamedLamineAllal/react-navigation
that referenced
this pull request
Feb 4, 2022
Hello 👋 We've been seeing [this crash](react-navigation#9970 (comment)) (shown in the following screenshot) happening in our production code. I unfortunately don't know how to consistently reproduce this problem, but I believe this PR is very safe and will fix the issue.  As explained in [this comment](https://github.com/react-navigation/react-navigation/pull/9970/files#r784475821), `TypeError: Cannot read property 'id' of undefined` tells us that somehow the `items` array is actually getting in a state where it contains `undefined`. Now, there's only four places the `items` array is written to: [line 84](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L84), [line 86](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L86), [line 108](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L108), and [line 110](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L110). Reviewing these one-by-one: #### Line 84 `items = items.slice(0, index - 1)` -> I don't see any way this can add an `undefined` value to the array ❌ #### Line 86 `items.push({ path, state, id })` -> At worst, this could append an object like: `{path: undefined, state: undefined, id: undefined}`, but we wouldn't end up with `undefined` itself in the `items` array. So not here either ❌ #### Line 108 `items = [{ path, state, id }];` -> Similar to line 86, this can't cause `undefined` to be in the `items` array ❌ #### Line 110 `items[index] = { path, state, id };` -> This _can_ cause `undefined` to be in the `items` array if `index` is greater than the length of the array, like so: ```js > let items = []; undefined > items[1] = 'Hello'; 'Hello' > items[0]; undefined ``` It seems this is our problem line, and tells us that the `index` is becoming greater than the length of the items array. I believe I found the only location in this component where that could happen, and fix it in this PR. Let me know if you have questions or concerns! If anyone from the community wants to help out with providing testing or consistent reproduction steps, that would be great!
MohamedLamineAllal
pushed a commit
to MohamedLamineAllal/react-navigation
that referenced
this pull request
Feb 4, 2022
Hello 👋 We've been seeing [this crash](react-navigation#9970 (comment)) (shown in the following screenshot) happening in our production code. I unfortunately don't know how to consistently reproduce this problem, but I believe this PR is very safe and will fix the issue.  As explained in [this comment](https://github.com/react-navigation/react-navigation/pull/9970/files#r784475821), `TypeError: Cannot read property 'id' of undefined` tells us that somehow the `items` array is actually getting in a state where it contains `undefined`. Now, there's only four places the `items` array is written to: [line 84](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L84), [line 86](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L86), [line 108](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L108), and [line 110](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L110). Reviewing these one-by-one: #### Line 84 `items = items.slice(0, index - 1)` -> I don't see any way this can add an `undefined` value to the array ❌ #### Line 86 `items.push({ path, state, id })` -> At worst, this could append an object like: `{path: undefined, state: undefined, id: undefined}`, but we wouldn't end up with `undefined` itself in the `items` array. So not here either ❌ #### Line 108 `items = [{ path, state, id }];` -> Similar to line 86, this can't cause `undefined` to be in the `items` array ❌ #### Line 110 `items[index] = { path, state, id };` -> This _can_ cause `undefined` to be in the `items` array if `index` is greater than the length of the array, like so: ```js > let items = []; undefined > items[1] = 'Hello'; 'Hello' > items[0]; undefined ``` It seems this is our problem line, and tells us that the `index` is becoming greater than the length of the items array. I believe I found the only location in this component where that could happen, and fix it in this PR. Let me know if you have questions or concerns! If anyone from the community wants to help out with providing testing or consistent reproduction steps, that would be great!
MohamedLamineAllal
pushed a commit
to MohamedLamineAllal/react-navigation
that referenced
this pull request
Feb 4, 2022
Hello 👋 We've been seeing [this crash](react-navigation#9970 (comment)) (shown in the following screenshot) happening in our production code. I unfortunately don't know how to consistently reproduce this problem, but I believe this PR is very safe and will fix the issue.  As explained in [this comment](https://github.com/react-navigation/react-navigation/pull/9970/files#r784475821), `TypeError: Cannot read property 'id' of undefined` tells us that somehow the `items` array is actually getting in a state where it contains `undefined`. Now, there's only four places the `items` array is written to: [line 84](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L84), [line 86](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L86), [line 108](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L108), and [line 110](https://github.com/react-navigation/react-navigation/blob/fb84805c889bbb7059e7e95592c004aea2a510d6/packages/native/src/useLinking.tsx#L110). Reviewing these one-by-one: #### Line 84 `items = items.slice(0, index - 1)` -> I don't see any way this can add an `undefined` value to the array ❌ #### Line 86 `items.push({ path, state, id })` -> At worst, this could append an object like: `{path: undefined, state: undefined, id: undefined}`, but we wouldn't end up with `undefined` itself in the `items` array. So not here either ❌ #### Line 108 `items = [{ path, state, id }];` -> Similar to line 86, this can't cause `undefined` to be in the `items` array ❌ #### Line 110 `items[index] = { path, state, id };` -> This _can_ cause `undefined` to be in the `items` array if `index` is greater than the length of the array, like so: ```js > let items = []; undefined > items[1] = 'Hello'; 'Hello' > items[0]; undefined ``` It seems this is our problem line, and tells us that the `index` is becoming greater than the length of the items array. I believe I found the only location in this component where that could happen, and fix it in this PR. Let me know if you have questions or concerns! If anyone from the community wants to help out with providing testing or consistent reproduction steps, that would be great!
|
There is another issue with the new code lines regarding item.id. This only seems to happen sometimes, when I click on the first drawer item. Please see the screenshot attached. And sometimes the error isn't thrown, but the browser seems to go backwards after clicking the first drawer item, which means leaving the website. |
Contributor
|
Hey @chrda81! I'm also running into this error intermittently & planning to debug, but unable to reproduce. Could you tell the step by step to repro for the drawer (unsure, is this also intermittent for drawer)? A video would be great! Thank you 😄 Another note: this seems to happen a lot in Chrome (unsure if this is related) |
11 tasks
I am running into the same bug as well, and weirdly it only happens to the first drawer/bottom tab item. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
Hello 👋
We've been seeing this crash (shown in the following screenshot) happening in our production code. I unfortunately don't know how to consistently reproduce this problem, but I believe this PR is very safe and will fix the issue.
As explained in this comment,
TypeError: Cannot read property 'id' of undefinedtells us that somehow theitemsarray is actually getting in a state where it containsundefined.Now, there's only four places the
itemsarray is written to: line 84, line 86, line 108, and line 110.Reviewing these one-by-one:
Line 84
items = items.slice(0, index - 1)-> I don't see any way this can add anundefinedvalue to the array ❌Line 86
items.push({ path, state, id })-> At worst, this could append an object like:{path: undefined, state: undefined, id: undefined}, but we wouldn't end up withundefineditself in theitemsarray. So not here either ❌Line 108
items = [{ path, state, id }];-> Similar to line 86, this can't causeundefinedto be in theitemsarray ❌Line 110
items[index] = { path, state, id };-> This can causeundefinedto be in theitemsarray ifindexis greater than the length of the array, like so:It seems this is our problem line, and tells us that the
indexis becoming greater than the length of the items array. I believe I found the only location in this component where that could happen, and fix it in this PR.Let me know if you have questions or concerns! If anyone from the community wants to help out with providing testing or consistent reproduction steps, that would be great!