2049425 - test_orderingAfterRemoveAdd.js tests hashmap iteration order.

Status: NEW

(Core :: Storage: localStorage & sessionStorage, task)

Description

[Emilio Cobos Álvarez [:emilio]]

https://searchfox.org/firefox-main/rev/c280a3de79038960073359b0b4777c676320b76a/dom/localstorage/test/unit/xpcshell.toml#103-106 is right except for the fact that it's bening. There's nothing guaranteeing that the hash keys don't get shuffled even within a single process, and bug 2048926 triggered that.

Comment 1

[Jan Varga [:janv]]

It seems that comment was added in bug 2020820.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Afaict, the test is testing that the order matches after removals, but that's not guaranteed given the current implementation. So mValues should switch to something insertion-order-preserving or so, like the EcmaMap object being proposed in bug 2030981, or so.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

I propose we disable the test for now (https://phabricator.services.mozilla.com/D308077) since the issue is pre-existing (pretty sure I can come up with a different set of keys that trigger the issue) and this blocks bug 2048926 and following enhancements.

Comment 4

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

I did a restating block at https://phabricator.services.mozilla.com/D308077#10690214. The tl;dr is that by spec we are allowed to reorder whenever we are adding or removing items and as long as we are not maintaining an ordering in the content process and instead relying on the hashtable ordering, the stability of ordering check is not a safe one to perform. There's a meta-question of whether we should be trying to maintain the ordering info that the parent process provides us with in the content processes since we actually do provide all of that and an argument could be made that we are non-spec-compliant as it relates to ordering if we don't retain our snapshots. But the spec also does not actually do the legwork to require a stable ordering, it's mainly just making it more clear what the impact of using hashtables is, and also as noted in the restating, the spec entirely handwaves around multiprocess things, so we're also probably fine there. Note that we definitely would not want to lose the hashtable lookup performance if we did start maintaining ordering info.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mccr8, could you have a look please?

For more information, please visit BugBot documentation.