| 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: | P2 | ||||||||||||
| 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: |
|
||||||||||||
Created attachment 420088 [details]
Patch
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 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. Comment on attachment 420088 [details]
Patch
Can't we keep previous behavior and data is nullptr and generate the error?
(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. (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. Created attachment 420149 [details]
Version returning error
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.... The tests is still failing... 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. (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 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. (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? 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. Created attachment 420446 [details]
Patch
Created attachment 420447 [details]
Patch
Comment on attachment 420447 [details]
Patch
Looks good.
Needs owner approval from Alex.
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} 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. 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. 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 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. Committed r272991 (234194@main): <https://commits.webkit.org/234194@main> (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. :) (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. 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....
|
/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.