[WIP] webkitgtk: Fix Darwin issues #126101
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jtojnar
reviewed
Jun 7, 2021
Comment on lines
235
to
236
| gappsWrapperArgsHook # FIXME: currently runs at preFixup | ||
| wrapGApp $out/bin/MiniBrowser --argv0 MiniBrowser |
There was a problem hiding this comment.
Why not do this in postFixup then?
There was a problem hiding this comment.
Also could you add comment about why are we wrapping this? (Missing GSettings schemas? GStreamer plug-ins?)
jtojnar
reviewed
Jun 7, 2021
| @@ -222,6 +230,17 @@ stdenv.mkDerivation rec { | |||
| sed 43i'#include <CommonCrypto/CommonCryptor.h>' -i Source/WTF/wtf/RandomDevice.cpp | |||
| ''; | |||
|
|
|||
| postInstall = '' | |||
| mv $out/libexec/MiniBrowser $out/bin/MiniBrowser | |||
There was a problem hiding this comment.
To encourage people to actually run it when creating PRs by improving discoverability.
It looks like I got the source path wrong however, and it's actually libexec/webkit2gtk-4.0/MiniBrowser. It took a while to figure out because of the time it takes to compile WebKit.
There was a problem hiding this comment.
I would worry it might also encourage people to use it, which might not be secure. Would not adding a comment asking people to do that achieve the same?
There was a problem hiding this comment.
Good point. I did a bit of research and it seems that sandboxing isn't enabled for MiniBrowser, so that would be problematic if people started using it for normal browsing. I'll put it back to libexec in the next commit.
I think we can reconsider this after Nixpkgs WebKitGTK has switched to GTK 4, though. With GTK 4, sandboxing will become mandatory on WebKitGTK. With that fixed, MiniBrowser should be secure as any other WebKitGTK-based browser. MiniBrowser itself just deals with GTK windows and dialogs, delegating rest of the work to WebKitWebView. All the security heavy-lifting should be done in WebKit, so as long as WebKitWebView is secure by default, I believe it's safe for normal use.
|
I found the cause of the keypress problem and reported the issue upstream at https://bugs.webkit.org/show_bug.cgi?id=227360 |
|
Very cool, thanks for the update. |
|
Just a little heads-up, upcoming releases of WebKitGTK may not build due to it requiring the macOS SDK 10.15 or later. Here's what I've got trying to build the latest code from the |
I'm pretty sure we have support for this in libc++ and this can be bypassed, but I don't remember how. |
11 tasks
See #135989 for example |
This was referenced Dec 15, 2021
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
|
Please mark as unstale. |
stale
bot
removed
the
2.status: stale
stale
bot
added
the
2.status: stale
|
Been a while -- just checked out this branch and tried to |
stale
bot
removed
the
2.status: stale
|
macports has aften working on arm64 Macs |
wegank
added
2.status: stale
stale
bot
removed
the
2.status: stale
aadcg
pushed a commit
to atlas-engineer/nyxt
that referenced
this pull request
Mar 27, 2024
These are problems that GTK should solve, not us. Additionally GTK does not work well on macOS, particularly WebKitGTK+. See NixOS/nixpkgs#126101.
wegank
added
the
2.status: stale
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
This attempts to fix WebkitGTK issues on Darwin uncovered in #125113 and #126082.
The current fix:
checkPhase.MiniBrowserbinary. This is small and extremely useful for uncovering issues with the WebKitGTK build.WebKitWebProcessprocesses from appearing in the macOS Dock.The major remaining problems are:
MiniBrowser)Additional info for the keypress problem:
I've confirmed the behavior with the Nyxt package in #126082 and
MiniBrowserin this PR, and is probably reproducible with any browser built against WebKitGTK. Special keys like Backspace and Cmd key combinations would work normally, but alphanumerical key input won't be passed to webpages.After debugging
MiniBrowser, I found that keypresses are being passed towebkitWebViewBaseKeyPressEvent, but is ignored after runningWebKit::WebCore::InputMethodFilter::filterKeyEvent. My current theory is that this is an upstream issue with IME handling working incorrectly on Darwin.Additional info for the crashes:
Crashlog for WebKitGTK WebProcess obtained by running Nyxt
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)ping @mjlbach @jmercouris