[WPE][GTK] web process leak if webkit_download_set_destination is called with empty destination

[yury-s]

c985acc

[WPE][GTK] web process leak if webkit_download_set_destination is called with empty destination
https://bugs.webkit.org/show_bug.cgi?id=242217

Reviewed by Michael Catanzaro.

Fail if download destination is set to "".

 * Source/WebKit/UIProcess/API/glib/WebKitDownload.cpp:
  (webkit_download_set_destination):

Canonical link: https://commits.webkit.org/256737@main

a37d8ab

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe
	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
✅ 🛠 🧪 merge	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[yury-s]

I'm not sure if dispatching download failure is the right way to address this, maybe NetworkDataTaskClient should be somehow notified without triggering the client event.

[mcatanzaro]

What's the status of this?

[yury-s]

What's the status of this?

I believe the ball is on the reviewer's side. I've been waiting for a feedback. If you feel that the fix is wrong or not worth merging I'm fine with closing the PR. I don't remember all the details where it was bothering us and we likely fixed it downstream.

[mcatanzaro]

Nice test.

[mcatanzaro]

So I have an alternative suggestion: let's just add a g_return_if_fail(uri[0] != '\0') to the top of webkit_download_set_destination() and move on? If the application wishes to cancel the download, it could just use webkit_download_cancel(): no need to call webkit_download_set_destination() with an empty string. That's a lot more explicit.

[yury-s]

So I have an alternative suggestion: let's just add a g_return_if_fail(uri[0] != '\0') to the top of webkit_download_set_destination() and move on? If the application wishes to cancel the download, it could just use webkit_download_cancel(): no need to call webkit_download_set_destination() with an empty string. That's a lot more explicit.

I'm trying to add a test for that behavior but couldn't avoid CRASH of the the test process. I modified the test the following way:

    g_signal_connect(download.get(), "decide-destination", G_CALLBACK(+[](WebKitDownload* download, const gchar* suggestedFilename, gpointer) {
        g_test_expect_message(nullptr, G_LOG_LEVEL_CRITICAL, "*webkit_download_set_destination*");
        webkit_download_set_destination(download, "");
        g_test_assert_expected_messages();
        return TRUE;
    }), nullptr);

And expected that to pass but it still crashes. I've also tried g_test_log_set_fatal_handler and g_log_set_default_handler to no avail. Any ideas? If not, I can just add the check without new tests.

[webkit-early-warning-system]

EWS run on current version of this PR (hash a37d8ab)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe
	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
✅ 🛠 🧪 merge	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

[mcatanzaro]

Is the critical causing the crash? Then the test is probably running with G_DEBUG=fatal-criticals, which is incompatible with use of g_test_expect_message() for CRITICAL errors. You can check by modifying the test to use getenv(); I see it is set by Tools/Scripts/webkitpy/port/glib.py, but not sure if that is used when running API tests. You could arguably use g_test_trap_subprocess() for this, but honestly I'd say don't bother. We don't normally test that precondition violations emit criticals. It's enough to have the check in the code. So while it's good that you thought to try making an API test for everything you can, in this case that's overkill IMO.

[yury-s]

Is the critical causing the crash?

I believe so, it has to do with the added g_return_if_fail.

Then the test is probably running with G_DEBUG=fatal-criticals, which is incompatible with use of g_test_expect_message() for CRITICAL errors. You can check by modifying the test to use getenv();

Yes, it does run with G_DEBUG=fatal-criticals, calling unsetenv("G_DEBUG"); in the test doesn't help with the crash though.

We don't normally test that precondition violations emit criticals. It's enough to have the check in the code. So while it's good that you thought to try making an API test for everything you can, in this case that's overkill IMO.

Makes sense, I'll leave it as is then without new tests. PTAL.

[webkit-commit-queue]

Committed 256737@main (c985acc): https://commits.webkit.org/256737@main

Reviewed commits have been landed. Closing PR #1970 and removing active labels.