Bug 180118

Summary: NetworkCache::Storage should protect itself when removing operations from its maps
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, commit-queue, ews-watchlist, koivisto, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Description youenn fablet 2017-11-28 16:19:18 PST
The operations can contain ref to the Storage object and removing them from the map may destroy the Storage object.
Comment 1 youenn fablet 2017-11-28 16:22:06 PST
Created attachment 327802 [details]
Patch
Comment 2 Antti Koivisto 2017-11-28 16:48:14 PST
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.
Comment 3 youenn fablet 2017-11-28 17:45:46 PST
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.
Comment 4 youenn fablet 2017-11-28 17:50:21 PST
Created attachment 327810 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2017-11-28 18:23:11 PST
The commit-queue encountered the following flaky tests while processing attachment 327810 [details]:

svg/animations/animateTransform-pattern-transform.html bug 180121 (authors: ap@webkit.org, arv@chromium.org, krit@webkit.org, mark.lam@apple.com, and zimmermann@kde.org)
The commit-queue is continuing to process your patch.
Comment 6 WebKit Commit Bot 2017-11-28 18:23:41 PST
Comment on attachment 327810 [details]
Patch for landing

Clearing flags on attachment: 327810

Committed r225253: <https://trac.webkit.org/changeset/225253>
Comment 7 WebKit Commit Bot 2017-11-28 18:23:43 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2017-11-28 18:24:22 PST
<rdar://problem/35738687>
Comment 9 Antti Koivisto 2017-11-29 03:01:31 PST
> > 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.