FluidFramework
FluidFramework copied to clipboard
Make handle parsing/decoding functions idempotent
Description
Fixes #8016. We need to port this back to RC3 and RC4 once merged to main.
For anyone implementing their own DDS, when they pick up an FF release with #19242 in it, they will likely be in a position where handles are going through parseHandles or serializer.decode twice.
We assumed this was idempotent but found that it is not, with two different codepaths needing to be fixed.
Reviewer Guidance
This is a superset of #19716. In that discussion you'll see reference made to some DDS Fuzz test failures for particular seeds. I was surprised to see those failures and did not get time to look into them, and eventually they disappeared.
I'm not sure if it's reckless to proceed with this without going back to find a repro of those failures. As noted, there have been several improvements to the DDS Fuzz harness - perhaps those failures were false positives.
⯆ @fluid-example/bundle-size-tests: -139 Bytes
| Metric Name | Baseline Size | Compare Size | Size Diff |
|---|---|---|---|
| aqueduct.js | 453.98 KB | 453.96 KB | ⯆ -17 Bytes |
| azureClient.js | 551.22 KB | 551.21 KB | ⯆ -16 Bytes |
| connectionState.js | 680 Bytes | 680 Bytes | ■ No change |
| containerRuntime.js | 257.82 KB | 257.82 KB | ■ No change |
| fluidFramework.js | 359.58 KB | 359.56 KB | ⯆ -21 Bytes |
| loader.js | 132.91 KB | 132.91 KB | ■ No change |
| map.js | 41.45 KB | 41.43 KB | ⯆ -16 Bytes |
| matrix.js | 143.67 KB | 143.66 KB | ⯆ -16 Bytes |
| odspClient.js | 519.75 KB | 519.73 KB | ⯆ -16 Bytes |
| odspDriver.js | 97.29 KB | 97.29 KB | ■ No change |
| odspPrefetchSnapshot.js | 42.15 KB | 42.15 KB | ■ No change |
| sharedString.js | 160.19 KB | 160.18 KB | ⯆ -16 Bytes |
| sharedTree.js | 359.58 KB | 359.56 KB | ⯆ -21 Bytes |
| Total Size | 3.2 MB | 3.2 MB | ⯆ -139 Bytes |
Baseline commit: 1018e0f1af5b99e288a7aba4b6c00fe3e0924110
Generated by :no_entry_sign: dangerJS against 5681b24246b431e3a501f521e4884e9b38386450
FYI--'reviewer guidance' section needs update.
Mark and I looked into these failures just now and verified that the same failure JSON (i.e. sequence of steps) doesn't reproduce the assert today. Since the original PR, there have been a few fixes to SharedString/merge-tree which are plausibly what fixed the issue. Notably, we fixed an issue where objects in SharedString ops would be reference-equal to property bags in the string. It would make sense removing the round-trip in that PR would cause bad behavior given the existence of this bug at the time.