Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to WebKit2 #311

Closed
mvz opened this issue Feb 3, 2016 · 48 comments
Closed

Upgrade to WebKit2 #311

mvz opened this issue Feb 3, 2016 · 48 comments
Assignees
Milestone

Comments

@mvz
Copy link

mvz commented Feb 3, 2016

WebKit 1 has security issues that won't be resolved. Please consider upgrading to WebKit2.

See https://blogs.gnome.org/mcatanzaro/2016/02/01/on-webkit-security-updates/

@Leiaz
Copy link
Collaborator

Leiaz commented Feb 16, 2016

I've looked into this (see webkit2 branch.) Upgrading would fix #315.

Some things are broken :

WebKit 2 uses global proxy settings, there is no API to set a custom proxy :

WebKitWebView makes its own scrollbars and doesn't implement GtkScrollable anymore. The scrollbars are not in the same style as other scrollbars in the application. And the headlines skimming functionality (Ctrl+Space) is broken.

The functions to check if something is selected are gone, so the "Copy" action in the context menu is always active.

@lwindolf
Copy link
Owner

@Leiaz The feature breaking make this a hard upgrade. I could live with the "Copy" action missing, but the skimming is not nice.

@Leiaz
Copy link
Collaborator

Leiaz commented Feb 26, 2016

It can probably be made to work by making a Web Extension for WebKit and using the DOM API.

Epiphany can serve as example, they use DBus to communicate with the extension.

@lwindolf
Copy link
Owner

Argh... this cannot be right, everyone using Webkit2Gtk needs to do this? Then it belongs into Webkit2Gtk...

@bitlord
Copy link

bitlord commented Feb 27, 2016

The scrollbars are not in the same style as other scrollbars in the application.

I guess this is fixed with latest >=webkit-gtk-2.11.90[1]

  • Switch to use overlay scrollbars like all other GTK+ widgets and ensure the behavior is consistent with GTK+ too.

[1] http://webkitgtk.org/2016/02/19/webkitgtk2.11.90-released.html

@rich-coe
Copy link
Contributor

rich-coe commented Mar 7, 2016

@Leiaz I tried buiding this, but I suspect I'm doing something wrong as I get blank window where the feed item is displayed. Is there a minimum webkit2-gtk needed for this to work?

Update: I updated libgtk gtk3-devel glibmm2-devel libXinerama-devel libXrandr-devel and wayland-devel and got it to work.

@Leiaz
Copy link
Collaborator

Leiaz commented Mar 7, 2016

I'm using webkit2gtk 2.10.7, but 2.6 should be enough. I'm not seeing those errors. Which version are you using ?

@rich-coe
Copy link
Contributor

rich-coe commented Mar 8, 2016

I had webkit2gtk 2.6.6 installed, but after updating to 2.10.7, still had issues because the dependencies had not installed. I also had issues, because I had older versions of some webkit/pango dependencies installed in /usr/local.

@rich-coe
Copy link
Contributor

rich-coe commented Mar 8, 2016

Some observations on the webkit2 port.
(1) Control-space no longer scrolls the current article, but acts like 'next' when the article is complete.
(2) If the displayed page crashes, the liferea app doesn't crash.
(3) I have not had any javascript errors. Still testing. So far so good.

@Leiaz
Copy link
Collaborator

Leiaz commented Mar 10, 2016

I've made a web extension and now Control+space and the copy menu item (should) work.

As noted in commit comments there is webkit_web_view_can_execute_editing_command that can be used to check if copy is to be enabled, but it is asynchronous so I don't think I could use it without changing the interface with LifereaHtmlView (?). So I'm using the web extension.

@rich-coe
Copy link
Contributor

Control-space is working. Thank you.

I noticed this message before, during startup and when a picture paints, and didn't log it before.
(WebKitWebProcess:24372): GLib-GObject-WARNING **: gsignal.c:2516: signal 'starting' is invalid for instance '0x21659b0' of type 'SoupMessage'

@Leiaz
Copy link
Collaborator

Leiaz commented Apr 1, 2016

@rich-coe This is from WebKit, the doc here says this signal is new to libsoup 2.50.

I've made some changes to do the menu with actions and use asynchronous calls.
The web extension is now only used for headlines skimming. It could be done in Javascript instead but this would require having Javascript enabled ...

I've removed the proxy settings. I wanted to change the deprecated GtkVBox GtkHBox to GtkBox but then I saw the comment in the doc saying that GtkGrid should be used because GtkBox is going to be deprecated too. So I didn't do it but it may be simpler to remake the ui files in the new Glade.

