[webkit-reviews] review granted: [Bug 200986] Crash under StringImpl::~StringImpl() in NetworkProcess::deleteWebsiteDataForRegistrableDomains() : [Attachment 376894] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 200986: Crash under StringImpl::~StringImpl() in
NetworkProcess::deleteWebsiteDataForRegistrableDomains()
https://bugs.webkit.org/show_bug.cgi?id=200986

Attachment 376894: Patch

https://bugs.webkit.org/attachment.cgi?id=376894&action=review




--- Comment #5 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 376894
  --> https://bugs.webkit.org/attachment.cgi?id=376894
Patch

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

r=me

>>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1734
>>> +		 RunLoop::main().dispatch([this, sessionID,
domainsToDeleteAllButCookiesFor = WTFMove(domainsToDeleteAllButCookiesFor),
callbackAggregator = callbackAggregator.copyRef(), securityOrigins =
indexedDatabaseOrigins(path)] {
>> 
>> So it looks like we had this thread-transfer backwards in the original code.
I wonder how we can make it harder to avoid this problem.
>> 
>> I understand why we need to crossThreadCopy path (created on the main
thread) to the cross-thread task (line 1733), but why is it okay to WTFMove
that cross-thread copied memory back to the main thread on line 1734?
> 
> Technically, we could crossThreadCopy() both, however, I know WTFMove() is
safe here because I have just isolatedCopied the data structure already and I
did not pass it to anybody else.
> It is safe to WTFMove() a String to another thread if:
> 1. RefCount is 1 (i.e. String is not shared with anybody else)
> and
> 2. String is not atomized
> 
> See String::isSafeToSendToAnotherThread().

Sounds good.