Bug 221672

Summary:

REGRESSION(r272636) [GTK] Crash and failures in API test /WebKit2Gtk/TestResources

Product:

WebKit

Reporter:

Lauro Moura <lmoura>

Component:

WebKitGTK

Assignee:

Lauro Moura <lmoura>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, berto, bugs-noreply, cgarcia, ews-watchlist, gustavo, mcatanzaro

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=221639
https://bugs.webkit.org/show_bug.cgi?id=186276
https://bugs.webkit.org/show_bug.cgi?id=177107

Attachments:

Description	Flags
Patch	none
Version returning error	ews-feeder: commit-queue-
Patch	none
Patch	mcatanzaro: review+

Lauro Moura

Reported 2021-02-10 06:55:50 PST

/WebKit2Gtk/TestResources Output: TEST: WebKitBuild/GTK/Release/bin/TestWebKitAPI/WebKit2Gtk/TestResources... /webkit/WebKitWebView/resources: PASS GLib-GIO-DEBUG: _g_io_module_get_default: Found default implementation local (GLocalVfs) for ‘gio-vfs’ /webkit/WebKitWebView/history-cache: PASS /webkit/WebKitWebView/sync-request-on-max-conns: PASS /webkit/WebKitWebResource/loading: PASS /webkit/WebKitWebResource/response: PASS /webkit/WebKitWebResource/mime-type: PASS /webkit/WebKitWebResource/suggested-filename: PASS /webkit/WebKitWebResource/active-uri: PASS /webkit/WebKitWebResource/get-data: PASS /webkit/WebKitWebResource/get-data-error: CRASH /webkit/WebKitWebResource/get-data-empty: FAIL GLib-GIO-DEBUG: _g_io_module_get_default: Found default implementation local (GLocalVfs) for ‘gio-vfs’ /webkit/WebKitWebPage/send-request: PASS GLib-GIO-DEBUG: _g_io_module_get_default: Found default implementation local (GLocalVfs) for ‘gio-vfs’ Unexpected failures (1) WebKitBuild/GTK/Release/bin/TestWebKitAPI/WebKit2Gtk/TestResources /webkit/WebKitWebResource/get-data-empty Unexpected crashes (1) WebKitBuild/GTK/Release/bin/TestWebKitAPI/WebKit2Gtk/TestResources /webkit/WebKitWebResource/get-data-error Ran 13 tests of 13 with 11 successful When running isolated, get-data-error only fails.

Attachments
Patch (6.10 KB, patch) 2021-02-11 20:40 PST, Lauro Moura	no flags	Details Formatted Diff Diff
Version returning error (4.93 KB, patch) 2021-02-12 10:57 PST, Lauro Moura	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (11.43 KB, patch) 2021-02-16 03:49 PST, Carlos Garcia Campos	no flags	Details Formatted Diff Diff
Patch (11.43 KB, patch) 2021-02-16 03:53 PST, Carlos Garcia Campos	mcatanzaro: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Lauro Moura

Comment 1 2021-02-11 20:40:49 PST

Created attachment 420088 [details] Patch

EWS Watchlist

Comment 2 2021-02-11 20:41:49 PST

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Lauro Moura

Comment 3 2021-02-11 20:47:45 PST

Not sure if this is the ideal approach, but might make more sense to return an empty resource on "error" than returning an error with an empty document, no? Given that error in this context means operation canceled, per the original check removed that Carlos mentioned in bug221639 #c9.

Carlos Garcia Campos

Comment 4 2021-02-12 00:04:32 PST

Comment on attachment 420088 [details] Patch Can't we keep previous behavior and data is nullptr and generate the error?

Lauro Moura

Comment 5 2021-02-12 05:52:06 PST

(In reply to Carlos Garcia Campos from comment #4) > Comment on attachment 420088 [details] > Patch > > Can't we keep previous behavior and data is nullptr and generate the error? That's an option too. In this case, would the error still be `G_IO_ERROR_CANCELLED` or would it be better to move to `G_IO_ERROR_FAILED` to indicate a more generic error as it now does not have the exact reason (i.e. whether it was canceled or was an empty resource)? Actually, in any case, it would be better to keep both tests, aligning the assertions, as one starts from `<HTML></HTML>` and the other from an empty resource.

Carlos Garcia Campos

Comment 6 2021-02-12 06:22:49 PST

(In reply to Lauro Moura from comment #5) > (In reply to Carlos Garcia Campos from comment #4) > > Comment on attachment 420088 [details] > > Patch > > > > Can't we keep previous behavior and data is nullptr and generate the error? > > That's an option too. In this case, would the error still be > `G_IO_ERROR_CANCELLED` or would it be better to move to `G_IO_ERROR_FAILED` > to indicate a more generic error as it now does not have the exact reason > (i.e. whether it was canceled or was an empty resource)? Yes, we should preserve the existing behavior if possible. > Actually, in any case, it would be better to keep both tests, aligning the > assertions, as one starts from `<HTML></HTML>` and the other from an empty > resource.

Lauro Moura

Comment 7 2021-02-12 10:57:05 PST

Created attachment 420149 [details] Version returning error

Michael Catanzaro

Comment 8 2021-02-12 17:05:09 PST

Comment on attachment 420149 [details] Version returning error View in context: https://bugs.webkit.org/attachment.cgi?id=420149&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebResource.cpp:350 > + if (!wkData) { > + g_task_return_new_error(task, G_IO_ERROR, G_IO_ERROR_CANCELLED, _("Operation was cancelled")); G_IO_ERROR_CANCELLED should be set by g_cancellable_set_error_if_cancelled(). I would be pretty surprised to see this error occur for any other reason....

Carlos Garcia Campos

Comment 9 2021-02-15 01:42:26 PST

The tests is still failing...

Carlos Garcia Campos

Comment 10 2021-02-15 01:44:29 PST

Comment on attachment 420149 [details] Version returning error View in context: https://bugs.webkit.org/attachment.cgi?id=420149&action=review > Tools/TestWebKitAPI/Tests/WebKitGLib/TestResources.cpp:584 > - g_assert_nonnull(data); > - g_assert_cmpuint(dataSize, ==, 1); > - g_assert_cmpint(data[0], ==, '\0'); > - g_assert_no_error(error.get()); > + g_assert_null(data); > + g_assert_error(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED); Why are we changing this? if we are preserving previous behavior we shouldn't change the test. In case of empty resource, the data shouldn't be null.

Lauro Moura

Comment 11 2021-02-15 04:35:26 PST

(In reply to Carlos Garcia Campos from comment #9) > The tests is still failing... Might have missed something in the refactor. Will check. (In reply to Carlos Garcia Campos from comment #10) > Comment on attachment 420149 [details] > Version returning error > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420149&action=review > > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestResources.cpp:584 > > - g_assert_nonnull(data); > > - g_assert_cmpuint(dataSize, ==, 1); > > - g_assert_cmpint(data[0], ==, '\0'); > > - g_assert_no_error(error.get()); > > + g_assert_null(data); > > + g_assert_error(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED); > > Why are we changing this? if we are preserving previous behavior we > shouldn't change the test. In case of empty resource, the data shouldn't be > null. IIUC, the issue after r272636 is that the callback for getResourceData family of functions removed the error argument[1]. Now the callback, receives null data for both cases (no frame, and actually empty resource). [1] https://trac.webkit.org/changeset/272636/webkit#file8

Carlos Garcia Campos

Comment 12 2021-02-15 05:01:23 PST

Comment on attachment 420149 [details] Version returning error View in context: https://bugs.webkit.org/attachment.cgi?id=420149&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebResource.cpp:354 > if (!wkData->bytes()) This is the empty case, if we are receiving null in both cases this is dead code now, but I'm not sure that's the case.

Lauro Moura

Comment 13 2021-02-15 19:57:14 PST

(In reply to Carlos Garcia Campos from comment #12) > Comment on attachment 420149 [details] > Version returning error > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420149&action=review > > > Source/WebKit/UIProcess/API/glib/WebKitWebResource.cpp:354 > > if (!wkData->bytes()) > > This is the empty case, if we are receiving null in both cases this is dead > code now, but I'm not sure that's the case. Yeah, this is what's happening. Both an empty resource and error ends up with API::Data* null in the resource callback. cc'ing Alex (author of the DataCallback->CompletionHandler patch). Trying to summarise: A) GLib usage of getResourceData relied on the error parameter to detect the cancellation of the request and notifying the public API users of WebKitWebResource. This parameter was removed from the DataCallback as part of r272636. B) As part of the move to completion handler, now the data callback is not invalidated when cancelling the load (as it was in m_callbacks.invalidate()). C) An alternative to the error code seems to be detecting a null data parameter in the data callback, which was and still is the case. D) One issue with this after r272636 is that for empty resources, while before we used to get an non-nil empty array in the data argument, now we get a nil one. Maybe something odd in the marshaling of the data reference through the IPC?

Carlos Garcia Campos

Comment 14 2021-02-16 03:00:14 PST

This is because now that we use sendWithAsyncReply, in case the message is cancelled, the callback is called with a new DataReference(). When the message is not cancelled, but the resource is empty the callback is called with the empty DataReference. In both cases API::Data::create() is now called taking the DataReference (instead of data + size) that returns nullptr in case of empty size. So, there are two problems here. There's a change in behavior that affects all the ports, because empty resources are broken, we should receive an empty API::Data instead of nullptr, getting an empty resource is not an error. That could be fixed by using API::Data::create() that receives data and size. But cancelling the message is also using an empty DataReference, so it's not possible to differentiate between a message cancellation and empty resource.

Carlos Garcia Campos

Comment 15 2021-02-16 03:49:46 PST

Created attachment 420446 [details] Patch

Carlos Garcia Campos

Comment 16 2021-02-16 03:53:13 PST

Created attachment 420447 [details] Patch

Michael Catanzaro

Comment 17 2021-02-16 05:57:12 PST

Comment on attachment 420447 [details] Patch Looks good. Needs owner approval from Alex.

Alex Christensen

Comment 18 2021-02-16 09:15:57 PST

Comment on attachment 420447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420447&action=review > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3738 > +void WebPage::getWebArchiveOfFrame(FrameIdentifier frameID, CompletionHandler<void(const Optional<IPC::DataReference>&)>&& callback) I don't see a code path where these would be called with nullopt. This change doesn't do anything. Same with all the changes in WebPage.{cpp, h, messages.in}

Michael Catanzaro

Comment 19 2021-02-16 09:29:59 PST

Comment on attachment 420447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420447&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebResource.cpp:350 > + if (!wkData) { > + g_task_return_new_error(task, G_IO_ERROR, G_IO_ERROR_CANCELLED, _("Operation was cancelled")); > + return; > + } Also you need to call g_task_return_error_if_cancelled(). Seems we were always missing that.

Carlos Garcia Campos

Comment 20 2021-02-16 22:05:54 PST

Comment on attachment 420447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420447&action=review >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3738 >> +void WebPage::getWebArchiveOfFrame(FrameIdentifier frameID, CompletionHandler<void(const Optional<IPC::DataReference>&)>&& callback) > > I don't see a code path where these would be called with nullopt. This change doesn't do anything. Same with all the changes in WebPage.{cpp, h, messages.in} Because it's generated code. It happens when the reply is cancelled.

Alex Christensen

Comment 21 2021-02-16 22:07:55 PST

Comment on attachment 420447 [details] Patch True. For the cocoa code, instead of using Optional<DataReference> I just made it so the API translation layer translates an empty data reference into null. There won't be any case where we want a non-null data object containing zero bytes. You might consider doing the same.

Carlos Garcia Campos

Comment 22 2021-02-16 22:22:45 PST

(In reply to Alex Christensen from comment #21) > Comment on attachment 420447 [details] > Patch > > True. > For the cocoa code, instead of using Optional<DataReference> I just made it > so the API translation layer translates an empty data reference into null. > There won't be any case where we want a non-null data object containing zero > bytes. You might consider doing the same. In GLib API null means there was an error and we should fill the GError too. An empty resources is not an error.

Carlos Garcia Campos

Comment 23 2021-02-17 02:21:01 PST

Committed r272991 (234194@main): <https://commits.webkit.org/234194@main>

Michael Catanzaro

Comment 24 2021-02-17 06:29:55 PST

(In reply to Carlos Garcia Campos from comment #22) > In GLib API null means there was an error and we should fill the GError too. > An empty resources is not an error. Indeed, history there in bug #186276. (In reply to Michael Catanzaro from comment #19) > Also you need to call g_task_return_error_if_cancelled(). Seems we were > always missing that. I see you landed without this. That means the function will return success even if it is cancelled before it is completed. That's weird, right? Even though the cancellation can't actually work properly because the GCancellable cannot be passed to the underlying WebKit API, we should still fake it by returning G_IO_ERROR_CANCELLED in case it got cancelled before the callback returned. That is, you've covered the tricky case where the call gets indirectly canceled at the WebKit level, but forgot the simple case where it's cancelled using the GCancellable. :)

Carlos Garcia Campos

Comment 25 2021-02-17 06:33:02 PST

(In reply to Michael Catanzaro from comment #24) > (In reply to Carlos Garcia Campos from comment #22) > > In GLib API null means there was an error and we should fill the GError too. > > An empty resources is not an error. > > Indeed, history there in bug #186276. > > (In reply to Michael Catanzaro from comment #19) > > Also you need to call g_task_return_error_if_cancelled(). Seems we were > > always missing that. > > I see you landed without this. That means the function will return success > even if it is cancelled before it is completed. That's weird, right? Even > though the cancellation can't actually work properly because the > GCancellable cannot be passed to the underlying WebKit API, we should still > fake it by returning G_IO_ERROR_CANCELLED in case it got cancelled before > the callback returned. > > That is, you've covered the tricky case where the call gets indirectly > canceled at the WebKit level, but forgot the simple case where it's > cancelled using the GCancellable. :) Yes, sorry, I forgot to comment about it. I'll do that in a separate patch, adding a specific test for that case too.

Michael Catanzaro

Comment 26 2021-02-17 06:45:26 PST

OK. I think this simple test would work, it just takes testWebResourceGetDataEmpty, adds a GCancellable, and then replaces the callback with the one from testWebResourceGetDataError: static void testWebResourceGetDataCancelled(Test* test, gconstpointer) { GRefPtr<GMainLoop> mainLoop = adoptGRef(g_main_loop_new(nullptr, FALSE)); GRefPtr<WebKitWebView> webView = WEBKIT_WEB_VIEW(Test::createWebView(test->m_webContext.get())); webkit_web_view_load_html(webView.get(), "", nullptr); g_signal_connect(webView.get(), "load-changed", G_CALLBACK(webViewLoadChanged), mainLoop.get()); g_main_loop_run(mainLoop.get()); auto* resource = webkit_web_view_get_main_resource(webView.get()); test->assertObjectIsDeletedWhenTestFinishes(G_OBJECT(resource)); GRefPtr<GCancellable> cancellable = adoptGRef(g_cancellable_new()); webkit_web_resource_get_data(resource, cancellable.get(), [](GObject* source, GAsyncResult* result, gpointer userData) { size_t dataSize; GUniqueOutPtr<GError> error; auto* data = webkit_web_resource_get_data_finish(WEBKIT_WEB_RESOURCE(source), result, &dataSize, &error.outPtr()); g_assert_null(data); g_assert_error(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED); g_main_loop_quit(static_cast<GMainLoop*>(userData)); }, mainLoop.get()); g_cancellable_cancel(cancellable.get()); g_main_loop_run(mainLoop.get()); } In theory, that should work....

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