@lwindolf Do I send a pull request or do you prefer Javascript to a web extension ?

@mcatanzaro
Copy link

Note that we only connect to SoupMessage::starting if compiled against sufficiently-new libsoup. The only way you could be seeing that error is if your WebKit was compiled against a newer version of libsoup than you have installed (a serious distro bug).

It was added here: http://trac.webkit.org/changeset/180928

@rich-coe
Copy link
Contributor

rich-coe commented Apr 2, 2016

@mcatanzaro I upgraded libsoup and that fixed it. I had upgraded webkit from a new repository. Unfortunately rpm is not very good at dependencies.

@bitlord
Copy link

bitlord commented Apr 9, 2016

For some time I'm trying to make this run on my system, but it is always stuck at startup, not sure if this is a relevant part, but it may be, it tries to "move"/"rename" a file which doesn't exist. I tried to remove all confings/cache ... ~/.cache/liferea ~/.local/share/liferea ~/.config/liferea

close(30) = 0 munmap(0x7fc828000000, 4096) = 0 rename("/home/branko/.config/liferea/feedlist.opml~", "/home/branko/.config/liferea/feedlist.opml") = 0 recvmsg(5, 0x7ffdd5ab45b0, 0) = -1 EAGAIN (Resource temporarily unavailable) poll([{fd=3, events=POLLIN}, {fd=5, events=POLLIN}, {fd=10, events=POLLIN}, {fd=22, events=POLLIN}], 4, 2994) = 0 (Timeout) recvmsg(5, 0x7ffdd5ab45b0, 0) = -1 EAGAIN (Resource temporarily unavailable) poll([{fd=3, events=POLLIN}, {fd=5, events=POLLIN}, {fd=10, events=POLLIN}, {fd=22, events=POLLIN}], 4, 9997) = 0 (Timeout) recvmsg(5, 0x7ffdd5ab45b0, 0) = -1 EAGAIN (Resource temporarily unavailable) poll([{fd=3, events=POLLIN}, {fd=5, events=POLLIN}, {fd=10, events=POLLIN}, {fd=22, events=POLLIN}], 4, 9879) = 0 (Timeout) close(34) = 0 recvmsg(5, 0x7ffdd5ab45b0, 0) = -1 EAGAIN (Resource temporarily unavailable) poll([{fd=3, events=POLLIN}, {fd=5, events=POLLIN}, {fd=10, events=POLLIN}, {fd=22, events=POLLIN}], 4, 109) = 0 (Timeout) recvmsg(5, 0x7ffdd5ab45b0, 0) = -1 EAGAIN (Resource temporarily unavailable) poll([{fd=3, events=POLLIN}, {fd=5, events=POLLIN}, {fd=10, events=POLLIN}, {fd=22, events=POLLIN}], 4, 1026) = 0 (Timeout) close(29) = 0 recvmsg(5, 0x7ffdd5ab45b0, 0) = -1 EAGAIN (Resource temporarily unavailable) poll([{fd=3, events=POLLIN}, {fd=5, events=POLLIN}, {fd=10, events=POLLIN}, {fd=22, events=POLLIN}], 4, 87) = 0 (Timeout) close(28) = 0 recvmsg(5, 0x7ffdd5ab45b0, 0) = -1 EAGAIN (Resource temporarily unavailable) poll([{fd=3, events=POLLIN}, {fd=5, events=POLLIN}, {fd=10, events=POLLIN}, {fd=22, events=POLLIN}], 4, 328) = 0 (Timeout) close(35) = 0 recvmsg(5, 0x7ffdd5ab45b0, 0) = -1 EAGAIN (Resource temporarily unavailable) poll([{fd=3, events=POLLIN}, {fd=5, events=POLLIN}, {fd=10, events=POLLIN}, {fd=22, events=POLLIN}], 4, 256) = 0 (Timeout) close(27) = 0 recvmsg(5, 0x7ffdd5ab45b0, 0) = -1 EAGAIN (Resource temporarily unavailable) poll([{fd=3, events=POLLIN}, {fd=5, events=POLLIN}, {fd=10, events=POLLIN}, {fd=22, events=POLLIN}], 4, 8300) = 0 (Timeout) recvmsg(5, 0x7ffdd5ab45b0, 0) = -1 EAGAIN (Resource temporarily unavailable) poll([{fd=3, events=POLLIN}, {fd=5, events=POLLIN}, {fd=10, events=POLLIN}, {fd=22, events=POLLIN}], 4, 9994q) = 1 ([{fd=5, revents=POLLIN}]) recvmsg(5, {msg_name(0)=NULL, msg_iov(1)=[{"U\2\f\1=~U\0\3\24\4\0\20\0\0\0\0\0\0\24\24\24\24\24\0\0\3\37%\2\0\0", 4096}], msg_controllen=0, msg_flags=0}, 0) = 32 recvmsg(5, 0x7ffdd5ab45d0, 0) = -1 EAGAIN (Resource temporarily unavailable) recvmsg(5, 0x7ffdd5ab45b0, 0) = -1 EAGAIN (Resource temporarily unavailable)

