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.
Actually, to be very conservative, I'll change it to return empty string only if both protocol and host are empty, OK?
Created attachment 423081 [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
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.
Created attachment 423088 [details] Patch for landing
Committed r274375: <https://commits.webkit.org/r274375> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423088 [details].
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?
It could probably be done using WKSecurityOriginCreate. I think this change is good cross-platform. We certainly don't want "://" to be the origin.
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....