Bug 239982 – StorageMap::removeItem may fail to remove item from map

• Status: RESOLVED
• Resolution: FIXED
• Component: Website Storage

[Sihui Liu]

...

[Sihui Liu]

rdar://80891555

[Sihui Liu]

Created attachment 458707 [details]
Patch

[Chris Dumez]

Comment on attachment 458707 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458707&action=review

Nice

> LayoutTests/storage/domstorage/sessionstorage/resources/window-open-remove-item.html:3
> +<script src="../../../../resources/js-test.js"></script>

Is this really needed?

[Sihui Liu]

Created attachment 458749 [details]
Patch for landing

[EWS]

Committed r293736 (250224@main): <https://commits.webkit.org/250224@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458749 [details].

[Darin Adler]

Comment on attachment 458749 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=458749&action=review

> Source/WebCore/storage/StorageMap.cpp:141
> +    m_impl->map.remove(key);

Could we have fixed this by using take rather than find/remove? Would be nice to avoid hashing twice.

[Sihui Liu]

Comment on attachment 458749 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=458749&action=review

>> Source/WebCore/storage/StorageMap.cpp:141
>> +    m_impl->map.remove(key);
> 
> Could we have fixed this by using take rather than find/remove? Would be nice to avoid hashing twice.

I don't quite understand the proposal. Are you suggesting replace remove() with take()? It seems HashMap<T, U, V, W, MappedTraits, Y>::take(const KeyType& key) is find and remove.

(What StorageMap::removeItem does now is to makes a copy of map if the map is shared and key exists, and removes key from the copy; the original map is not changed.)

[Chris Dumez]

(In reply to Sihui Liu from comment #7)
> Comment on attachment 458749 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458749&action=review
> 
> >> Source/WebCore/storage/StorageMap.cpp:141
> >> +    m_impl->map.remove(key);
> > 
> > Could we have fixed this by using take rather than find/remove? Would be nice to avoid hashing twice.
> 
> I don't quite understand the proposal. Are you suggesting replace remove()
> with take()? It seems HashMap<T, U, V, W, MappedTraits, Y>::take(const
> KeyType& key) is find and remove.
> 
> (What StorageMap::removeItem does now is to makes a copy of map if the map
> is shared and key exists, and removes key from the copy; the original map is
> not changed.)

As Sihui points out, this proposal wouldn't work as we are working with 2 different HashMaps. We did a look up in the first HashMap, then we're doing the remove in a second HashMap (that happens to be a copy of the first HashMap).

[Darin Adler]

Comment on attachment 458749 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=458749&action=review

>>>> Source/WebCore/storage/StorageMap.cpp:141
>>>> +    m_impl->map.remove(key);
>>> 
>>> Could we have fixed this by using take rather than find/remove? Would be nice to avoid hashing twice.
>> 
>> I don't quite understand the proposal. Are you suggesting replace remove() with take()? It seems HashMap<T, U, V, W, MappedTraits, Y>::take(const KeyType& key) is find and remove.
>> 
>> (What StorageMap::removeItem does now is to makes a copy of map if the map is shared and key exists, and removes key from the copy; the original map is not changed.)
> 
> As Sihui points out, this proposal wouldn't work as we are working with 2 different HashMaps. We did a look up in the first HashMap, then we're doing the remove in a second HashMap (that happens to be a copy of the first HashMap).

I see; the take could only be done if the reference count was 1. I would have written:

    if (m_impl->hasOneRef())
        m_impl->map.remove(iter);
    else {
        m_impl = m_impl->copy();
        m_impl->map.remove(key);
    }

[Chris Dumez]

(In reply to Darin Adler from comment #9)
> Comment on attachment 458749 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458749&action=review
> 
> >>>> Source/WebCore/storage/StorageMap.cpp:141
> >>>> +    m_impl->map.remove(key);
> >>> 
> >>> Could we have fixed this by using take rather than find/remove? Would be nice to avoid hashing twice.
> >> 
> >> I don't quite understand the proposal. Are you suggesting replace remove() with take()? It seems HashMap<T, U, V, W, MappedTraits, Y>::take(const KeyType& key) is find and remove.
> >> 
> >> (What StorageMap::removeItem does now is to makes a copy of map if the map is shared and key exists, and removes key from the copy; the original map is not changed.)
> > 
> > As Sihui points out, this proposal wouldn't work as we are working with 2 different HashMaps. We did a look up in the first HashMap, then we're doing the remove in a second HashMap (that happens to be a copy of the first HashMap).
> 
> I see; the take could only be done if the reference count was 1. I would
> have written:
> 
>     if (m_impl->hasOneRef())
>         m_impl->map.remove(iter);
>     else {
>         m_impl = m_impl->copy();
>         m_impl->map.remove(key);
>     }

Right, this would work and be a bit more efficient in the probably common case where the StorageMap isn't shared. I like it.

[Sihui Liu]

Committed r294124 (250502@trunk): <https://commits.webkit.org/250502@trunk>