@Leiaz
Copy link
Collaborator

Leiaz commented Apr 10, 2016

The file ~/.config/liferea/feedlist.opml~ is only supposed to exist temporarily and this line :

rename("/home/branko/.config/liferea/feedlist.opml~", "/home/branko/.config/liferea/feedlist.opml") = 0

Tells you the return value was 0, which means it succeeded.

One reason Liferea may fail to start is if you didn't install it, it can't find the correct GSettings schema. Otherwise can you post the gdb backtrace ?

@bitlord
Copy link

bitlord commented Apr 11, 2016

Thanks @Leiaz, it is built locally and installed locally, I used --prefix=/home/branko/bin and I installed it locally. As for Gsettings schemas, I did have issues with them, but I found a solution
GSETTINGS_SCHEMA_DIR="/home/branko/bin/share/glib-2.0/schemas/"
without it it "crashes" (Trap) at startup.

As for gdb, I don't have debug symbols, maybe I can rebuild some things and have debug symbols

#0 0x00007fece1db51cd in poll () from /lib64/libc.so.6 #1 0x00007fece22d6eec in ?? () from /usr/lib64/libglib-2.0.so.0 #2 0x00007fece22d6ffc in g_main_context_iteration () from /usr/lib64/libglib-2.0.so.0 #3 0x00007fece2af6c0c in g_application_run () from /usr/lib64/libgio-2.0.so.0 #4 0x000000000041a856 in main (argc=1, argv=0x7ffcc1fa0808) at main.c:258

@Leiaz
Copy link
Collaborator

Leiaz commented Apr 11, 2016

Liferea symbols should be enough for most things.

But I just realized, you mean it doesn't actually crash ? If you were using 1.10 before, the tray icon is now a plugin, so your window may be hidden.

@bitlord
Copy link

bitlord commented Apr 11, 2016

Ok, that was the issue, I was thinking about it before, but I excluded it as a problem when I removed my current config.

It works now with '-w shown'.

Thank you @Leiaz.

@bitlord
Copy link

bitlord commented Apr 26, 2016

