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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 21 12:02:54 PDT 2019

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

Attachment 376894: Patch


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

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


>>> 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.

More information about the webkit-reviews mailing list