Bug 223140 – REGRESSION(r274270): [WPE][GTK] Broke Epiphany test /embed/ephy-web-view/error-pages-not-stored-in-history

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

After changing WebKitSecurityOrigin from WebCore::SecurityOrigin to WebCore::SecurityOriginData, this Epiphany test now crashes:

(gdb) bt full
#0  __strchr_avx2 () at ../sysdeps/x86_64/multiarch/strchr-avx2.S:57
No locals.
#1  0x00007fc62915d2fe in ephy_permissions_manager_get_settings_for_origin (manager=0x12c4440, 
    origin=0x1b38440 "://") at ../dist-unpack/epiphany-3.38.3/lib/ephy-permissions-manager.c:127
        origin_path = 0x7fc5b8292fe8 "://"
        trimmed_protocol = 0x0
        settings = 0x0
        security_origin = 0x7fc61b97b360
        pos = 0x4 <error: Cannot access memory at address 0x4>
        __func__ = "ephy_permissions_manager_get_settings_for_origin"
#2  0x00007fc62915d504 in ephy_permissions_manager_get_permission (manager=0x12c4440, 
    type=EPHY_PERMISSION_TYPE_SHOW_ADS, origin=0x1b38440 "://")
    at ../dist-unpack/epiphany-3.38.3/lib/ephy-permissions-manager.c:186
        settings = 0x14021a0
        __func__ = "ephy_permissions_manager_get_permission"
#3  0x00007fc62923e5c9 in update_ucm_ads_state (web_view=0x1b22830, 
    uri=0x7fc61b93e340 "http://localhost:2984375930/") at ../dist-unpack/epiphany-3.38.3/embed/ephy-web-view.c:1484
        ucm = 0x1acb110
        permission = EPHY_PERMISSION_UNDECIDED
        enable = 0
        origin = 0x1b38440 "://"
        shell = 0x1402350
#4  0x00007fc62923e819 in load_changed_cb (web_view=0x1b22830, load_event=WEBKIT_LOAD_COMMITTED, user_data=0x0)
    at ../dist-unpack/epiphany-3.38.3/embed/ephy-web-view.c:1545
        uri = 0x7fc61b93e340 "http://localhost:2984375930/"
        view = 0x1b22830
        object = 0x1b22830

Problem is we wind up passing a bogus origin "://" to EphyPermissionsManager, which isn't prepared for that. The problem is webkit_security_origin_to_string(), which would have previously returned NULL in this case.

It's easy to fix. Alex, I've CCed you on this because I made one change to WebCore::SecurityOriginData:

diff --git a/Source/WebCore/page/SecurityOriginData.cpp b/Source/WebCore/page/SecurityOriginData.cpp
index 430cb8b0b3ac..7286fd6379fc 100644
--- a/Source/WebCore/page/SecurityOriginData.cpp
+++ b/Source/WebCore/page/SecurityOriginData.cpp
@@ -41,6 +41,9 @@ String SecurityOriginData::toString() const
     if (protocol == "file")
         return "file://"_s;
 
+    if (protocol.isEmpty())
+        return { };
+
     if (!port)
         return makeString(protocol, "://", host);
     return makeString(protocol, "://", host, ':', static_cast<uint32_t>(*port));

which I think is fine because a security origin with no protocol is pretty meaningless. Do you agree that change is correct? I could do it in WebKitSecurityOrigin itself if you prefer.

[Michael Catanzaro]

Actually, to be very conservative, I'll change it to return empty string only if both protocol and host are empty, OK?

[Michael Catanzaro]

Created attachment 423081 [details]
Patch

[EWS Watchlist]

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

[Michael Catanzaro]

Comment on attachment 423081 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSecurityOrigin.cpp:136
> +static void testBogusPort(Test*, gconstpointer)
> +{
> +    // Garbage in, garbage out...
> +    WebKitSecurityOrigin* origin = webkit_security_origin_new("http", "localhost", 9999);

That's... not a bogus port. I'll fix that before landing.

[Michael Catanzaro]

Created attachment 423088 [details]
Patch for landing

[EWS]

Committed r274375: <https://commits.webkit.org/r274375>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423088 [details].

[Darin Adler]

Comment on attachment 423088 [details]
Patch for landing

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

> Source/WebCore/page/SecurityOriginData.cpp:45
> +    if (protocol.isEmpty() && host.isEmpty())
> +        return { };

This is a cross-platform change. It’s good we have regression tests for this for [glib]. Is there also a way we can make a cross-platform test for it?

[Alex Christensen]

It could probably be done using WKSecurityOriginCreate.
I think this change is good cross-platform.  We certainly don't want "://" to be the origin.

[Michael Catanzaro]

We have a test WKSecurityOrigin.cpp already that's designed to be cross-platform, but it's not listed in any CMake file, so it's really XCode-specific unfortunately....