Skip to content
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.

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

Storage: fix clear and removeItem #1136

Merged
merged 7 commits into from Feb 28, 2022
Merged

Storage: fix clear and removeItem #1136

merged 7 commits into from Feb 28, 2022

Conversation

samouri
Copy link
Member

@samouri samouri commented Feb 22, 2022

summary
Fixes: ampproject/amphtml#37739

The mutations array usually contains an index into StringContext for
value of key/value in the operation. Historically, "delete" was
modeled as 0th index into the array. Unfortunately, this didn't make
sense if the first op was genuinely referring to the 0th string
value (See #1035).

While #1035 fixed the first get/set operation, it also broke delete operations!
This PR now models the delete operation as a -1 in the unsigned mutation array, corresponding to 2^16-1.

The obvious flaw would be if we ever had that many mutations being transferred at once.
From what I can tell, thats more of a theoretical issue than a real one.

The mutations array usually contains an index into StringContext for
value of key/value in the operation. Historically, "delete" was
modeled as 0th index into the array. Unfortunately, this didn't make
sense if the first SET op was genuinely referring to the 0th string
value (See #1035).

While this fixed the first get/set operation, it also meant that deletes
were no longer possible! This PR now models the delete operation as a -1
in the unsigned mutation array, corresponding to 2^16-1.
@samouri samouri self-assigned this Feb 22, 2022
@@ -107,3 +107,7 @@ export const enum ResolveOrReject {
RESOLVE = 1,
REJECT = 2,
}

// Mutations are are modeled as Uint16Array.
// Deletion is specified as a -1, which when placed into the array becomes 2^16-1.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we setting them to -1 in the first place? Wouldn't 0 work?

Copy link
Member Author

@samouri samouri Feb 22, 2022

Choose a reason for hiding this comment

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

b/c then 0 would be overloaded and impossible to distinguish between "remove" or 0th index into Strings array. by picking the last index we make the issue much less likely.

An alternative could be to always have null as the first element of the strings array (reserved to mean deletions)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just define it as index as the + 1 of the strings index? Then 0 is impossible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Always sending over +1 from the worker seems messier on both ends than reserving the 0th index. WDYT

@samouri
Copy link
Member Author

samouri commented Feb 23, 2022

update / background
The main-thread code holds on to an array of strings, called StringContext. This way the worker-thread only has to send any string over the postMessage interface once. It can then refer to that string via its index into the array.

Sometimes it is important to model a "null" as the string value.
For example, when the worker-thread sends a message to main-thread to remove a key from localStorage, it does do via: {key: number, value: number}. Each of these numbers correspond to an index into the string array. To signify removeItem, the value has to indicate "no value". Its a bit of a sloppy modeling, but required due to the fixed-size of message types in the mutation array. Therefore in order to avoid a large refactor, we'll need some number to be reserved as meaning "no value".

This PR reserves the 0th slot in main-thread StringContext, and starts the count of the worker-thread from 1. This was somewhat suggested by #1136 (comment) as a better alternative than reserving the last slot.

@samouri samouri force-pushed the fix-delete branch 2 times, most recently from 2e73514 to ad8ca58 Compare February 24, 2022 12:53
@samouri
Copy link
Member Author

samouri commented Feb 24, 2022

After a discussion with @jridgewell, we agreed that while reserving 0th slot may be simpler, it could have unexpected ramifications to the codebase vs isolating to a single processor. In a58f111 I made the index +1 in worker, -1 in main-thread as suggested earlier.

Can do the 0 reservation in a future PR

@samouri samouri merged commit a6bcd6f into main Feb 28, 2022
@samouri samouri deleted the fix-delete branch February 28, 2022 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

localStorage.removeItem creates key with arbitrary value from state
2 participants