1747102 - Handle ill-formed keys in local storage

Status: REOPENED

(Core :: Storage: localStorage & sessionStorage, defect, P3)

Description

[Jari Jalkanen]

Local storage keys which are ill-formed as Unicode strings, such as String.fromCharCode(0xdbff) , are currently prone to corruption which in turn may lead to spurious collisions or data recovery errors.

To prevent this, error handling or the domain of supported byte sequences should be extended to cover these cases.

Comment 1

[Jari Jalkanen]

Attachment: Bug 1747102 - Handle ill-formed local storage keys. r=#dom-storage-reviewers

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60f55a6084f7
Handle ill-formed local storage keys. r=dom-storage-reviewers,asuth

Comment 3

[Atila Butkovits]

Backed out for causing failures at test_brokenUTF-16.html.

Backout link: https://hg.mozilla.org/integration/autoland/rev/08536310d5f48d71f001eecdbf4676869d257997

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=60f55a6084f799514481b71be1914445a95f2899&selectedTaskRun=c3iFcBqfQ926VSSPRL8K6w.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=367214509&repo=autoland&lineNumber=6859
https://treeherder.mozilla.org/logviewer?job_id=367215109&repo=autoland&lineNumber=12765

Comment 4

[Jari Jalkanen]

The failure was due to a mochitest checking what happens when invalid unicode is used as a key - and this behavior was specifically changed by this bug.

The failing test case slipped through unnoticed among unrelated intermittent failures sometimes occurring at CI.

The patch now updates the test case to work with the new behavior.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ed5964d2cec
Handle ill-formed local storage keys. r=dom-storage-reviewers,asuth

Comment 6

[Atila Butkovits]

Backed out for causing failures at storage_setitem.window.html.

Backout link: https://hg.mozilla.org/integration/autoland/rev/06c29d2388a78a84b0b4a4b65785fb2b4d89f426

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=AaItD_G9TDiWEvZSeOCw7w.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=4ed5964d2cec552a9bf7296e874dfe00118ce373

Failure log: https://treeherder.mozilla.org/logviewer?job_id=367817021&repo=autoland&lineNumber=78124

Comment 7

[Jan Varga [:janv]]

It seems, we shouldn't do this after all.

Comment 8

[Jari Jalkanen]

(In reply to Jan Varga [:janv] from comment #7)

It seems, we shouldn't do this after all.

Agreed, at the end, it might be better to fix the underlying issue. The circumstances where it manifests are probably less common than the cases where the proposed fix would throw an error.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

For posterity, the WPT that did exist is https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webstorage/storage_setitem.window.js

Comment 10

[Jan Varga [:janv]]

Ok, can we mark this as WONTFIX ?

Comment 11

[Release mgmt bot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jjalkanen, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 12

[Jari Jalkanen]

(In reply to Release mgmt bot [:marco/ :calixte] from comment #11)

If you still have some work to do, you can add an action "Plan Changes" in Phabricator.

Changes planned.