Skip to content

Commit 002e585

Browse files
gnoffAndyPengc12
gnoff
authored and
AndyPengc12
committed
[Fizz] Prevent uncloned large precomputed chunks without relying on render-time assertions (facebook#28568)
A while back we implemented a heuristic that if a chunk was large it was assumed to be produced by the render and thus was safe to stream which results in transferring the underlying object memory. Later we ran into an issue where a precomputed chunk grew large enough to trigger this hueristic and it started causing renders to fail because once a second render had occurred the precomputed chunk would not have an underlying buffer of bytes to send and these bytes would be omitted from the stream. We implemented a technique to detect large precomputed chunks and we enforced that these always be cloned before writing. Unfortunately our test coverage was not perfect and there has been for a very long time now a usage pattern where if you complete a boundary in one flush and then complete a boundary that has stylehsheet dependencies in another flush you can get a large precomputed chunk that was not being cloned to be sent twice causing streaming errors. I've thought about why we even went with this solution in the first place and I think it was a mistake. It relies on a dev only check to catch paired with potentially version specific order of operations on the streaming side. This is too unreliable. Additionally the low limit of view size for Edge is not used in Node.js but there is not real justification for this. In this change I updated the view size for edge streaming to match Node at 2048 bytes which is still relatively small and we have no data one way or another to preference 512 over this. Then I updated the assertion logic to error anytime a precomputed chunk exceeds the size. This eliminates the need to clone these chunks by just making sure our view size is always larger than the largest precomputed chunk we can possibly write. I'm generally in favor of this for a few reasons. First, we'll always know during testing whether we've violated the limit as long as we exercise each stream config because the precomputed chunks are created in module scope. Second, we can always split up large chunks so making sure the precomptued chunk is smaller than whatever view size we actually desire is relatively trivial.
1 parent e90046a commit 002e585

File tree

10 files changed

+77
-94
lines changed

10 files changed

+77
-94
lines changed

‎packages/react-dom-bindings/src/server/ReactDOMLegacyServerStreamConfig.js‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,6 @@ export function typedArrayToBinaryChunk(
5858
throw new Error('Not implemented.');
5959
}
6060

61-
export function clonePrecomputedChunk(
62-
chunk: PrecomputedChunk,
63-
): PrecomputedChunk {
64-
return chunk;
65-
}
66-
6761
export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
6862
throw new Error('Not implemented.');
6963
}

