Bug 220509

Summary:

[SOUP] Stop using SoupRequest API to load files in preparation for libsoup3

Product:

WebKit

Reporter:

Carlos Garcia Campos <cgarcia>

Component:

WebKitGTK

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

annulen, aperez, berto, bugs-noreply, ews-watchlist, glenn, gustavo, gyuyoung.kim, jbedard, jenner, pgriffis, ryuan.choi, sergio

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=220981
https://bugs.webkit.org/show_bug.cgi?id=221127
https://bugs.webkit.org/show_bug.cgi?id=221379

Bug Depends on:

Bug Blocks:

220508, 220764

Attachments:

Description	Flags
Patch	aperez: review-, ews-feeder: commit-queue-
Patch	aperez: review+

Carlos Garcia Campos

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

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Carlos Garcia Campos

Comment 1 2021-01-11 06:41:53 PST

Created attachment 417376 [details] Patch

Adrian Perez

Comment 2 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.

Carlos Garcia Campos

Comment 3 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.

Carlos Garcia Campos

Comment 4 2021-01-20 05:15:19 PST

Created attachment 417960 [details] Patch

Adrian Perez

Comment 5 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.

Carlos Garcia Campos

Comment 6 2021-01-26 03:49:30 PST

Committed r271879: <https://trac.webkit.org/changeset/271879>

Robert Jenner

Comment 7 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

Note You need to log in before you can comment on or make changes to this bug.