Bug 239982 - StorageMap::removeItem may fail to remove item from map
Summary: StorageMap::removeItem may fail to remove item from map
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Website Storage (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-05-02 14:18 PDT by Sihui Liu
Modified: 2022-05-12 14:57 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.98 KB, patch)
2022-05-02 14:28 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (4.90 KB, patch)
2022-05-03 10:57 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2022-05-02 14:18:29 PDT
...
Comment 1 Sihui Liu 2022-05-02 14:18:43 PDT
rdar://80891555
Comment 2 Sihui Liu 2022-05-02 14:28:55 PDT
Created attachment 458707 [details]
Patch
Comment 3 Chris Dumez 2022-05-02 14:32:17 PDT
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?
Comment 4 Sihui Liu 2022-05-03 10:57:20 PDT
Created attachment 458749 [details]
Patch for landing
Comment 5 EWS 2022-05-03 13:19:12 PDT
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].
Comment 6 Darin Adler 2022-05-03 14:44:39 PDT
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.
Comment 7 Sihui Liu 2022-05-03 15:16:28 PDT
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.)
Comment 8 Chris Dumez 2022-05-03 15:30:02 PDT
(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).
Comment 9 Darin Adler 2022-05-03 16:52:54 PDT
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);
    }
Comment 10 Chris Dumez 2022-05-03 16:55:08 PDT
(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.
Comment 11 Sihui Liu 2022-05-12 14:57:22 PDT
Committed r294124 (250502@trunk): <https://commits.webkit.org/250502@trunk>