[Webkit-unassigned] [Bug 231880] [GTK][WPE] Support setting status code and getting HTTP method in custom URI scheme handlers

[bugzilla-daemon at webkit.org]

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

Michael Catanzaro <mcatanzaro at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mcatanzaro at gnome.org
 Attachment #441555|commit-queue?               |commit-queue-
              Flags|                            |

--- Comment #4 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 441555
  --> https://bugs.webkit.org/attachment.cgi?id=441555
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=441555&action=review

Looks pretty good overall.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:173
> + * Since: 2.35

2.36

(2.35 is a development release leading up to 2.36. It's confusing.)

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:175
> +void webkit_uri_scheme_request_set_status_code(WebKitURISchemeRequest* request, int statusCode)

Since this is public API, it use gint. (Yes, they're exactly the same. It's just style.)

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:190
> + * Since: 2.35

2.36

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:192
> +const char* webkit_uri_scheme_request_get_http_method(WebKitURISchemeRequest* request)

Since this is public API, it should use gchar*.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:196
> +    return request->priv->task->request().httpMethod().utf8().data();

This is a use-after-free: after the function call returns, httpMethod().utf8() is destroyed and the caller is left with a dangling pointer. To avoid this, you'll need to cache the result in a CString object that you add to the priv struct. You can see three other examples of how to do it.

I remember making the exact same mistake in one of my first patches to WebKit....

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:-184
> -        response.setHTTPStatusText("OK"_s);

Maybe run this if the status code is 200?

> Source/WebKit/UIProcess/API/gtk/WebKitURISchemeRequest.h:76
> +WEBKIT_API void
> +webkit_uri_scheme_request_set_status_code (WebKitURISchemeRequest* request,
> +                                           int status_code);

The public headers use GNOME style, different from the WebKit style used in the C++ sources:

WEBKIT_API void
webkit_uri_scheme_request_set_status_code (WebKitURISchemeRequest *request,
                                                                                                gint                                                   status_code);

Note the parameter alignment is different, * hangs on the variable name, and int -> gint.

> Source/WebKit/UIProcess/API/gtk/WebKitURISchemeRequest.h:79
> +webkit_uri_scheme_request_get_http_method (WebKitURISchemeRequest* request);

WebKitURISchemeRequest *request

> Source/WebKit/UIProcess/API/wpe/WebKitURISchemeRequest.h:80
> +WEBKIT_API void
> +webkit_uri_scheme_request_set_status_code (WebKitURISchemeRequest* request, 
> +                                           int status_code);
> +
> +WEBKIT_API const gchar *
> +webkit_uri_scheme_request_get_http_method (WebKitURISchemeRequest* request);

Same style nits here.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:106
> +        URISchemeHandler(const char* reply, int replyLength, const char* mimeType, int statusCode)

Use a default argument:

URISchemeHandler(const char* reply, int replyLength, const char* mimeType, int statusCode = 200)

Then you don't need two separate overloads.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:137
> +
> +

Only one blank line here.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:181
> +    void registerURISchemeHandler(const char* scheme, const char* reply, int replyLength, const char* mimeType, int statusCode)

Again, you only need one implementation if you use a default argument:

void registerURISchemeHandler(const char* scheme, const char* reply, int replyLength, const char* mimeType, int statusCode = 200)

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:325
> +    test->registerURISchemeHandler("notfound", kBarHTML, strlen(kBarHTML), "text/html", 404);
> +    test->m_loadEvents.clear();
> +    test->loadURI("notfound:blank");
> +    test->waitUntilLoadFinished();
> +    g_assert_true(test->m_loadEvents.contains(LoadTrackingTest::LoadFailed));

What happens if you pass 200 as the status code? Does the test still pass (load fails with LoadTrackingTest::LoadFailed), or does the test fail? I'm nervous because it looks like the load would fail because "notfound:blank" is not a registered URI scheme, not because of the 404 status code. The test is only useful if it fails without your change, right?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211018/a2b4f9b0/attachment-0001.htm>