Fix edge-case Fast Refresh bug that caused Fibers with warnings/errors to be untracked prematurely (#21536)
Refactor error/warning count tracking to avoid pre-allocating an ID for Fibers that aren't yet mounted. Instead, we store a temporary reference to the Fiber itself and later check to see if it successfully mounted before merging pending error/warning counts. This avoids a problematic edge case where a force-remounted Fiber (from Fast Refresh) caused us to untrack a Fiber that was still mounted, resulting in a DevTools error if that Fiber was inspected in the Components tab.
Brian Vaughn committed
May 20, 2021 at 08:24 UTC
b8bbb6a13d0d0fc93bd1bf51bd66d626aad84672
2 files changed
+150
-93
packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
+11
-15
@@ -1946,15 +1946,11 @@ describe('InspectedElement', () => {
1946
});
1947
1948
describe('inline errors and warnings', () => {
1949
- // Some actions require the Fiber id.
1950
- // In those instances you might want to make assertions based on the ID instead of the index.
1951
- function getErrorsAndWarningsForElement(id: number) {
1952
- const index = ((store.getIndexOfElementID(id): any): number);
1953
- return getErrorsAndWarningsForElementAtIndex(index);
1954
- }
1955
-
1949
async function getErrorsAndWarningsForElementAtIndex(index) {
1950
const id = ((store.getElementIDAtIndex(index): any): number);
1951
+ if (id == null) {
1952
+ throw Error(`Element at index "${index}"" not found in store`);
1953
+ }
1954
1955
let errors = null;
1956
let warnings = null;
@@ -2222,8 +2218,8 @@ describe('InspectedElement', () => {
2218
jest.runOnlyPendingTimers();
2219
2220
let data = [
2225
- await getErrorsAndWarningsForElement(1),
2226
- await getErrorsAndWarningsForElement(2),
2221
+ await getErrorsAndWarningsForElementAtIndex(0),
2222
+ await getErrorsAndWarningsForElementAtIndex(1),
2223
];
2224
expect(data).toMatchInlineSnapshot(`
2225
Array [
@@ -2260,8 +2256,8 @@ describe('InspectedElement', () => {
2256
jest.runOnlyPendingTimers();
2257
2258
data = [
2263
- await getErrorsAndWarningsForElement(1),
2264
- await getErrorsAndWarningsForElement(2),
2259
+ await getErrorsAndWarningsForElementAtIndex(0),
2260
+ await getErrorsAndWarningsForElementAtIndex(1),
2261
];
2262
expect(data).toMatchInlineSnapshot(`
2263
Array [
@@ -2319,8 +2315,8 @@ describe('InspectedElement', () => {
2315
jest.runOnlyPendingTimers();
2316
2317
let data = [
2322
- await getErrorsAndWarningsForElement(1),
2323
- await getErrorsAndWarningsForElement(2),
2318
+ await getErrorsAndWarningsForElementAtIndex(0),
2319
+ await getErrorsAndWarningsForElementAtIndex(1),
2320
];
2321
expect(data).toMatchInlineSnapshot(`
2322
Array [
@@ -2357,8 +2353,8 @@ describe('InspectedElement', () => {
2353
jest.runOnlyPendingTimers();
2354
2355
data = [
2360
- await getErrorsAndWarningsForElement(1),
2361
- await getErrorsAndWarningsForElement(2),
2356
+ await getErrorsAndWarningsForElementAtIndex(0),
2357
+ await getErrorsAndWarningsForElementAtIndex(1),
2358
];
2359
expect(data).toMatchInlineSnapshot(`
2360
Array [
packages/react-devtools-shared/src/backend/renderer.js
+139
-78
@@ -564,50 +564,82 @@ export function attach(
564
typeof setSuspenseHandler === 'function' &&
565
typeof scheduleUpdate === 'function';
566
567
- // Set of Fibers (IDs) with recently changed number of error/warning messages.
568
- const fibersWithChangedErrorOrWarningCounts: Set<number> = new Set();
567
+ // Tracks Fibers with recently changed number of error/warning messages.
568
+ // These collections store the Fiber rather than the ID,
569
+ // in order to avoid generating an ID for Fibers that never get mounted
570
+ // (due to e.g. Suspense or error boundaries).
571
+ // onErrorOrWarning() adds Fibers and recordPendingErrorsAndWarnings() later clears them.
572
+ const fibersWithChangedErrorOrWarningCounts: Set<Fiber> = new Set();
573
+ const pendingFiberToErrorsMap: Map<Fiber, Map<string, number>> = new Map();
574
+ const pendingFiberToWarningsMap: Map<Fiber, Map<string, number>> = new Map();
575
576
// Mapping of fiber IDs to error/warning messages and counts.
571
- const fiberToErrorsMap: Map<number, Map<string, number>> = new Map();
572
- const fiberToWarningsMap: Map<number, Map<string, number>> = new Map();
577
+ const fiberIDToErrorsMap: Map<number, Map<string, number>> = new Map();
578
+ const fiberIDToWarningsMap: Map<number, Map<string, number>> = new Map();
579
580
function clearErrorsAndWarnings() {
581
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
576
- for (const id of fiberToErrorsMap.keys()) {
577
- fibersWithChangedErrorOrWarningCounts.add(id);
578
- updateMostRecentlyInspectedElementIfNecessary(id);
582
+ for (const id of fiberIDToErrorsMap.keys()) {
583
+ const fiber = idToArbitraryFiberMap.get(id);
584
+ if (fiber != null) {
585
+ fibersWithChangedErrorOrWarningCounts.add(fiber);
586
+ updateMostRecentlyInspectedElementIfNecessary(id);
587
+ }
588
}
589
590
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
582
- for (const id of fiberToWarningsMap.keys()) {
583
- fibersWithChangedErrorOrWarningCounts.add(id);
584
- updateMostRecentlyInspectedElementIfNecessary(id);
591
+ for (const id of fiberIDToWarningsMap.keys()) {
592
+ const fiber = idToArbitraryFiberMap.get(id);
593
+ if (fiber != null) {
594
+ fibersWithChangedErrorOrWarningCounts.add(fiber);
595
+ updateMostRecentlyInspectedElementIfNecessary(id);
596
+ }
597
}
598
587
- fiberToErrorsMap.clear();
588
- fiberToWarningsMap.clear();
599
+ fiberIDToErrorsMap.clear();
600
+ fiberIDToWarningsMap.clear();
601
602
flushPendingEvents();
603
}
604
593
- function clearErrorsForFiberID(id: number) {
594
- if (fiberToErrorsMap.has(id)) {
595
- fiberToErrorsMap.delete(id);
596
- fibersWithChangedErrorOrWarningCounts.add(id);
597
- flushPendingEvents();
598
- }
605
+ function clearMessageCountHelper(
606
+ fiberID: number,
607
+ pendingFiberToMessageCountMap: Map<Fiber, Map<string, number>>,
608
+ fiberIDToMessageCountMap: Map<number, Map<string, number>>,
609
+ ) {
610
+ const fiber = idToArbitraryFiberMap.get(fiberID);
611
+ if (fiber != null) {
612
+ // Throw out any pending changes.
613
+ pendingFiberToErrorsMap.delete(fiber);
614
600
- updateMostRecentlyInspectedElementIfNecessary(id);
601
- }
615
+ if (fiberIDToMessageCountMap.has(fiberID)) {
616
+ fiberIDToMessageCountMap.delete(fiberID);
617
+
618
+ // If previous flushed counts have changed, schedule an update too.
619
+ fibersWithChangedErrorOrWarningCounts.add(fiber);
620
+ flushPendingEvents();
621
603
- function clearWarningsForFiberID(id: number) {
604
- if (fiberToWarningsMap.has(id)) {
605
- fiberToWarningsMap.delete(id);
606
- fibersWithChangedErrorOrWarningCounts.add(id);
607
- flushPendingEvents();
622
+ updateMostRecentlyInspectedElementIfNecessary(fiberID);
623
+ } else {
624
+ fibersWithChangedErrorOrWarningCounts.delete(fiber);
625
+ }
626
}
627
+ }
628
610
- updateMostRecentlyInspectedElementIfNecessary(id);
629
+ function clearErrorsForFiberID(fiberID: number) {
630
+ clearMessageCountHelper(
631
+ fiberID,
632
+ pendingFiberToErrorsMap,
633
+ fiberIDToErrorsMap,
634
+ );
635
+ }
636
+
637
+ function clearWarningsForFiberID(fiberID: number) {
638
+ clearMessageCountHelper(
639
+ fiberID,
640
+ pendingFiberToWarningsMap,
641
+ fiberIDToWarningsMap,
642
+ );
643
}
644
645
function updateMostRecentlyInspectedElementIfNecessary(
@@ -628,36 +660,24 @@ export function attach(
660
args: $ReadOnlyArray<any>,
661
): void {
662
const message = format(...args);
631
-
632
- // Note that by calling these functions we may be creating the ID for the first time.
633
- // If the Fiber is then never mounted, we are responsible for cleaning up after ourselves.
634
- // This is important because getOrGenerateFiberID() stores a Fiber in a couple of local Maps.
635
- // If the Fiber never mounts and we don't clean up after this code, we could leak.
636
- // Fortunately we would only leak Fibers that have errors/warnings associated with them,
637
- // which is hopefully only a small set and only in DEV mode– but this is still not great.
638
- // We should clean up Fibers like this when flushing; see recordPendingErrorsAndWarnings().
639
- const fiberID = getOrGenerateFiberID(fiber);
640
-
663
if (__DEBUG__) {
664
debug('onErrorOrWarning', fiber, null, `${type}: "${message}"`);
665
}
666
667
// Mark this Fiber as needed its warning/error count updated during the next flush.
646
- fibersWithChangedErrorOrWarningCounts.add(fiberID);
668
+ fibersWithChangedErrorOrWarningCounts.add(fiber);
669
648
- // Update the error/warning messages and counts for the Fiber.
649
- const fiberMap = type === 'error' ? fiberToErrorsMap : fiberToWarningsMap;
650
- const messageMap = fiberMap.get(fiberID);
670
+ // Track the warning/error for later.
671
+ const fiberMap =
672
+ type === 'error' ? pendingFiberToErrorsMap : pendingFiberToWarningsMap;
673
+ const messageMap = fiberMap.get(fiber);
674
if (messageMap != null) {
675
const count = messageMap.get(message) || 0;
676
messageMap.set(message, count + 1);
677
} else {
655
- fiberMap.set(fiberID, new Map([[message, 1]]));
678
+ fiberMap.set(fiber, new Map([[message, 1]]));
679
}
680
658
- // If this Fiber is currently being inspected, mark it as needing an udpate as well.
659
- updateMostRecentlyInspectedElementIfNecessary(fiberID);
660
-
681
// Passive effects may trigger errors or warnings too;
682
// In this case, we should wait until the rest of the passive effects have run,
683
// but we shouldn't wait until the next commit because that might be a long time.
@@ -1497,53 +1517,94 @@ export function attach(
1517
1518
function reevaluateErrorsAndWarnings() {
1519
fibersWithChangedErrorOrWarningCounts.clear();
1500
- fiberToErrorsMap.forEach((countMap, fiberID) => {
1501
- fibersWithChangedErrorOrWarningCounts.add(fiberID);
1520
+ fiberIDToErrorsMap.forEach((countMap, fiberID) => {
1521
+ const fiber = idToArbitraryFiberMap.get(fiberID);
1522
+ if (fiber != null) {
1523
+ fibersWithChangedErrorOrWarningCounts.add(fiber);
1524
+ }
1525
});
1503
- fiberToWarningsMap.forEach((countMap, fiberID) => {
1504
- fibersWithChangedErrorOrWarningCounts.add(fiberID);
1526
+ fiberIDToWarningsMap.forEach((countMap, fiberID) => {
1527
+ const fiber = idToArbitraryFiberMap.get(fiberID);
1528
+ if (fiber != null) {
1529
+ fibersWithChangedErrorOrWarningCounts.add(fiber);
1530
+ }
1531
});
1532
recordPendingErrorsAndWarnings();
1533
}
1534
1509
- function recordPendingErrorsAndWarnings() {
1510
- clearPendingErrorsAndWarningsAfterDelay();
1535
+ function mergeMapsAndGetCountHelper(
1536
+ fiber: Fiber,
1537
+ fiberID: number,
1538
+ pendingFiberToMessageCountMap: Map<Fiber, Map<string, number>>,
1539
+ fiberIDToMessageCountMap: Map<number, Map<string, number>>,
1540
+ ): number {
1541
+ let newCount = 0;
1542
1512
- fibersWithChangedErrorOrWarningCounts.forEach(fiberID => {
1513
- const fiber = idToArbitraryFiberMap.get(fiberID);
1514
- if (fiber != null) {
1515
- // Don't send updates for Fibers that didn't mount due to e.g. Suspense or an error boundary.
1516
- // We may also need to clean up after ourselves to avoid leaks.
1517
- // See inline comments in onErrorOrWarning() for more info.
1518
- if (isFiberMountedImpl(fiber) !== MOUNTED) {
1519
- untrackFiberID(fiber);
1520
- return;
1521
- }
1543
+ let messageCountMap = fiberIDToMessageCountMap.get(fiberID);
1544
1523
- let errorCount = 0;
1524
- let warningCount = 0;
1545
+ const pendingMessageCountMap = pendingFiberToMessageCountMap.get(fiber);
1546
+ if (pendingMessageCountMap != null) {
1547
+ if (messageCountMap == null) {
1548
+ messageCountMap = pendingMessageCountMap;
1549
1526
- if (!shouldFilterFiber(fiber)) {
1527
- const errorCountsMap = fiberToErrorsMap.get(fiberID);
1528
- const warningCountsMap = fiberToWarningsMap.get(fiberID);
1550
+ fiberIDToMessageCountMap.set(fiberID, pendingMessageCountMap);
1551
+ } else {
1552
+ // This Flow refinement should not be necessary and yet...
1553
+ const refinedMessageCountMap = ((messageCountMap: any): Map<
1554
+ string,
1555
+ number,
1556
+ >);
1557
+
1558
+ pendingMessageCountMap.forEach((pendingCount, message) => {
1559
+ const previousCount = refinedMessageCountMap.get(message) || 0;
1560
+ refinedMessageCountMap.set(message, previousCount + pendingCount);
1561
+ });
1562
+ }
1563
+ }
1564
1530
- if (errorCountsMap != null) {
1531
- errorCountsMap.forEach(count => {
1532
- errorCount += count;
1533
- });
1534
- }
1535
- if (warningCountsMap != null) {
1536
- warningCountsMap.forEach(count => {
1537
- warningCount += count;
1538
- });
1539
- }
1540
- }
1565
+ if (!shouldFilterFiber(fiber)) {
1566
+ if (messageCountMap != null) {
1567
+ messageCountMap.forEach(count => {
1568
+ newCount += count;
1569
+ });
1570
+ }
1571
+ }
1572
+
1573
+ pendingFiberToMessageCountMap.delete(fiber);
1574
+
1575
+ return newCount;
1576
+ }
1577
+
1578
+ function recordPendingErrorsAndWarnings() {
1579
+ clearPendingErrorsAndWarningsAfterDelay();
1580
+
1581
+ fibersWithChangedErrorOrWarningCounts.forEach(fiber => {
1582
+ const fiberID = getFiberIDUnsafe(fiber);
1583
+ if (fiberID === null) {
1584
+ // Don't send updates for Fibers that didn't mount due to e.g. Suspense or an error boundary.
1585
+ } else {
1586
+ const errorCount = mergeMapsAndGetCountHelper(
1587
+ fiber,
1588
+ fiberID,
1589
+ pendingFiberToErrorsMap,
1590
+ fiberIDToErrorsMap,
1591
+ );
1592
+ const warningCount = mergeMapsAndGetCountHelper(
1593
+ fiber,
1594
+ fiberID,
1595
+ pendingFiberToWarningsMap,
1596
+ fiberIDToWarningsMap,
1597
+ );
1598
1599
pushOperation(TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS);
1600
pushOperation(fiberID);
1601
pushOperation(errorCount);
1602
pushOperation(warningCount);
1603
}
1604
+
1605
+ // Always clean up so that we don't leak.
1606
+ pendingFiberToErrorsMap.delete(fiber);
1607
+ pendingFiberToWarningsMap.delete(fiber);
1608
});
1609
fibersWithChangedErrorOrWarningCounts.clear();
1610
}
@@ -3012,8 +3073,8 @@ export function attach(
3073
rootType = fiberRoot._debugRootType;
3074
}
3075
3015
- const errors = fiberToErrorsMap.get(id) || new Map();
3016
- const warnings = fiberToWarningsMap.get(id) || new Map();
3076
+ const errors = fiberIDToErrorsMap.get(id) || new Map();
3077
+ const warnings = fiberIDToWarningsMap.get(id) || new Map();
3078
3079
return {
3080
id,