Bug 175679 - Regression(r220817): We should only copy the original request headers for Ping loads
Summary: Regression(r220817): We should only copy the original request headers for Pin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 175628
  Show dependency treegraph
 
Reported: 2017-08-17 12:14 PDT by Chris Dumez
Modified: 2017-08-17 15:57 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.40 KB, patch)
2017-08-17 12:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.32 KB, patch)
2017-08-17 13:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (27.61 KB, patch)
2017-08-17 14:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-08-17 12:14:30 PDT
We should only copy the original request headers for Ping loads (Beacon at the moment), not for all CachedResourceRequests.
Comment 1 Chris Dumez 2017-08-17 12:16:04 PDT
Created attachment 318398 [details]
Patch
Comment 2 youenn fablet 2017-08-17 12:47:06 PDT
Comment on attachment 318398 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:836
> +        resource->setOriginalRequestHeaders(WTFMove(originalRequestHeaders));

We should probably also set the headers in the case of DeferOption::Defer.
If not, originalRequestHeaders could just be a parameter passed to CachedResource::load().
Comment 3 Chris Dumez 2017-08-17 13:00:00 PDT
Created attachment 318406 [details]
Patch
Comment 4 youenn fablet 2017-08-17 13:09:48 PDT
Comment on attachment 318406 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResource.cpp:276
> +            platformStrategies()->loaderStrategy()->createPingHandle(frame.loader().networkingContext(), request, HTTPHeaderMap(*m_originalRequestHeaders), *m_origin, contentSecurityPolicy, m_options);

IIRC, IPC is taking const& parameters.
WK2 createPingHandle could probably be implemented with just a const&.
Comment 5 youenn fablet 2017-08-17 13:10:06 PDT
(In reply to youenn fablet from comment #4)
> Comment on attachment 318406 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318406&action=review
> 
> > Source/WebCore/loader/cache/CachedResource.cpp:276
> > +            platformStrategies()->loaderStrategy()->createPingHandle(frame.loader().networkingContext(), request, HTTPHeaderMap(*m_originalRequestHeaders), *m_origin, contentSecurityPolicy, m_options);
> 
> IIRC, IPC is taking const& parameters.
> WK2 createPingHandle could probably be implemented with just a const&.

So that we remove this extra header copy.
Comment 6 Chris Dumez 2017-08-17 13:22:25 PDT
(In reply to youenn fablet from comment #5)
> (In reply to youenn fablet from comment #4)
> > Comment on attachment 318406 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=318406&action=review
> > 
> > > Source/WebCore/loader/cache/CachedResource.cpp:276
> > > +            platformStrategies()->loaderStrategy()->createPingHandle(frame.loader().networkingContext(), request, HTTPHeaderMap(*m_originalRequestHeaders), *m_origin, contentSecurityPolicy, m_options);
> > 
> > IIRC, IPC is taking const& parameters.
> > WK2 createPingHandle could probably be implemented with just a const&.
> 
> So that we remove this extra header copy.

WK2:
loadParameters.originalRequestHeaders = WTFMove(originalRequestHeaders);
Comment 7 youenn fablet 2017-08-17 13:38:31 PDT
> WK2:
> loadParameters.originalRequestHeaders = WTFMove(originalRequestHeaders);

Would the following work?
- Pass originalRequestHeaders as a direct parameter of NetworkConnectionToWebProcess::LoadPing. It is not used for other loads.
- Create the message with something like Messages::NetworkConnectionToWebProcess::LoadPing(loadParameters, originalRequestHeaders)

IIRC, Messages structures only needs const&.
Comment 8 Chris Dumez 2017-08-17 13:41:33 PDT
(In reply to youenn fablet from comment #7)
> > WK2:
> > loadParameters.originalRequestHeaders = WTFMove(originalRequestHeaders);
> 
> Would the following work?
> - Pass originalRequestHeaders as a direct parameter of
> NetworkConnectionToWebProcess::LoadPing. It is not used for other loads.
> - Create the message with something like
> Messages::NetworkConnectionToWebProcess::LoadPing(loadParameters,
> originalRequestHeaders)
> 
> IIRC, Messages structures only needs const&.

Yes, I have confirmed Messages structures only need const&. I can do what you suggest although I am unconvinced this is a worthwhile optimization.
Comment 9 youenn fablet 2017-08-17 13:51:21 PDT
> Yes, I have confirmed Messages structures only need const&. I can do what
> you suggest although I am unconvinced this is a worthwhile optimization.

Agreed, the main point of this patch is to limit the header copy to beacon.

As a follow-up of that patch, I think it makes sense to split the original headers from NetworkLoaderParameters since they are related to ping loads only.
It should be easy to add it back when implementing keepalive for fetch API loads, if we find these headers to be needed.
Comment 10 Chris Dumez 2017-08-17 13:53:22 PDT
(In reply to youenn fablet from comment #9)
> > Yes, I have confirmed Messages structures only need const&. I can do what
> > you suggest although I am unconvinced this is a worthwhile optimization.
> 
> Agreed, the main point of this patch is to limit the header copy to beacon.
> 
> As a follow-up of that patch, I think it makes sense to split the original
> headers from NetworkLoaderParameters since they are related to ping loads
> only.
> It should be easy to add it back when implementing keepalive for fetch API
> loads, if we find these headers to be needed.

It's trivial, I'll do another iteration.
Comment 11 Chris Dumez 2017-08-17 14:30:32 PDT
Created attachment 318423 [details]
Patch
Comment 12 Chris Dumez 2017-08-17 15:57:04 PDT
Comment on attachment 318423 [details]
Patch

Clearing flags on attachment: 318423

Committed r220888: <http://trac.webkit.org/changeset/220888>
Comment 13 Chris Dumez 2017-08-17 15:57:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2017-08-17 15:57:34 PDT
<rdar://problem/33952143>