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

Error: "Could not find node with id "266" in commit tree" #16446

Closed
ldcc-juj opened this issue Aug 19, 2019 · 9 comments
Closed

Error: "Could not find node with id "266" in commit tree" #16446

ldcc-juj opened this issue Aug 19, 2019 · 9 comments

Comments

@ldcc-juj
Copy link

PLEASE INCLUDE REPRO INSTRUCTIONS AND EXAMPLE CODE

I got this error when I click 'Ranked'.


Please do not remove the text below this line

DevTools version: 4.0.4-3c6a219

Call stack: at chrome-extension://fmkadmapgofadopljbjfkapdkoienihi/build/main.js:11:11441
at Map.forEach ()
at commitIndex (chrome-extension://fmkadmapgofadopljbjfkapdkoienihi/build/main.js:11:11387)
at e.getRankedChartData (chrome-extension://fmkadmapgofadopljbjfkapdkoienihi/build/main.js:11:11920)
at _i (chrome-extension://fmkadmapgofadopljbjfkapdkoienihi/build/main.js:56:277123)
at Ha (chrome-extension://fmkadmapgofadopljbjfkapdkoienihi/build/main.js:43:55890)
at Xl (chrome-extension://fmkadmapgofadopljbjfkapdkoienihi/build/main.js:43:98280)
at Hl (chrome-extension://fmkadmapgofadopljbjfkapdkoienihi/build/main.js:43:84255)
at Fl (chrome-extension://fmkadmapgofadopljbjfkapdkoienihi/build/main.js:43:81285)
at chrome-extension://fmkadmapgofadopljbjfkapdkoienihi/build/main.js:43:25363

Component stack: in _i
in div
in div
in div
in Or
in Unknown
in n
in Unknown
in div
in div
in Ha
in le
in ve
in ko
in Ul

@bvaughn
Copy link
Contributor

bvaughn commented Aug 19, 2019

More detailed repro steps would be appreciated. What did you do up util you clicked "Ranked"?

@Apollinaire
Copy link

Same here. I got this problem today on this profiling: https://gist.github.com/Apollinaire/effeb23efae1d603e9e0b6a8e39132e5
First time i had the error it said id 273, then I refreshed the page, the profiling was still present so I downloaded it to see if just the profiling reproduces the error. It does, but now it says id 257.
Reproduction: I just opened the profiler, recorded, fiddled with my app, stopped the record, then looked at the commits. Everything was fine, until I clicked on the Ranked tab. I instantly saw the error message that got me here.
Version: React Developer Tools 4.2.1 (11/27/2019) Adds React debugging tools to the Chrome Developer Tools. Created from revision 3816ae7c3 on 11/27/2019.
Hope that helps

@bvaughn
Copy link
Contributor

bvaughn commented Dec 10, 2019

@Apollinaire Thank you for sharing the profile JSON and extra info! 🙇

@bvaughn
Copy link
Contributor

bvaughn commented Jan 2, 2020

Digging into the repro this morning.

Looks like the missing Fiber (257) is in the data, but according to the operations array, it doesn't get added until the second commit. However the profiler data for the commit references it in the first commit. Not accounting for that, there seem to be other Fibers that are also off by one like this.

Looking closer at the operations array, I see 5 commits even though there are only 3 commitData entries. Two of the operations entries stand out because of their small size:

// For more info see
// https://github.com/facebook/react/blob/master/packages/react-devtools/OVERVIEW.md
[
  1, // renderer id
  1, // root id
  0  // string table size
],

I think this might be the key to the problem. I think these commits are bailouts so the profiler is ignoring them. Except it's also indexing their operations arrays - so it's ending up throwing things off. If I manually adjust the profiler data to remove these two no-op entries, the data loads and parses without any errors.

To add more clarity, I think what is happening is that the backend realizes the commit was empty and so it does not record any profiler data for it:

if (isProfiling && isProfilingSupported && !didBailoutAtRoot) {

if (isProfiling && isProfilingSupported && !didBailoutAtRoot) {

But it does still flush the operations to the front end:

We have a check for this case which actually seems to cause this problem rather than prevent it:

// If we're currently profiling, send an "operations" method even if there are no mutations to the tree.
// The frontend needs this no-op info to know how to reconstruct the tree for each commit,
// even if a particular commit didn't change the shape of the tree.
if (!isProfiling) {
return;
}

I have a solid lead now on how to dig into this! 😄

@bvaughn
Copy link
Contributor

bvaughn commented Jan 2, 2020

The specific bailout paths I linked to above were added after this issue started happening (#17253) but I think the general idea of what's causing this bug is still correct.

In fact, the new test I added for #17253 demonstrates the bug path I mentioned above (so I'll need to fix that). This may account for why we're seeing more reports of this issue recently. The bug was clearly around before that change too though, so I'll need to find the other path.

The didBailoutAtRoot condition and the later check to see if anything was newly mounted or unmounted aren't really the same. (Both can happen in the same commit but not necessarily.) For example, just re-rendering the same tree at the root level will result in an operations "empty" array looking like the one above, but would not match the recently-added "bailout" check.

bvaughn pushed a commit to bvaughn/react that referenced this issue Jan 2, 2020
The Profiler stores:

1. A snapshot of the React tree when profiling started
2. The operations array for each commit
3. Profiling metadata (e.g. durations, what changed, etc) for each commit

It uses this information (snapshot + operations diff) to reconstruct the state of the application for a given commit as it's viewed in the Profiler UI. Because of this, it's very important that the operations and metadata arrays align. If they don't align, the profiler will be unable to correctly reconstruct the tree, and it will likely throw errors (like 'Could not find node…')

facebook#16446 tracks a long-standing bug where these two arrays get misaligned. I am still not entirely sure what causes this bug, but with PR facebook#17253, I exacerbated things by introducing another potential way for it to happen. This PR addresses the regression at least (and adds test coverage for it).

I will follow up this afternoon on the original facebook#16446 issue. I think I may have a lead on what's happening at least, if not exactly an idea of how to reproduce it.
@bvaughn
Copy link
Contributor

bvaughn commented Jan 2, 2020

@Apollinaire I don't suppose you are able to repro this broken profile export? If so, is there any chance I could see the code that reproduces it? Might help me to track down exactly why it's happening.

bvaughn pushed a commit that referenced this issue Jan 3, 2020
The Profiler stores:

1. A snapshot of the React tree when profiling started
2. The operations array for each commit
3. Profiling metadata (e.g. durations, what changed, etc) for each commit

It uses this information (snapshot + operations diff) to reconstruct the state of the application for a given commit as it's viewed in the Profiler UI. Because of this, it's very important that the operations and metadata arrays align. If they don't align, the profiler will be unable to correctly reconstruct the tree, and it will likely throw errors (like 'Could not find node…')

#16446 tracks a long-standing bug where these two arrays get misaligned. I am still not entirely sure what causes this bug, but with PR #17253, I exacerbated things by introducing another potential way for it to happen. This PR addresses the regression at least (and adds test coverage for it).

I will follow up this afternoon on the original #16446 issue. I think I may have a lead on what's happening at least, if not exactly an idea of how to reproduce it.
@bvaughn
Copy link
Contributor

bvaughn commented Jan 3, 2020

FYI v4.4 (released today) should greatly improve this issue, but will not fix it entirely. I'm still working on it.

bvaughn pushed a commit to bvaughn/react that referenced this issue Jan 3, 2020
Previously, DevTools filtered empty commits on the backend, while profiling, through the use of a bailout heuristic that React currently happens to use. This approach was brittle and may have exacerbated the long-standing Profiler bug facebook#16446.

This PR removes that heuristic and adds as a post-processing filtering pass instead. This removes the coupling between DevTools and a React internal implementation detail that may change.

I believe DevTools has two choices of criteria for this filtering:
* Filter commits that have no actual duration metadata.
* Filter commits that have no recorded operations (no mutations to the tree, no changed tree base durations).

I chose the first option, filtering by commits that have no reported metadata. It will miss an edge case, e.g. , but we would have nothing meaningful to show in the Profiler for those cases anyway. (This particular edge case is why one of the snapshots changed with this commit.)

The second option, filtering by recorded operations, could potentially miss a more important edge case: where a component *did* render, but its  didn't change. (In that event, there would be no operations to send.)
@bvaughn
Copy link
Contributor

bvaughn commented Jan 5, 2020

Hopefully fixed by #17771

@bvaughn bvaughn closed this as completed Jan 5, 2020
bvaughn pushed a commit that referenced this issue Jan 5, 2020
Previously, DevTools filtered empty commits on the backend, while profiling, through the use of a bailout heuristic that React currently happens to use. This approach was brittle and may have exacerbated the long-standing Profiler bug #16446.

This PR removes that heuristic and adds as a post-processing filtering pass instead. This removes the coupling between DevTools and a React internal implementation detail that may change.

I believe DevTools has two choices of criteria for this filtering:
* Filter commits that have no actual duration metadata.
* Filter commits that have no recorded operations (no mutations to the tree, no changed tree base durations).

I chose the first option, filtering by commits that have no reported metadata. It will miss an edge case, e.g. , but we would have nothing meaningful to show in the Profiler for those cases anyway. (This particular edge case is why one of the snapshots changed with this commit.)

The second option, filtering by recorded operations, could potentially miss a more important edge case: where a component *did* render, but its  didn't change. (In that event, there would be no operations to send.)
@Apollinaire
Copy link

@bvaughn I was able to repro it a couple of times when I submitted my first comment here. I have not tried to do it again since I can see that you have (hopefully) found a solution.
And yes, the original code that was profiled is private.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants