Bug 240200 - [SOUP2] Compute number of header bytes whe using soup 2
Summary: [SOUP2] Compute number of header bytes whe using soup 2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-05-06 19:08 PDT by Yury Semikhatsky
Modified: 2022-05-16 15:27 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.99 KB, patch)
2022-05-06 19:12 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (3.07 KB, patch)
2022-05-09 12:43 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2022-05-06 19:08:33 PDT
[SOUP2] Compute number of header bytes whe using soup 2
Comment 1 Yury Semikhatsky 2022-05-06 19:12:35 PDT
Created attachment 458986 [details]
Patch
Comment 2 Carlos Garcia Campos 2022-05-07 01:05:39 PDT
Why not using soup3 instead? I'm not sure we want to add new features to soup2 code path at this point.
Comment 3 Darin Adler 2022-05-08 14:23:34 PDT
Comment on attachment 458986 [details]
Patch

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

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1277
> +            int requestHeadersSize = 0;

Seems like this should be uint64_t, not int. Unlikely we will overrun 32 bits, but why mix types unnecessarily? Since requestHeaderBytesSent is uint64_t, and strlen returns size_t there is no reason to involve int.

Of course we also have to make addHeaderSizes use the same type.
Comment 4 Michael Catanzaro 2022-05-08 16:39:40 PDT
Comment on attachment 458986 [details]
Patch

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

Doesn't seem like a very large amount of code to maintain compared to the rest of the soup2 code, though. I'm sure they'll switch to soup3 after upgrading to Ubuntu 22.04. In the meantime, even GNOME hasn't managed to switch WebKit over yet.

> Source/WebKit/ChangeLog:3
> +        [SOUP2] Compute number of header bytes whe using soup 2

when

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1224
> +    int* size = static_cast<int*>(pointer);

I would use unsigned here instead of int. Also, you can use GPOINTER_TO_UINT, which is a little self-documenting.

>> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1277
>> +            int requestHeadersSize = 0;
> 
> Seems like this should be uint64_t, not int. Unlikely we will overrun 32 bits, but why mix types unnecessarily? Since requestHeaderBytesSent is uint64_t, and strlen returns size_t there is no reason to involve int.
> 
> Of course we also have to make addHeaderSizes use the same type.

Hm, very good catch, Darin! But since gpointer may be 32 bytes, you really cannot use uint64_t here without more changes. unsigned int would work, though.

I think you could use uint64_t if you switch from soup_message_headers_foreach() to SoupMessageHeadersIter. The code won't be as nice, but seems worth it to me.
Comment 5 Michael Catanzaro 2022-05-08 16:43:57 PDT
(In reply to Michael Catanzaro from comment #4)
> since gpointer may be 32 bytes

Um, of course I mean 4 bytes, 32 bits.
Comment 6 Adrian Perez 2022-05-09 00:05:29 PDT
Comment on attachment 458986 [details]
Patch

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

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1225
> +    *size += strlen(name) + strlen(value) + 4;

Where does the “4” come from? Knowing how HTTP works and how Soup formats
requests, I personally know that it's one byte for the colon, one byte for
the space after it, and two more bytes for the CRLF sequence at the end of
the line; but having a comment here explaining this would help other people
reading the code ;-)
Comment 7 Darin Adler 2022-05-09 07:32:37 PDT
Comment on attachment 458986 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1277
>>> +            int requestHeadersSize = 0;
>> 
>> Seems like this should be uint64_t, not int. Unlikely we will overrun 32 bits, but why mix types unnecessarily? Since requestHeaderBytesSent is uint64_t, and strlen returns size_t there is no reason to involve int.
>> 
>> Of course we also have to make addHeaderSizes use the same type.
> 
> Hm, very good catch, Darin! But since gpointer may be 32 bytes, you really cannot use uint64_t here without more changes. unsigned int would work, though.
> 
> I think you could use uint64_t if you switch from soup_message_headers_foreach() to SoupMessageHeadersIter. The code won't be as nice, but seems worth it to me.