I must say that his port works really good for me. There is only one minor issue, sometimes when you start liferea (in my case it's in systray) and update feeds (maybe not important) try to open one news item there is nothing in the webview (or however it is called), just blank, as a workaround I change view mode (Normal, Wide, Combined), switch from one to another (it starts working), and back to preferred one "Normal". I'm still on webkit 2.10.x and gtk+ 3.18.x, I'll try updating webkit to 2.12. I guess it will work with other "older" libs...

@bitlord
Copy link

bitlord commented Jul 27, 2016

After upgrading to latest stable webkit, gtk+, ... it is much better experience.

I would love to see this "merged" into official repos, and being updated, it works very well, and stable for me. Not to mention advantages of using modern webkit with bug and security fixes.

Thank you @Leiaz for this port, and thanks to @lwindolf and all contributors for liferea.

@lwindolf
Copy link
Owner

@bitlord Thanks for the feedback on your tests!

@Leiaz What is currently hindering me to merge it for 1.12 is the missing Webkit2 proxy support. Because it would hurt all non-GNOME users relying on Liferea's custom proxy settings.

@genodeftest
Copy link
Contributor

Hm, when choosing between a rarely used feature and getting a bunch of known security bugs fixed I'd choose the latter.

@jbicha
Copy link

jbicha commented Jul 31, 2016

I believe it would be best if proxy settings were set system-wide. Should every app that uses the Internet have its own proxy settings dialog? Because I think that's probably a really bad experience for users.

@mcatanzaro
Copy link

For the record, the current situation:

  • glib-networking uses GNOME system settings (GSettings via GProxyResolverGNOME) if GNOME_DESKTOP_SESSION_ID is present in the environment; this should be sufficient for all GNOME users unless you want to use a different proxy just for Liferea for some reason.
  • Otherwise, glib-networking uses libproxy (via GLibProxyResolver) which just checks the FTP_PROXY, HTTPS_PROXY, and HTTP_PROXY environment variables.

So the system-wide configuration (which is definitely best) should already handled satisfactorily.

I am not sure why anyone would want to use different proxy settings just for Liferea -- we would certainly never expose such settings in a GNOME app, for the reason @jbicha suggests -- but my understanding is that's the only missing feature here. So what exactly is needed from WebKit? Just a way to set the proxy-uri property of the SoupSession in the network process? If so, that shouldn't be too hard, it's just a matter of someone finding time to work on it and get a patch adding that new API through the WebKit review process. But if you want to be able to use a custom GProxyResolver, that's not going to be technically possible unfortunately.

@genodeftest
Copy link
Contributor

@mcatanzaro : When using e.g. Liferea with Tor, setting proxy through GNOME or through environment variables is a bad idea if you also want to use web services which will leak private data (e.g. gnome weather app (by design) or any browser on some websites or gnome-contacts with this bug). Users might want to separate those pieces of information from their RSS feeds.
Also, many provies (including Tor), are known to attack users. Using RSS through them would be ok, but not something like evolution or your web mail account.

@lwindolf
Copy link
Owner

lwindolf commented Aug 2, 2016

@mcatanzaro Another use case are the non-GNOME users (Windows, XFCE, Enlightenment). Dropping the manual proxy setting would remove usability a lot for those.

@lwindolf
Copy link
Owner

lwindolf commented Aug 2, 2016

@jbicha I agree the UX is not perfect. Still Liferea is not a GNOME application and cannot rely on the GNOME defaults. We workaround the UX issues by closely following the major dialog layouts (GNOME and Firefox) while defaulting to the GNOME settings.

@lwindolf
Copy link
Owner

lwindolf commented Aug 2, 2016

I believe we need to follow the environment variable hack to workaround, as implemented by Nuvola (https://github.com/tiliado/nuvolaplayer/blob/c01dd44d2cd1f7114ecd5bf5db7311c9850a107f/src/nuvolakit-runner/webkit/WebEngine.vala) as suggested in (https://bugs.webkit.org/show_bug.cgi?id=128674). It should have low impact on complexity and is reasonable to test.

This way we can switch to recent Webkit.

@Leiaz I'll merge your implementation soon. I hope you do not mind.

@lwindolf lwindolf self-assigned this Aug 2, 2016
@lwindolf lwindolf added this to the 1.12-RC1 milestone Aug 2, 2016
@mcatanzaro
Copy link

Nuvola's hack doesn't look safe, we've learned from crashes in GNOME that you can't modify the environment in a threaded program after creating your first secondary thread, as it's impractical to ensure the other threads will never access it at the same time. (setenv needs to be done at the very top of main, or it's almost surely not safe.) It'd be hard to do with a preferences dialog anyway. I think we just need to add some API along the lines of webkit_web_context_set_proxy_uri to WebKit.

@Leiaz
Copy link
Collaborator

Leiaz commented Aug 2, 2016

@mcatanzaro Yes it would be enough, setting the proxy-uri of the SoupSession is how it is done with WebKit 1.

@lwindolf I had removed the proxy settings, as the environment variable thing is a bit hacky and the users who want a different proxy for Liferea could set the env variables themselves (but yes it is less user friendly than a GUI). I think setting it the way Nuvola does will require restarting Liferea for changes to take effect.

You'll have to rebase or cherry-pick to remove the commits where I removed the proxy settings. There is also this pull request which I haven't merged.

@rich-coe
Copy link
Contributor

rich-coe commented Aug 3, 2016

The youtube video at embedded in this web page
https://www.geeksaresexy.net/2016/07/27/internet-ads-salesmen/
kills the webkit code.
I think it's a webkit issue, but I haven't tried it yet with the standalone webkit viewer.
webkit2gtk (4.0 I believe)

@mcatanzaro
Copy link

I have a WIP patch for WebKit in https://bugs.webkit.org/show_bug.cgi?id=128674, will try to finish it soonish.

@mcatanzaro
Copy link

I updated the WebKit bug with a new patch. Please test it out and see if it's sufficient for Liferea.

@Leiaz
Copy link
Collaborator

Leiaz commented Sep 5, 2016

Sorry for not having noticed this sooner, but there is currently an inconsistency in the way Liferea behave (master version at least). Liferea has three proxy settings : Auto Detect (use environment setting), No proxy or Manual Setting.

The proxy is configured by setting the SoupSession proxy-uri property. Setting this property has the side effect of unsetting the default proxy-resolver property of the SoupSession.

For both feed downloading and WebKit, when the user starts Liferea with either Auto Detect or No Proxy selected, proxy-uri isn't modified and the default proxy resolver is used. If the user changes the setting to either Auto Detect or No Proxy, the proxy-uri is set to NULL, which disables the default proxy resolver until Liferea is restarted.

@lwindolf Perhaps proxy settings could be reduced to two options: Environment Default or Manual Setting ? This would work nicely with @mcatanzaro patch.

The branch webkit2-with-proxy is my webkit2 branch without commits that removed proxy settings. The branch webkit2-with-proxy-patch uses @mcatanzaro 's patch.

@mcatanzaro
Copy link

mcatanzaro commented Sep 5, 2016

Hm. We could add a way to guarantee no proxy will be used (perhaps by passing "direct://" as the proxy URI?) That seems like it might be useful to have and easy to implement; what do you think? (It would unset both the proxy-uri and proxy-resolver properties of the SoupSession.)

Although, I wonder if, considering the fact that nobody noticed the existing Liferea option was broken until now, maybe this setting is not so important.

@Leiaz
Copy link
Collaborator

Leiaz commented Sep 7, 2016

With the api proposed on the bug page it shouldn't be a problem to (fix and) keep the option.

@genodeftest
Copy link
Contributor

The "Scrollbars have different style in WebKit2" issue is tracked here: https://bugs.webkit.org/show_bug.cgi?id=161735

@mcatanzaro
Copy link

Note that we didn't know about the scrollbar problem until today, since nobody ever reported it and we only test Adwaita.

@Leiaz
Copy link
Collaborator

Leiaz commented Sep 8, 2016

The scrollbars work fine here since 2.12 (or around that). They just weren't styled before. That bug is different.

@rich-coe
Copy link
Contributor

rich-coe commented Sep 8, 2016

On KDE Plasma 5, there's some UI widget that overlays the gtk scrollbar. I'm not sure why or what problem it's attempting to fix. It's ugly also.

@mcatanzaro
Copy link

mcatanzaro commented Sep 8, 2016

Does it look better in KDE if you run with GTK_OVERLAY_SCROLLING=0 in the environment?

If other GTK+ 3 apps do not experience this issue, please file a new WebKit bug separate from Milan's.

@rich-coe
Copy link
Contributor

rich-coe commented Sep 8, 2016

@mcatanzaro GTK_OVERLAY_SCROLLING=0 just makes the ugly scoll bar appear all the time instead of auto-hiding.

@lwindolf
Copy link
Owner

I've merged @Leiaz Webkit2 implementation. @Leiaz I marked all as GPLv2+ with you as author. Can you give me an "OK" for this please?

@ALL: I do not have scroll bar issues in GNOME and Unity, but will continue watching out for those.

Issues that need fixing before 1.12 though:

  • Article skimming broke
  • Very small font size on my system (maybe a HiDPI problem)
  • a Webkit2 version dependant use of the new proxy (#ifdef version > x.y and stdout info message otherwise)

Nonetheless I will close this ticket as it is about the Webkit2 migration and track the other issues as 1.12 integration tickets. Please everyone start posting tickets on

@Leiaz
Copy link
Collaborator

Leiaz commented Sep 16, 2016

@lwindolf OK. Another possible issue : I noticed that I'm using Glib macros that avoid a lot of boilerplate for GTypes but are new to glib 2.44. Current Debian stable (8 Jessie) is only at 2.42, but it has webkit2gtk-4.0. Do I make it work on Debian stable ?

For article skimming I'm afraid that webkit_dom_dom_window_scroll_by could be asynchronous ...

@mcatanzaro
Copy link

Regarding small fonts: the font size API is terrible to use and needs to be replaced, but in the meantime you have to handle it manually to get hidpi right. There is some boilerplate code you can copypaste from epiphany for this. See https://bugs.webkit.org/show_bug.cgi?id=142673 and https://git.gnome.org/browse/epiphany/tree/embed/ephy-embed-prefs.c#n206

Regarding proxy support: I'll try to have a new WebKit patch ready in the next week or two; my previous API proposal was rejected.

@mcatanzaro
Copy link

The proxy API has landed in WebKit; it will be available in WebKitGTK+ 2.16. Better late than never!

@lwindolf
Copy link
Owner

Great! Thanks for letting us now @mcatanzaro !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants