Skip to content

Commit

Permalink
[WPE][GTK] API test TestDownloads `/webkit/Downloads/local-file-err…
Browse files Browse the repository at this point in the history
…or` is flaky

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

Reviewed by Carlos Garcia Campos.

In one of the subtests, we start downloading a local file and then
immediately cancel it.

When the download completes fast, `WebKitDownloadClient::didFinish()`
method can be called twice. Firstly, because the download is actually
completed, and secondly, from `Download::cancel()`'s completion handler,
which we set in `webkit_download_cancel()'.

Since we set `m_download` to `nullptr`
in `WebKitDownloadClient::didFinish()`, the second call will crash.

The solution is to call `WebKitDownloadClient::legacyDidCancel()`
instead of `WebKitDownloadClient::didFinish()` from the download's
cancel completion handler.

* Source/WebKit/UIProcess/API/glib/WebKitDownload.cpp:
* Source/WebKit/UIProcess/API/glib/WebKitDownloadClient.cpp:

Canonical link: https://commits.webkit.org/266725@main
  • Loading branch information
obyknovenius committed Aug 9, 2023
1 parent f11e0c6 commit b73ca66
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 14 deletions.
10 changes: 1 addition & 9 deletions Source/WebKit/UIProcess/API/glib/WebKitDownload.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -428,14 +428,6 @@ void webkitDownloadCancelled(WebKitDownload* download)

void webkitDownloadFinished(WebKitDownload* download)
{
if (download->priv->isCancelled) {
// Since cancellation is asynchronous, didFinish might be called even
// if the download was cancelled. User cancelled the download,
// so we should fail with cancelled error even if the download
// actually finished successfully.
webkitDownloadCancelled(download);
return;
}
if (download->priv->timer)
g_timer_stop(download->priv->timer.get());
g_signal_emit(download, signals[FINISHED], 0, nullptr);
Expand Down Expand Up @@ -602,7 +594,7 @@ void webkit_download_cancel(WebKitDownload* download)

download->priv->isCancelled = true;
download->priv->download->cancel([download = Ref { *download->priv->download }] (auto*) {
download->client().didFinish(download.get());
download->client().legacyDidCancel(download.get());
});
}

Expand Down
19 changes: 14 additions & 5 deletions Source/WebKit/UIProcess/API/glib/WebKitDownloadClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,22 +93,31 @@ class DownloadClient final : public API::DownloadClient {

void didFail(DownloadProxy& downloadProxy, const ResourceError& error, API::Data*) override
{
if (webkitDownloadIsCancelled(m_download.get()))
return;

ASSERT(m_download);
if (webkitDownloadIsCancelled(m_download.get())) {
// Cancellation takes precedence over other errors.
webkitDownloadCancelled(m_download.get());
} else
webkitDownloadFailed(m_download.get(), error);
webkitDownloadFailed(m_download.get(), error);
m_download = nullptr;
}

void didFinish(DownloadProxy& downloadProxy) override
{
if (webkitDownloadIsCancelled(m_download.get()))
return;

ASSERT(m_download);
webkitDownloadFinished(m_download.get());
m_download = nullptr;
}

void legacyDidCancel(WebKit::DownloadProxy&)
{
ASSERT(m_download);
webkitDownloadCancelled(m_download.get());
m_download = nullptr;
}

void processDidCrash(DownloadProxy&) override
{
m_download = nullptr;
Expand Down

0 comments on commit b73ca66

Please sign in to comment.