FluidFramework icon indicating copy to clipboard operation
FluidFramework copied to clipboard

Make handle parsing/decoding functions idempotent

Open markfields opened this issue 1 year ago • 1 comments

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.

markfields avatar May 15 '24 23:05 markfields

@fluid-example/bundle-size-tests: -139 Bytes
Metric NameBaseline SizeCompare SizeSize 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

msfluid-bot avatar May 16 '24 00:05 msfluid-bot

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.

Abe27342 avatar May 20 '24 18:05 Abe27342