Bug 180118 – NetworkCache::Storage should protect itself when removing operations from its maps

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

The operations can contain ref to the Storage object and removing them from the map may destroy the Storage object.

[youenn fablet]

Created attachment 327802 [details]
Patch

[Antti Koivisto]

Comment on attachment 327802 [details]
Patch

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

Storage::remove(const Key& key) probably needs one too before removeFromPendingWriteOperations() call.

> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:672
> +        auto protectedThis = makeRef(*this);
> +
>          ASSERT(m_activeReadOperations.contains(&readOperation));
>          m_activeReadOperations.remove(&readOperation);

Alternatively these could do

auto toDelete = m_activeReadOperations.take(&readOperation);

> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:925
>          RunLoop::main().dispatch([this, &traverseOperation] {
>              traverseOperation.handler(nullptr, { });
> +
> +            auto protectedThis = makeRef(*this);
> +
>              m_activeTraverseOperations.remove(&traverseOperation);

This is not strictly needed here but I guess it is good documentation.

[youenn fablet]

Thanks for the review!

(In reply to Antti Koivisto from comment #2)
> Comment on attachment 327802 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327802&action=review
> 
> Storage::remove(const Key& key) probably needs one too before
> removeFromPendingWriteOperations() call.

Will add one.

> > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:672
> > +        auto protectedThis = makeRef(*this);
> > +
> >          ASSERT(m_activeReadOperations.contains(&readOperation));
> >          m_activeReadOperations.remove(&readOperation);
> 
> Alternatively these could do
> 
> auto toDelete = m_activeReadOperations.take(&readOperation);

Would be nice but we would need to add UNUSED_PARAM.
protectedThis is also widely used and somehow clearer about the meaning.

[youenn fablet]

Created attachment 327810 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 327810 [details]:

svg/animations/animateTransform-pattern-transform.html bug 180121 (authors: [e-mail], [e-mail], [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 327810 [details]
Patch for landing

Clearing flags on attachment: 327810

Committed r225253: <https://trac.webkit.org/changeset/225253>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35738687>

[Antti Koivisto]

> > auto toDelete = m_activeReadOperations.take(&readOperation);
> 
> Would be nice but we would need to add UNUSED_PARAM.
> protectedThis is also widely used and somehow clearer about the meaning.

No, it doesn't need UNUSED_PARAM. The compiler is smart enough to know about the implicit destructor call. 

protectedThis is fine too though.