Bug 223140 - REGRESSION(r274270): [WPE][GTK] Broke Epiphany test /embed/ephy-web-view/error-pages-not-stored-in-history
Summary: REGRESSION(r274270): [WPE][GTK] Broke Epiphany test /embed/ephy-web-view/erro...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
Depends on:
Reported: 2021-03-12 14:10 PST by Michael Catanzaro
Modified: 2021-03-13 06:34 PST (History)
8 users (show)

See Also:

Patch (6.57 KB, patch)
2021-03-12 14:20 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch for landing (5.81 KB, patch)
2021-03-12 15:29 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2021-03-12 14:10:16 PST
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.
Comment 1 Michael Catanzaro 2021-03-12 14:15:30 PST
Actually, to be very conservative, I'll change it to return empty string only if both protocol and host are empty, OK?
Comment 2 Michael Catanzaro 2021-03-12 14:20:40 PST
Created attachment 423081 [details]
Comment 3 EWS Watchlist 2021-03-12 14:21:31 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
Comment 4 Michael Catanzaro 2021-03-12 15:08:54 PST
Comment on attachment 423081 [details]

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.
Comment 5 Michael Catanzaro 2021-03-12 15:29:28 PST
Created attachment 423088 [details]
Patch for landing
Comment 6 EWS 2021-03-12 17:55:31 PST
Committed r274375: <https://commits.webkit.org/r274375>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423088 [details].
Comment 7 Darin Adler 2021-03-12 17:58:56 PST
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?
Comment 8 Alex Christensen 2021-03-12 20:34:53 PST
It could probably be done using WKSecurityOriginCreate.
I think this change is good cross-platform.  We certainly don't want "://" to be the origin.
Comment 9 Michael Catanzaro 2021-03-13 06:34:30 PST
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....