Bug 185209 – Cleanup NetworkResourceLoader::didReceiveResponse()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Daniel Bates]

Cleanup NetworkResourceLoader::didReceiveResponse().

[Daniel Bates]

Created attachment 339319 [details]
Patch

[Daniel Bates]

Will add a remark to the ChangeLog before landing that I removed all logging except for a single log message to indicate when the network process is waiting to receive message NetworkResourceLoader::ContinueDidReceiveResponse. This is the only interesting message to log because it indicates that the network process cannot make forward progress for the load on its own.

[Daniel Bates]

Comment on attachment 339319 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:439
> +    // For main resources the embedding client must decide whether to allow the load. Therefore we wait to receive message
> +    // NetworkResourceLoader::ContinueDidReceiveResponse before continuing a load for a main resource.

Before landing I will change this comment to read:

We wait to receive message NetworkResourceLoader::ContinueDidReceiveResponse before continuing a load for a main resource because the embedding client must decide whether to allow the load.

[Daniel Bates]

Created attachment 339321 [details]
Patch

[Chris Dumez]

Comment on attachment 339321 [details]
Patch

r=me

[Chris Dumez]

Comment on attachment 339321 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-447
> -        RELEASE_LOG_IF_ALLOWED("didReceiveResponse: Should not wait for message from WebContent process before continuing resource load (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ")", m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier);

Note that we do lose some logging here. Not convinced it matters though.

[Chris Dumez]

Comment on attachment 339321 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-447
>> -        RELEASE_LOG_IF_ALLOWED("didReceiveResponse: Should not wait for message from WebContent process before continuing resource load (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ")", m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier);
> 
> Note that we do lose some logging here. Not convinced it matters though.

Just asked Keith (who added this logging) and he seems to think it is not a big deal to drop it.

[youenn fablet]

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:432
> +        return ShouldContinueDidReceiveResponse::No;

I am not very clear what this FIXME is about.
Since we know we want to cancel this load, we say to the NetworkLoad to not continue automatically until further notice.
We could consider other alternatives but I am not sure what the benefit is.
NetworkLoad should anyway handle the case where it is being cancelled while waiting to continue.

[Daniel Bates]

Comment on attachment 339321 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-447
>>> -        RELEASE_LOG_IF_ALLOWED("didReceiveResponse: Should not wait for message from WebContent process before continuing resource load (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ")", m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier);
>> 
>> Note that we do lose some logging here. Not convinced it matters though.
> 
> Just asked Keith (who added this logging) and he seems to think it is not a big deal to drop it.

Will remove all logging. Just to clarify, this logging you are referring here is in this patch. It was moved and its message was re-worded on line 446.

[Daniel Bates]

(In reply to youenn fablet from comment #8)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339321&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:432
> > +        return ShouldContinueDidReceiveResponse::No;
> 
> I am not very clear what this FIXME is about.
> Since we know we want to cancel this load, we say to the NetworkLoad to not
> continue automatically until further notice.
> We could consider other alternatives but I am not sure what the benefit is.
> NetworkLoad should anyway handle the case where it is being cancelled while
> waiting to continue.

FIXME is describing that ShouldContinueDidReceiveResponse::No should not be used as a general-purpose mechanism for failing a load or cancelling a load. ShouldContinueDidReceiveResponse::No should only be used as described by its name: the network process wanting to wait to receive a message from the web content process before continuing the load. Scheduling a load failure and taking advantage of ShouldContinueDidReceiveResponse::No to tell the client (e.g. NetworkLoad) to defer calling its completion handler with a policy response is an anti-pattern. We should more directly (read synchronously) fail or cancel the load.

[Daniel Bates]

Committed r231271: <https://trac.webkit.org/changeset/231271>

[Radar WebKit Bug Importer]

<rdar://problem/39921473>