Skip to content

[Bugfix] Incorrect return pointer on deleted fiber #20942

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

[Bugfix] Incorrect return pointer on deleted fiber #20942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80076d7
Add failing regression test
acdlite Mar 5, 2021
01ece92
Set return pointer of deleted tree
acdlite Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 189 additions & 0 deletions packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,146 @@

let React;
let ReactNoop;
let Scheduler;
let Suspense;
let SuspenseList;
let getCacheForType;
let caches;
let seededCache;

beforeEach(() => {
React = require('react');
ReactNoop = require('react-noop-renderer');
Scheduler = require('scheduler');

Suspense = React.Suspense;
SuspenseList = React.unstable_SuspenseList;

getCacheForType = React.unstable_getCacheForType;

caches = [];
seededCache = null;
});

function createTextCache() {
if (seededCache !== null) {
// Trick to seed a cache before it exists.
// TODO: Need a built-in API to seed data before the initial render (i.e.
// not a refresh because nothing has mounted yet).
const cache = seededCache;
seededCache = null;
return cache;
}

const data = new Map();
const version = caches.length + 1;
const cache = {
version,
data,
resolve(text) {
const record = data.get(text);
if (record === undefined) {
const newRecord = {
status: 'resolved',
value: text,
};
data.set(text, newRecord);
} else if (record.status === 'pending') {
const thenable = record.value;
record.status = 'resolved';
record.value = text;
thenable.pings.forEach(t => t());
}
},
reject(text, error) {
const record = data.get(text);
if (record === undefined) {
const newRecord = {
status: 'rejected',
value: error,
};
data.set(text, newRecord);
} else if (record.status === 'pending') {
const thenable = record.value;
record.status = 'rejected';
record.value = error;
thenable.pings.forEach(t => t());
}
},
};
caches.push(cache);
return cache;
}

function readText(text) {
const textCache = getCacheForType(createTextCache);
const record = textCache.data.get(text);
if (record !== undefined) {
switch (record.status) {
case 'pending':
Scheduler.unstable_yieldValue(`Suspend! [${text}]`);
throw record.value;
case 'rejected':
Scheduler.unstable_yieldValue(`Error! [${text}]`);
throw record.value;
case 'resolved':
return textCache.version;
}
} else {
Scheduler.unstable_yieldValue(`Suspend! [${text}]`);

const thenable = {
pings: [],
then(resolve) {
if (newRecord.status === 'pending') {
thenable.pings.push(resolve);
} else {
Promise.resolve().then(() => resolve(newRecord.value));
}
},
};

const newRecord = {
status: 'pending',
value: thenable,
};
textCache.data.set(text, newRecord);

throw thenable;
}
}

function Text({text}) {
Scheduler.unstable_yieldValue(text);
return <span prop={text} />;
}

function AsyncText({text, showVersion}) {
const version = readText(text);
const fullText = showVersion ? `${text} [v${version}]` : text;
Scheduler.unstable_yieldValue(fullText);
return <span prop={fullText} />;
}

// function seedNextTextCache(text) {
// if (seededCache === null) {
// seededCache = createTextCache();
// }
// seededCache.resolve(text);
// }

function resolveMostRecentTextCache(text) {
if (caches.length === 0) {
throw Error('Cache does not exist.');
} else {
// Resolve the most recently created cache. An older cache can by
// resolved with `caches[index].resolve(text)`.
caches[caches.length - 1].resolve(text);
}
}

const resolveText = resolveMostRecentTextCache;

// Don't feel too guilty if you have to delete this test.
// @gate dfsEffectsRefactor
// @gate __DEV__
Expand Down Expand Up @@ -59,3 +193,58 @@ test('warns in DEV if return pointer is inconsistent', async () => {
'Internal React error: Return pointer is inconsistent with parent.',
);
});

// @gate experimental
// @gate enableCache
test('regression (#20932): return pointer is correct before entering deleted tree', async () => {
// Based on a production bug. Designed to trigger a very specific
// implementation path.
function Tail() {
return (
<Suspense fallback={<Text text="Loading Tail..." />}>
<Text text="Tail" />
</Suspense>
);
}

function App() {
return (
<SuspenseList revealOrder="forwards">
<Suspense fallback={<Text text="Loading Async..." />}>
<Async />
</Suspense>
<Tail />
</SuspenseList>
);
}

let setAsyncText;
function Async() {
const [c, _setAsyncText] = React.useState(0);
setAsyncText = _setAsyncText;
return <AsyncText text={c} />;
}

const root = ReactNoop.createRoot();
await ReactNoop.act(async () => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded([
'Suspend! [0]',
'Loading Async...',
'Loading Tail...',
]);
await ReactNoop.act(async () => {
resolveText(0);
});
expect(Scheduler).toHaveYielded([0, 'Tail']);
await ReactNoop.act(async () => {
setAsyncText(x => x + 1);
});
expect(Scheduler).toHaveYielded([
'Suspend! [1]',
'Loading Async...',
'Suspend! [1]',
'Loading Async...',
]);
});
4 changes: 4 additions & 0 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1964,6 +1964,10 @@ function commitMutationEffects_begin(
if (deletions !== null) {
for (let i = 0; i < deletions.length; i++) {
const childToDelete = deletions[i];
// When deleting a host node, we follow the return path to find the
// neartest mounted host parent. Which means the return pointer of
// the deleted tree must be correct.
Copy link
Collaborator

@sebmarkbage sebmarkbage Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of doing that on-demand, we could do that eagerly here and then pass in the parent.

It's likely to be at least one DOM node deleted. If it's more than one, then it's unnecessary to backtrack more than once. We meant to do that optimization for that reason at some point anyway.

Or better yet, keep track of it on the stack since we're traversing through the parent anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't use recursion for this function yet (was reverted because of slow down in open source build) so it'd have to be a virtual stack.

So I'll go with option 1 (eagerly find the current parent) for now and leave a TODO for when this recursive traversal is converted to use the JS stack.

childToDelete.return = fiber;
Copy link
Contributor

Choose a reason for hiding this comment

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

In my testing, the return pointer was not null the first time something was deleted. It ended up being null later b'c we tried to delete the same thing twice.

I'm wondering if the repro I had (the larger one that involved Feed and was more touch and go) is a different problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It’s always non-null at this particular location, but before the fix it would sometimes point to the wrong alternate. Does that describe what you were seeing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, pretty sure I was seeing a double deletion, and the pointer was cleared in detach mutation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, I checked again and it was a double deletion.

Fix here: #20942

if (__DEV__) {
invokeGuardedCallback(
null,
Expand Down
4 changes: 4 additions & 0 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1964,6 +1964,10 @@ function commitMutationEffects_begin(
if (deletions !== null) {
for (let i = 0; i < deletions.length; i++) {
const childToDelete = deletions[i];
// When deleting a host node, we follow the return path to find the
// neartest mounted host parent. Which means the return pointer of
// the deleted tree must be correct.
childToDelete.return = fiber;
if (__DEV__) {
invokeGuardedCallback(
null,
Expand Down