Bug 240200 – [SOUP2] Compute number of header bytes whe using soup 2

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Yury Semikhatsky]

[SOUP2] Compute number of header bytes whe using soup 2

[Yury Semikhatsky]

Created attachment 458986 [details]
Patch

[Carlos Garcia Campos]

Why not using soup3 instead? I'm not sure we want to add new features to soup2 code path at this point.

[Darin Adler]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4)
> since gpointer may be 32 bytes

Um, of course I mean 4 bytes, 32 bits.

[Adrian Perez]

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 ;-)

[Darin Adler]

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.

[Michael Catanzaro]

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.

[Yury Semikhatsky]

(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

[Yury Semikhatsky]

Created attachment 459069 [details]
Patch

[Yury Semikhatsky]

Carlos, Michael, what is the conclusion on this change given all the considerations about soup 2 support?

[Michael Catanzaro]

Comment on attachment 459069 [details]
Patch

It's not very much code. Let's just do it.

[Adrian Perez]

(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 :)

[EWS]

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].