Bug 125422 - [WK2] Implement platform specific Resource Response for SOUP
Summary: [WK2] Implement platform specific Resource Response for SOUP
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Holt
URL:
Keywords:
: 121430 (view as bug list)
Depends on:
Blocks: 108832 126128
  Show dependency treegraph
 
Reported: 2013-12-08 11:16 PST by Brian Holt
Modified: 2014-12-03 07:58 PST (History)
9 users (show)

See Also:


Attachments
Patch (2.62 KB, patch)
2013-12-08 11:24 PST, Brian Holt
no flags Details | Formatted Diff | Diff
Patch (2.87 KB, patch)
2013-12-10 03:12 PST, Brian Holt
no flags Details | Formatted Diff | Diff
Patch for landing (2.91 KB, patch)
2013-12-22 01:44 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Holt 2013-12-08 11:16:38 PST
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.
Comment 1 Brian Holt 2013-12-08 11:24:30 PST
Created attachment 218700 [details]
Patch
Comment 2 Martin Robinson 2013-12-08 11:44:10 PST
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.
Comment 3 Csaba Osztrogonác 2013-12-09 03:36:04 PST
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.
Comment 4 Brian Holt 2013-12-09 04:42:43 PST
(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.
Comment 5 Sergio Villar Senin 2013-12-09 06:32:27 PST
Reverting the cq+. Haven't read Martin's comment #2
Comment 6 Brady Eidson 2013-12-09 09:50:13 PST
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.
Comment 7 Brian Holt 2013-12-10 03:04:46 PST
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.
Comment 8 Brian Holt 2013-12-10 03:12:41 PST
Created attachment 218848 [details]
Patch
Comment 9 Brian Holt 2013-12-10 04:40:42 PST
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.
Comment 10 Martin Robinson 2013-12-19 14:13:34 PST
(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.
Comment 11 Carlos Garcia Campos 2013-12-20 01:37:04 PST
(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.
Comment 12 Carlos Garcia Campos 2013-12-22 01:44:41 PST
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.
Comment 13 WebKit Commit Bot 2013-12-23 00:37:43 PST
Comment on attachment 219866 [details]
Patch for landing

Clearing flags on attachment: 219866

Committed r160988: <http://trac.webkit.org/changeset/160988>
Comment 14 WebKit Commit Bot 2013-12-23 00:37:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Csaba Osztrogonác 2014-12-03 07:58:15 PST
*** Bug 121430 has been marked as a duplicate of this bug. ***