Calling webkit_web_context_register_uri_scheme() with a “special” URI scheme name (like “file” or “http”) which is not supposed to be overriden should do a check at the API entry, instead of trying to pass that down to WebKit unchecked and cause a failed assertion: ASSERTION FAILED: !WTF::URLParser::isSpecialScheme(canonicalizedScheme.value()) Backtrace: #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x0000007fafb8fb54 in __GI_abort () at abort.c:79 #2 0x0000007fb0d4a4c0 in CRASH_WITH_INFO(...) () at /home/buildroot/buildroot/rpi3/output/build/wpewebkit-2.28.0/DerivedSources/ForwardingHeaders/wtf/Assertions.h:660 #3 WebKit::WebPageProxy::setURLSchemeHandlerForScheme (this=this@entry=0x7fa37f6000, handler=..., scheme=...) at /home/buildroot/buildroot/rpi3/output/build/wpewebkit-2.28.0/Source/WebKit/UIProcess/WebPageProxy.cpp:9114 #4 0x0000007fb0e5f7dc in webkitWebContextCreatePageForWebView (context=0x287691e0, webView=<optimized out>, webView@entry=0x287ac100, userContentManager=<optimized out>, relatedView=<optimized out>) at /home/buildroot/buildroot/rpi3/output/build/wpewebkit-2.28.0/Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1828 #5 ...
Created attachment 395254 [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 http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Committed r259382: <https://trac.webkit.org/changeset/259382> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395254 [details].
<rdar://problem/61200217>
Note this makes it impossible to implement support for ftp. Probably only schemes that are actually supported by WebKit should be blacklisted.
(In reply to Michael Catanzaro from comment #5) > Note this makes it impossible to implement support for ftp. Probably only > schemes that are actually supported by WebKit should be blacklisted. It was already impossible to implement it outside of WebKit: without the early check added by this at the GTK/WPE API level, the assertion would be triggered anyway inside WebKit when trying to register an URI scheme handler for FTP. If we want to allow implementing FTP via a custom URI scheme handler, then that would be a separate issue, and we will need to do a number of changes inside WebKit. If anybody knows whether any application was (or intends to) do that, please go ahead and open a new bug :)
(In reply to Adrian Perez from comment #6) > (In reply to Michael Catanzaro from comment #5) > > Note this makes it impossible to implement support for ftp. Probably only > > schemes that are actually supported by WebKit should be blacklisted. > > It was already impossible to implement it outside of WebKit: without > the early check added by this at the GTK/WPE API level, the assertion > would be triggered anyway inside WebKit when trying to register an URI > scheme handler for FTP. By the way, there is an “ENABLE_FTPDIR” build option in WebKit, enabled by default. I think there is no support for returning documents of type “application/x-ftp-directory” from libsoup, and that's why even when it's enabled trying to load e.g. ftp://ftp.es.debian.org results in an error saying “The URL cannot be shown” ;-)
(In reply to Adrian Perez from comment #6) > It was already impossible to implement it outside of WebKit: without > the early check added by this at the GTK/WPE API level, the assertion > would be triggered anyway inside WebKit when trying to register an URI > scheme handler for FTP. That can't be true, because Epiphany registered this scheme unconditionally on startup until 3.36. That code has been there for years and was only removed for 3.36 because we noticed Chrome had decided to no longer handle ftp and decided to just give up on it. > If we want to allow implementing FTP via a custom URI scheme handler, > then that would be a separate issue, and we will need to do a number > of changes inside WebKit. If anybody knows whether any application was > (or intends to) do that, please go ahead and open a new bug :) So yeah, Epiphany prior to 3.36 implemented FTP via a custom URI scheme handler. If you think it would result in an assertion, then maybe I need to check to see if Epiphany is crashing on F31. We might have busted older Epiphany when upgrading to 2.28 if this assertion is new.
I'm reopening because I'm worried about adding criticals for old Ephy versions. Maybe we should switch to g_warning() instead? I don't think the code was previously invalid.
(In reply to Michael Catanzaro from comment #9) > I'm reopening because I'm worried about adding criticals for old Ephy > versions. Maybe we should switch to g_warning() instead? The ASSERT was added in r213686, but didn't affect us because we used legacy custom protocol implementation until r250597. I think r250597 is when the regression was introduced. Introducing criticals in previously-valid applications is not good, so let's switch to g_warning(). Also, let's not hardcode the list of protocols here.
Created attachment 402386 [details] Patch
Created attachment 402391 [details] Patch
Comment on attachment 402391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402391&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1280 > + g_critical("Cannot register invalid URI scheme %s", scheme); I would probably write this as: auto isValidScheme = !!WTF::URLParser::maybeCanonicalizeScheme(scheme); g_return_if_fail(iValidScheme); So the error message follows the usual format emitted by the g_return[_val]_if_fail() macros.
I need to use canonicalizedScheme below, and I can avoid !! by using C++ bool: auto canonicalizedScheme = WTF::URLParser::maybeCanonicalizeScheme(scheme); bool isValidScheme = canonicalizedScheme; g_return_if_fail(isValidScheme); OK? Honestly I prefer to write out the critical as I did originally, but you're the reviewer. ;)
(In reply to Michael Catanzaro from comment #14) > I need to use canonicalizedScheme below, and I can avoid !! by using C++ > bool: > > auto canonicalizedScheme = WTF::URLParser::maybeCanonicalizeScheme(scheme); > bool isValidScheme = canonicalizedScheme; > g_return_if_fail(isValidScheme); > > OK? Honestly I prefer to write out the critical as I did originally, but > you're the reviewer. ;) I don't have a very strong preference, to be honest… The reason why I was suggesting ending up using g_return_if_fail() would be to make sure it is GLib the one formatting the message, to make all the checks on API input parameters output messages with the same format.
(In reply to Adrian Perez from comment #15) > I don't have a very strong preference, to be honest… OK, I'm going to commit the original version of the patch, because I'd rather not expose a local variable in the error message. I like the error message g_return_if_fail() gives when used directly on public API function parameters, but when more complex decisions are needed, like in this case, I think it's nicer to write out a custom error message.
Committed r263369: <https://trac.webkit.org/changeset/263369> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402391 [details].