Bug 239255 – StorageMap::importItems may update currentSize wrongly in release build

• Component: Website Storage
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 457482 [details]
Patch

[Sihui Liu]

Created attachment 457486 [details]
Patch

[Chris Dumez]

Comment on attachment 457486 [details]
Patch

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

> Source/WebCore/storage/StorageMap.cpp:166
> +        RELEASE_LOG_ERROR(Storage, "StorageMap::importItems failed to import because map is not empty");

Why is it OK to ignore the items in this case? If that's not supposed to happen, should we ASSERT?

[Sihui Liu]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 457486 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457486&action=review
> 
> > Source/WebCore/storage/StorageMap.cpp:166
> > +        RELEASE_LOG_ERROR(Storage, "StorageMap::importItems failed to import because map is not empty");
> 
> Why is it OK to ignore the items in this case? If that's not supposed to
> happen, should we ASSERT?

Yes, we can turn it into assertion

[Sihui Liu]

Created attachment 457539 [details]
Patch

[Chris Dumez]

Comment on attachment 457539 [details]
Patch

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

> Source/WebCore/storage/StorageMap.cpp:166
> +    CheckedUint32 newSize = m_impl->currentSize;

why `= m_impl->currentSize`? I thought the map was empty?

Can we assert that m_impl->currentSize is 0 and then just use `CheckedUint32 newSize;` here?

[Sihui Liu]

Created attachment 457542 [details]
Patch

[Chris Dumez]

Comment on attachment 457542 [details]
Patch

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

r=me assuming the bots are happy.

> Source/WebCore/storage/StorageMap.cpp:164
> +    RELEASE_ASSERT(!m_impl->currentSize);

Why did you drop the `RELEASE_ASSERT(m_impl->map.isEmpty());` ?

I feel it'd be good to check that both there is no item in the map and the size is indeed 0.

[Sihui Liu]

Comment on attachment 457542 [details]
Patch

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

>> Source/WebCore/storage/StorageMap.cpp:164
>> +    RELEASE_ASSERT(!m_impl->currentSize);
> 
> Why did you drop the `RELEASE_ASSERT(m_impl->map.isEmpty());` ?
> 
> I feel it'd be good to check that both there is no item in the map and the size is indeed 0.

I thought you mean replacing RELEASE_ASSERT with this... will update.

[Sihui Liu]

Created attachment 457543 [details]
Patch

[EWS]

Committed r292836 (249609@main): <https://commits.webkit.org/249609@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/91719712>