Bug 220509 - [SOUP] Stop using SoupRequest API to load files in preparation for libsoup3
Summary: [SOUP] Stop using SoupRequest API to load files in preparation for libsoup3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: libsoup3 220764
  Show dependency treegraph
 
Reported: 2021-01-11 06:30 PST by Carlos Garcia Campos
Modified: 2021-02-03 21:03 PST (History)
13 users (show)

See Also:


Attachments
Patch (36.64 KB, patch)
2021-01-11 06:41 PST, Carlos Garcia Campos
aperez: review-
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (36.38 KB, patch)
2021-01-20 05:15 PST, Carlos Garcia Campos
aperez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2021-01-11 06:30:32 PST
SoupRequest API is gone in libsoup3 and there's no replacement for file requests. GResource and data URI loads already happen in the web process so we only need to care about file and directory loads.
Comment 1 Carlos Garcia Campos 2021-01-11 06:41:53 PST
Created attachment 417376 [details]
Patch
Comment 2 Adrian Perez 2021-01-19 09:00:31 PST
Comment on attachment 417376 [details]
Patch

Patch looks good overall, but I have a few comments, could you please take
a look at them below? Thanks!

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

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:259
> +        else if (m_file) {

I checked the code a bit, and IIUC it looks like only one of m_inputStream,
m_multipartInputStream, m_soupRequest, or m_file can be valid at a time. It
would be nice to convert this to use std::variant<…> at some point — but of
course that's out of the scope of this patch, and we would need to build in
C++17 mode first 🤷️

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1182
> +    if (file != task->m_file.get())

As far as I can see, this could be an ASSERT(file == task->m_file.get())
because the callback is only used above (in line 247) where m_file.get()
is passed to g_file_query_info_async().

The only case in which m_file.get() could be different is after a call
to ::clearRequest(), as done below, but at this point here the pointer
should always be valid because the request has not been cleared yet.

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1200
> +    g_file_read_async(file, RunLoopSourcePriority::AsyncIONetwork, task->m_cancellable.get(), reinterpret_cast<GAsyncReadyCallback>(readFileCallback), protectedThis.leakRef());

This will try to read the file even if g_file_query_info_finish() fails.
Do I understand correctly that the idea is to try to read the file anyway
and let it fail, to avoid duplicating the code to handle read errors here?
If that's the case, a one-line comment explaining this will be handy for
whoever needs to read this code in the future.

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1222
> +    if (file != task->m_file.get())

Same here, this could be an ASSERT().

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1247
> +    if (file != task->m_file.get())

And here, this could be an ASSERT() as well.

> Source/WebKit/NetworkProcess/soup/Resources/directory.css:1
> +:root {

Does this CSS file also come from the Firefox directory listing code?
At least we should have a comment here stating the license for this file.

> Source/WebKit/NetworkProcess/soup/WebKitDirectoryInputStream.cpp:46
> +        "<html><head>"                                                  \

The backslashes at the end of line are not needed, the preprocessor will
happily paste strings together into a single one across multiple lines.

> Source/WebKit/NetworkProcess/soup/WebKitDirectoryInputStream.cpp:101
> +        "<tr>"                                                          \

Same here, the backslashes are not needed.
Comment 3 Carlos Garcia Campos 2021-01-20 04:58:38 PST
Comment on attachment 417376 [details]
Patch

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

>> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:259
>> +        else if (m_file) {
> 
> I checked the code a bit, and IIUC it looks like only one of m_inputStream,
> m_multipartInputStream, m_soupRequest, or m_file can be valid at a time. It
> would be nice to convert this to use std::variant<…> at some point — but of
> course that's out of the scope of this patch, and we would need to build in
> C++17 mode first 🤷️

Well, we have Variant in WTF, but that would be out of scope of this patch.
Comment 4 Carlos Garcia Campos 2021-01-20 05:15:19 PST
Created attachment 417960 [details]
Patch
Comment 5 Adrian Perez 2021-01-25 07:16:19 PST
Comment on attachment 417960 [details]
Patch

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

> Source/WebKit/NetworkProcess/soup/Resources/directory.css:1
> +:root {

Could we at least add a comment mentioning that this file comes from
libsoup and that therefore it's assumed that it has a license compatible
with WebKit's?

Other than this detail, I think the patch can be landed.
Comment 6 Carlos Garcia Campos 2021-01-26 03:49:30 PST
Committed r271879: <https://trac.webkit.org/changeset/271879>
Comment 7 Robert Jenner 2021-01-26 16:11:40 PST
Changes in r271879 have caused test:

fast/selectors/selection-window-inactive-text-shadow.html

to flakey fail.


Tracking in:

https://bugs.webkit.org/show_bug.cgi?id=221011