That’s not quite right. The gpointer here is a *pointer* to the int, the one I suggested changing to uint64_t, not the int *converted* to a pointer, so the size of gpointer is not a concern.
Comment 8 Michael Catanzaro 2022-05-09 07:47:30 PDT
Comment on attachment 458986 [details]
Patch

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

>>>> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1277
>>>> +            int requestHeadersSize = 0;
>>> 
>>> Seems like this should be uint64_t, not int. Unlikely we will overrun 32 bits, but why mix types unnecessarily? Since requestHeaderBytesSent is uint64_t, and strlen returns size_t there is no reason to involve int.
>>> 
>>> Of course we also have to make addHeaderSizes use the same type.
>> 
>> Hm, very good catch, Darin! But since gpointer may be 32 bytes, you really cannot use uint64_t here without more changes. unsigned int would work, though.
>> 
>> I think you could use uint64_t if you switch from soup_message_headers_foreach() to SoupMessageHeadersIter. The code won't be as nice, but seems worth it to me.
> 
> That’s not quite right. The gpointer here is a *pointer* to the int, the one I suggested changing to uint64_t, not the int *converted* to a pointer, so the size of gpointer is not a concern.

Um... yes. Wow, everything that I wrote was wrong. So better ignore my entire previous comment, and just switch to uint64_t like Darin suggests.
Comment 9 Yury Semikhatsky 2022-05-09 12:42:36 PDT
(In reply to Carlos Garcia Campos from comment #2)
> Why not using soup3 instead? I'm not sure we want to add new features to
> soup2 code path at this point.

Ubuntu 20.04 LTS comes with libsoup2 only and this is where most of our linux users are and as far as I understand (https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy) it's going to be supported by WebKit for one more year. We might be able to switch to soup3 if the new flatpak based packaging works (https://bugs.webkit.org/show_bug.cgi?id=237107) and we bundle libsoup along with the browser binaries but for now we depend on the host libraries.

(In reply to Adrian Perez from comment #6)
> Comment on attachment 458986 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458986&action=review
> 
> > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1225
> > +    *size += strlen(name) + strlen(value) + 4;
> 
> Where does the “4” come from? Knowing how HTTP works and how Soup formats
> requests, I personally know that it's one byte for the colon, one byte for
> the space after it, and two more bytes for the CRLF sequence at the end of
> the line; but having a comment here explaining this would help other people
> reading the code ;-)

Added a comment explaining the format.


(In reply to Darin Adler from comment #3)
> Comment on attachment 458986 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458986&action=review
> 
> > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1277
> > +            int requestHeadersSize = 0;
> 
> Seems like this should be uint64_t, not int. Unlikely we will overrun 32
> bits, but why mix types unnecessarily? Since requestHeaderBytesSent is
> uint64_t, and strlen returns size_t there is no reason to involve int.
> 
> Of course we also have to make addHeaderSizes use the same type.

Changed the code to use uint64_t
Comment 10 Yury Semikhatsky 2022-05-09 12:43:08 PDT
Created attachment 459069 [details]
Patch
Comment 11 Yury Semikhatsky 2022-05-16 11:35:19 PDT
Carlos, Michael, what is the conclusion on this change given all the considerations about soup 2 support?
Comment 12 Michael Catanzaro 2022-05-16 12:52:55 PDT
Comment on attachment 459069 [details]
Patch

It's not very much code. Let's just do it.
Comment 13 Adrian Perez 2022-05-16 13:24:31 PDT
(In reply to Michael Catanzaro from comment #12)
> Comment on attachment 459069 [details]
> Patch
> 
> It's not very much code. Let's just do it.

FWIW, I agree, it'll be good to have this patch applied :)
Comment 14 EWS 2022-05-16 15:27:06 PDT
Committed r294268 (250616@main): <https://commits.webkit.org/250616@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459069 [details].