‎packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ import {
5050
writeChunkAndReturn,
5151
stringToChunk,
5252
stringToPrecomputedChunk,
53-
clonePrecomputedChunk,
5453
} from 'react-server/src/ReactServerStreamConfig';
5554
import {
5655
resolveRequest,
@@ -4267,15 +4266,13 @@ export function writeCompletedBoundaryInstruction(
42674266
) {
42684267
resumableState.instructions |=
42694268
SentStyleInsertionFunction | SentCompleteBoundaryFunction;
4270-
writeChunk(
4271-
destination,
4272-
clonePrecomputedChunk(completeBoundaryWithStylesScript1FullBoth),
4273-
);
4269+
writeChunk(destination, completeBoundaryWithStylesScript1FullBoth);
42744270
} else if (
42754271
(resumableState.instructions & SentStyleInsertionFunction) ===
42764272
NothingSent
42774273
) {
42784274
resumableState.instructions |= SentStyleInsertionFunction;
4275+
42794276
writeChunk(destination, completeBoundaryWithStylesScript1FullPartial);
42804277
} else {
42814278
writeChunk(destination, completeBoundaryWithStylesScript1Partial);

‎packages/react-dom/src/__tests__/ReactDOMFloat-test.js‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -731,6 +731,65 @@ describe('ReactDOMFloat', () => {
731731
).toEqual(['<script src="src-of-external-runtime" async=""></script>']);
732732
});
733733

734+
// @gate enableFloat
735+
it('can send style insertion implementation independent of boundary commpletion instruction implementation', async () => {
736+
await act(() => {
737+
renderToPipeableStream(
738+
<html>
739+
<body>
740+
<Suspense fallback="loading foo...">
741+
<BlockedOn value="foo">foo</BlockedOn>
742+
</Suspense>
743+
<Suspense fallback="loading bar...">
744+
<BlockedOn value="bar">
745+
<link rel="stylesheet" href="bar" precedence="bar" />
746+
bar
747+
</BlockedOn>
748+
</Suspense>
749+
</body>
750+
</html>,
751+
).pipe(writable);
752+
});
753+
754+
expect(getMeaningfulChildren(document)).toEqual(
755+
<html>
756+
<head />
757+
<body>
758+
{'loading foo...'}
759+
{'loading bar...'}
760+
</body>
761+
</html>,
762+
);
763+
764+
await act(() => {
765+
resolveText('foo');
766+
});
767+
expect(getMeaningfulChildren(document)).toEqual(
768+
<html>
769+
<head />
770+
<body>
771+
foo
772+
{'loading bar...'}
773+
</body>
774+
</html>,
775+
);
776+
await act(() => {
777+
resolveText('bar');
778+
});
779+
expect(getMeaningfulChildren(document)).toEqual(
780+
<html>
781+
<head>
782+
<link rel="stylesheet" href="bar" data-precedence="bar" />
783+
</head>
784+
<body>
785+
foo
786+
{'loading bar...'}
787+
<link rel="preload" href="bar" as="style" />
788+
</body>
789+
</html>,
790+
);
791+
});
792+
734793
// @gate enableFloat
735794
it('can avoid inserting a late stylesheet if it already rendered on the client', async () => {
736795
await act(() => {

‎packages/react-noop-renderer/src/ReactNoopFlightServer.js‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,6 @@ const ReactNoopFlightServer = ReactFlightServer({
4646
stringToPrecomputedChunk(content: string): Uint8Array {
4747
return textEncoder.encode(content);
4848
},
49-
clonePrecomputedChunk(chunk: Uint8Array): Uint8Array {
50-
return chunk;
51-
},
5249
isClientReference(reference: Object): boolean {
5350
return reference.$$typeof === Symbol.for('react.client.reference');
5451
},

‎packages/react-server/src/ReactServerStreamConfigBrowser.js‎

Lines changed: 6 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ export function flushBuffered(destination: Destination) {
2222
// transform streams. https://github.com/whatwg/streams/issues/960
2323
}
2424

25-
const VIEW_SIZE = 512;
25+
const VIEW_SIZE = 2048;
2626
let currentView = null;
2727
let writtenBytes = 0;
2828

@@ -40,15 +40,6 @@ export function writeChunk(
4040
}
4141

4242
if (chunk.byteLength > VIEW_SIZE) {
43-
if (__DEV__) {
44-
if (precomputedChunkSet.has(chunk)) {
45-
console.error(
46-
'A large precomputed chunk was passed to writeChunk without being copied.' +
47-
' Large chunks get enqueued directly and are not copied. This is incompatible with precomputed chunks because you cannot enqueue the same precomputed chunk twice.' +
48-
' Use "cloneChunk" to make a copy of this large precomputed chunk before writing it. This is a bug in React.',
49-
);
50-
}
51-
}
5243
// this chunk may overflow a single view which implies it was not
5344
// one that is cached by the streaming renderer. We will enqueu
5445
// it directly and expect it is not re-used
@@ -120,15 +111,15 @@ export function stringToChunk(content: string): Chunk {
120111
return textEncoder.encode(content);
121112
}
122113

123-
const precomputedChunkSet: Set<Chunk | BinaryChunk> = __DEV__
124-
? new Set()
125-
: (null: any);
126-
127114
export function stringToPrecomputedChunk(content: string): PrecomputedChunk {
128115
const precomputedChunk = textEncoder.encode(content);
129116

130117
if (__DEV__) {
131-
precomputedChunkSet.add(precomputedChunk);
118+
if (precomputedChunk.byteLength > VIEW_SIZE) {
119+
console.error(
120+
'precomputed chunks must be smaller than the view size configured for this host. This is a bug in React.',
121+
);
122+
}
132123
}
133124

134125
return precomputedChunk;
@@ -151,14 +142,6 @@ export function typedArrayToBinaryChunk(
151142
return content.byteLength > VIEW_SIZE ? buffer.slice() : buffer;
152143
}
153144

154-
export function clonePrecomputedChunk(
155-
precomputedChunk: PrecomputedChunk,
156-
): PrecomputedChunk {
157-
return precomputedChunk.byteLength > VIEW_SIZE
158-
? precomputedChunk.slice()
159-
: precomputedChunk;
160-
}
161-
162145
export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
163146
return chunk.byteLength;
164147
}

‎packages/react-server/src/ReactServerStreamConfigBun.js‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,6 @@ export function typedArrayToBinaryChunk(
7070
return content;
7171
}
7272

73-
export function clonePrecomputedChunk(
74-
chunk: PrecomputedChunk,
75-
): PrecomputedChunk {
76-
return chunk;
77-
}
78-
7973
export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
8074
return Buffer.byteLength(chunk, 'utf8');
8175
}

‎packages/react-server/src/ReactServerStreamConfigEdge.js‎

Lines changed: 6 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ export function flushBuffered(destination: Destination) {
2222
// transform streams. https://github.com/whatwg/streams/issues/960
2323
}
2424

25-
const VIEW_SIZE = 512;
25+
const VIEW_SIZE = 2048;
2626
let currentView = null;
2727
let writtenBytes = 0;
2828

@@ -40,15 +40,6 @@ export function writeChunk(
4040
}
4141

4242
if (chunk.byteLength > VIEW_SIZE) {
43-
if (__DEV__) {
44-
if (precomputedChunkSet.has(chunk)) {
45-
console.error(
46-
'A large precomputed chunk was passed to writeChunk without being copied.' +
47-
' Large chunks get enqueued directly and are not copied. This is incompatible with precomputed chunks because you cannot enqueue the same precomputed chunk twice.' +
48-
' Use "cloneChunk" to make a copy of this large precomputed chunk before writing it. This is a bug in React.',
49-
);
50-
}
51-
}
5243
// this chunk may overflow a single view which implies it was not
5344
// one that is cached by the streaming renderer. We will enqueu
5445
// it directly and expect it is not re-used
@@ -120,15 +111,15 @@ export function stringToChunk(content: string): Chunk {
120111
return textEncoder.encode(content);
121112
}
122113

123-
const precomputedChunkSet: Set<Chunk | BinaryChunk> = __DEV__
124-
? new Set()
125-
: (null: any);
126-
127114
export function stringToPrecomputedChunk(content: string): PrecomputedChunk {
128115
const precomputedChunk = textEncoder.encode(content);
129116

130117
if (__DEV__) {
131-
precomputedChunkSet.add(precomputedChunk);
118+
if (precomputedChunk.byteLength > VIEW_SIZE) {
119+
console.error(
120+
'precomputed chunks must be smaller than the view size configured for this host. This is a bug in React.',
121+
);
122+
}
132123
}
133124

134125
return precomputedChunk;
@@ -151,14 +142,6 @@ export function typedArrayToBinaryChunk(
151142
return content.byteLength > VIEW_SIZE ? buffer.slice() : buffer;
152143
}
153144

154-
export function clonePrecomputedChunk(
155-
precomputedChunk: PrecomputedChunk,
156-
): PrecomputedChunk {
157-
return precomputedChunk.byteLength > VIEW_SIZE
158-
? precomputedChunk.slice()
159-
: precomputedChunk;
160-
}
161-
162145
export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
163146
return chunk.byteLength;
164147
}

‎packages/react-server/src/ReactServerStreamConfigFB.js‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -60,12 +60,6 @@ export function typedArrayToBinaryChunk(
6060
throw new Error('Not implemented.');
6161
}
6262

63-
export function clonePrecomputedChunk(
64-
chunk: PrecomputedChunk,
65-
): PrecomputedChunk {
66-
return chunk;
67-
}
68-
6963
export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
7064
throw new Error('Not implemented.');
7165
}

‎packages/react-server/src/ReactServerStreamConfigNode.js‎

Lines changed: 4 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -99,15 +99,6 @@ function writeViewChunk(
9999
return;
100100
}
101101
if (chunk.byteLength > VIEW_SIZE) {
102-
if (__DEV__) {
103-
if (precomputedChunkSet && precomputedChunkSet.has(chunk)) {
104-
console.error(
105-
'A large precomputed chunk was passed to writeChunk without being copied.' +
106-
' Large chunks get enqueued directly and are not copied. This is incompatible with precomputed chunks because you cannot enqueue the same precomputed chunk twice.' +
107-
' Use "cloneChunk" to make a copy of this large precomputed chunk before writing it. This is a bug in React.',
108-
);
109-
}
110-
}
111102
// this chunk may overflow a single view which implies it was not
112103
// one that is cached by the streaming renderer. We will enqueu
113104
// it directly and expect it is not re-used
@@ -201,14 +192,14 @@ export function stringToChunk(content: string): Chunk {
201192
return content;
202193
}
203194

204-
const precomputedChunkSet = __DEV__ ? new Set<PrecomputedChunk>() : null;
205-
206195
export function stringToPrecomputedChunk(content: string): PrecomputedChunk {
207196
const precomputedChunk = textEncoder.encode(content);
208197

209198
if (__DEV__) {
210-
if (precomputedChunkSet) {
211-
precomputedChunkSet.add(precomputedChunk);
199+
if (precomputedChunk.byteLength > VIEW_SIZE) {
200+
console.error(
201+
'precomputed chunks must be smaller than the view size configured for this host. This is a bug in React.',
202+
);
212203
}
213204
}
214205

@@ -222,14 +213,6 @@ export function typedArrayToBinaryChunk(
222213
return new Uint8Array(content.buffer, content.byteOffset, content.byteLength);
223214
}
224215

225-
export function clonePrecomputedChunk(
226-
precomputedChunk: PrecomputedChunk,
227-
): PrecomputedChunk {
228-
return precomputedChunk.length > VIEW_SIZE
229-
? precomputedChunk.slice()
230-
: precomputedChunk;
231-
}
232-
233216
export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number {
234217
return typeof chunk === 'string'
235218
? Buffer.byteLength(chunk, 'utf8')

‎packages/react-server/src/forks/ReactServerStreamConfig.custom.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ export const closeWithError = $$$config.closeWithError;
4141
export const stringToChunk = $$$config.stringToChunk;
4242
export const stringToPrecomputedChunk = $$$config.stringToPrecomputedChunk;
4343
export const typedArrayToBinaryChunk = $$$config.typedArrayToBinaryChunk;
44-
export const clonePrecomputedChunk = $$$config.clonePrecomputedChunk;
4544
export const byteLengthOfChunk = $$$config.byteLengthOfChunk;
4645
export const byteLengthOfBinaryChunk = $$$config.byteLengthOfBinaryChunk;
4746
export const createFastHash = $$$config.createFastHash;

0 commit comments

Comments
 (0)