[webkit-reviews] review granted: [Bug 194027] Stop using blobRegistry in NetworkProcess : [Attachment 360955] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 194027: Stop using blobRegistry in NetworkProcess
https://bugs.webkit.org/show_bug.cgi?id=194027

Attachment 360955: Patch

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




--- Comment #21 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 360955
  --> https://bugs.webkit.org/attachment.cgi?id=360955
Patch

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

> Source/WebCore/platform/network/BlobRegistryImpl.h:59
>      void appendStorageItems(BlobData*, const BlobDataItemList&, long long
offset, long long length);

Do we need all the methods below to be public?

> Source/WebCore/platform/network/FormData.cpp:321
> +    BlobData* blobData =
static_cast<BlobRegistryImpl&>(blobRegistry).getBlobDataFromURL(url);

auto*?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:326
> +	   file->prepareForFileAccess();

for loop in the if block?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:340
> +    auto loader = NetworkResourceLoader::create(WTFMove(loadParameters),
*this, WTFMove(blobFiles));

I would move the blob handling in NetworkResourceLoader and PingLoad.
They already have a ref to the connection.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:418
> +    ASSERT_UNUSED(blobFiles, blobFiles.isEmpty());

Maybe just ASSERT(!parameters.request.httpBody())

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:571
> +Vector<RefPtr<WebCore::BlobDataFileReference>>
NetworkConnectionToWebProcess::filesInBlob(const URL& url)

Could we make them a Vector<Ref> here or in a follow-up?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:192
> +    ASSERT_UNUSED(count, count == 1);

I am not sure we should use removeAllMatching here since it requires iterating
on the whole array, for no good reason since the current code seems pretty good
at making sure this double entry case will never happen.
We could use removeFirstMatching() and if we want to ASSERT that we cannot find
connection in m_webProcessConnections.
Or we could make m_webProcessConnections a HashSet, not sure the order is
actually important.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-917
> -	   fileReference->prepareForFileAccess();

It seems a bit odd to no longer prepareForFileAccess in
consumeSandboxExtensions but do revokeFileAccess in
invalidateSandboxExtensions.
And revokeFileAccess is done conditionally on m_didConsumeSandboxExtensions.
Maybe we should at least do revokeFileAccess if m_fileReferences is not empty.

I think that keeping prepareForFileAccess/revokeFileAccess inside
NetworkResourceLoader would also help ensuring to keep this logic in sync.

> Source/WebKit/NetworkProcess/PingLoad.cpp:85
> +	       file->revokeFileAccess();

Can we have cases where file is nullptr?
Should we pass a Vector<Ref<>>.
It seems that NetworkResourceLoader is not doing this check.