Bug 125422 – [WK2] Implement platform specific Resource Response for SOUP

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brian Holt]

When building the Network Process for non-Mac ports there are a few places where mac specific code is used without a guard causing compile failures.

[Brian Holt]

Created attachment 218700 [details]
Patch

[Martin Robinson]

Comment on attachment 218700 [details]
Patch

This looks good to me, but I'll let a Mac reviewer chip in before we land this. It's possible that we will need GTK+ specific implementations of these sections eventually, but we are working on getting the NetworkProcess building so that we can proceed with that.

[Csaba Osztrogonác]

Comment on attachment 218700 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:249
> +#if PLATFORM(MAC)
>      m_suggestedRequestForWillSendRequest.updateFromDelegatePreservingOldHTTPBody(newRequest.nsURLRequest(DoNotUpdateHTTPBody));
> +#endif

There is a bug report for it: https://bugs.webkit.org/show_bug.cgi?id=121430
But Darin mentioned we should explain why Mac needs is and why we don't.

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:111
> +#if PLATFORM(MAC)
>      responseCopy.setCertificateChain(certificateInfo.certificateChain());
> +#endif

We won't need this once https://bugs.webkit.org/show_bug.cgi?id=124724 is fixed.

[Brian Holt]

(In reply to comment #3)
> (From update of attachment 218700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218700&action=review

> There is a bug report for it: https://bugs.webkit.org/show_bug.cgi?id=121430
> But Darin mentioned we should explain why Mac needs is and why we don't.
> 

I found that bug after I proposed this patch and I've commented on it.  We should definitely find out why we don't need it, but this is blocking us using the netwok process while we try to find out.

> > Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:111
> > +#if PLATFORM(MAC)
> >      responseCopy.setCertificateChain(certificateInfo.certificateChain());
> > +#endif
> 
> We won't need this once https://bugs.webkit.org/show_bug.cgi?id=124724 is fixed.

Yes, it will become obsolete when that patch lands, but we need it now so that the network process compiles.

[Sergio Villar Senin]

Reverting the cq+. Haven't read Martin's comment #2

[Brady Eidson]

Comment on attachment 218700 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:249
>> +#endif
> 
> There is a bug report for it: https://bugs.webkit.org/show_bug.cgi?id=121430
> But Darin mentioned we should explain why Mac needs is and why we don't.

Please explain why here.

>>> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:111
>>> +#endif
>> 
>> We won't need this once https://bugs.webkit.org/show_bug.cgi?id=124724 is fixed.
> 
> Yes, it will become obsolete when that patch lands, but we need it now so that the network process compiles.

Please include a FIXME here referencing the bug that makes it obsolete.

[Brian Holt]

Comment on attachment 218700 [details]
Patch

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

>>> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:249
>>> +#endif
>> 
>> There is a bug report for it: https://bugs.webkit.org/show_bug.cgi?id=121430
>> But Darin mentioned we should explain why Mac needs is and why we don't.
> 
> Please explain why here.

As Martin expected, we do need a SOUP specific implementation here.  The candidate line of code will be 

m_suggestedRequestForWillSendRequest.updateFromDelegatePreservingOldHTTPBody(newRequest);

which differs from the Mac implementation because the Mac version of updateFromDelegatePreservingOldHTTPBody() accepts a NSURLRequest* which is a CF class and doesn't exist in the SOUP world.  The policy DoNotUpdateHTTPBody is therefore specific to the NSURLRequest, and is implied for SOUP in the name of the function called: updateFromDelegatePreservingOldHTTPBody().

I will submit a new patch shortly.

>>>> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:111
>>>> +#endif
>>> 
>>> We won't need this once https://bugs.webkit.org/show_bug.cgi?id=124724 is fixed.
>> 
>> Yes, it will become obsolete when that patch lands, but we need it now so that the network process compiles.
> 
> Please include a FIXME here referencing the bug that makes it obsolete.

I will submit the SOUP specific implementation shortly.

[Brian Holt]

Created attachment 218848 [details]
Patch

[Brian Holt]

Alexey made a good point in https://bugs.webkit.org/show_bug.cgi?id=121430#c7
that no ports should send the httpBody over IPC.  So all we need to do is to  demonstrate that for soup we don't update the http body.

The only place that the http body is set for soup is in ResourceRequest::updateFromSoupMessage().  updateFromDelegatePreservingOldHTTPBody() simply sets *this for the RequestRequest to the delegatedResourceRequest which doesn't call updateFromSoupMessage().  So if the http body was empty it stays empty and will remain out of sync.

Martin will test the code to confirm that this is indeed the case.

[Martin Robinson]

(In reply to comment #9)

> Martin will test the code to confirm that this is indeed the case.

I have not been able to confirm this directly, since the NetworkProcess isn't working correctly me for me at the moment. From looking at the code though, it seems that the Soup version of ResourceHandle does not use the typical updatePlatformRequest and updateResourceRequest path. This is a design problem for sure, but it also means it's unlikely that we're copying the body into the other request.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > Martin will test the code to confirm that this is indeed the case.
> 
> I have not been able to confirm this directly, since the NetworkProcess isn't working correctly me for me at the moment. From looking at the code though, it seems that the Soup version of ResourceHandle does not use the typical updatePlatformRequest and updateResourceRequest path. This is a design problem for sure, but it also means it's unlikely that we're copying the body into the other request.

I've checked we are never sending the http body over IPC, at least in the tests I've done. I think we can land this patch in any case, to allow at least build with the network process, to make it easier for everybody to continue working on this and all other network process related issues.

[Carlos Garcia Campos]

Created attachment 219866 [details]
Patch for landing

Updated patch to include FIXMES and bug urls. This is blocking our work in the network process, because it doesn't build without it.

[WebKit Commit Bot]

Comment on attachment 219866 [details]
Patch for landing

Clearing flags on attachment: 219866

Committed r160988: <http://trac.webkit.org/changeset/160988>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

*** Bug 121430 has been marked as a duplicate of this bug. ***