Bug 194979 - [GTK][WPE] Enable PSON
Summary: [GTK][WPE] Enable PSON
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
: 195005 (view as bug list)
Depends on: 195270 195273
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-23 13:06 PST by Michael Catanzaro
Modified: 2019-06-06 22:43 PDT (History)
11 users (show)

See Also:


Attachments
Patch (1.40 KB, patch)
2019-02-23 13:07 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.35 KB, patch)
2019-02-25 07:23 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (18.62 KB, patch)
2019-06-06 00:37 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2019-02-23 13:06:38 PST
Enable PSON. Seems to work. YOLO, etc.
Comment 1 Michael Catanzaro 2019-02-23 13:07:55 PST
Created attachment 362832 [details]
Patch
Comment 2 Zan Dobersek 2019-02-24 06:40:21 PST
WPE output locks up on a backwards navigation.

It'd be nice to enable this by default, but there has to be a way to override it at runtime.
Comment 3 Michael Catanzaro 2019-02-24 14:14:53 PST
Hmm, back/forward works in GTK MiniBrowser.

We don't want to provide a way to disable it because it's required for Spectre mitigation. If it doesn't work yet, we should just not enable it.
Comment 4 Chris Dumez 2019-02-24 14:25:56 PST
Looks like you may need some fixes specific to your ports. PSON has been enabled on macOS and iOS by default for a while now and works as expected although we are still ironing out a few (minor) quirks.
Comment 5 Michael Catanzaro 2019-02-25 06:47:44 PST
(In reply to Chris Dumez from comment #4)
> Looks like you may need some fixes specific to your ports. PSON has been
> enabled on macOS and iOS by default for a while now and works as expected
> although we are still ironing out a few (minor) quirks.

Yeah, I don't have time to look into it soon though, so we can either enable it only for GTK and not for WPE, or just not do it yet.

Let's try the former, actually, and see if anyone tries problems.
Comment 6 Michael Catanzaro 2019-02-25 06:48:15 PST
(Note: it's expected that something will be wrong, but at least back/forward does work for me.)
Comment 7 Michael Catanzaro 2019-02-25 07:23:00 PST
Created attachment 362902 [details]
Patch
Comment 8 Zan Dobersek 2019-02-25 08:58:57 PST
If you can, please open a WPE-specific bug for this. Otherwise I will when I get to it.
Comment 9 WebKit Commit Bot 2019-02-25 10:04:13 PST
Comment on attachment 362902 [details]
Patch

Clearing flags on attachment: 362902

Committed r242045: <https://trac.webkit.org/changeset/242045>
Comment 10 WebKit Commit Bot 2019-02-25 10:04:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Michael Catanzaro 2019-02-25 12:44:14 PST
Before:

Failed 2 jsc tests failed 25 failures 2 new passes 4 flakes api tests: 7 failures, 3 crashes, 13 timeouts webdriver-test 48 failures, 7 new passes

After:

Failed 1 jsc test failed 26 failures 3 new passes 7 flakes api tests: 7 failures, 3 crashes, 12 timeouts webdriver-test 49 failures, 7 new passes

👍
Comment 12 Carlos Garcia Campos 2019-03-04 03:58:48 PST
(In reply to Michael Catanzaro from comment #11)
> Before:
> 
> Failed 2 jsc tests failed 25 failures 2 new passes 4 flakes api tests: 7
> failures, 3 crashes, 13 timeouts webdriver-test 48 failures, 7 new passes
> 
> After:
> 
> Failed 1 jsc test failed 26 failures 3 new passes 7 flakes api tests: 7
> failures, 3 crashes, 12 timeouts webdriver-test 49 failures, 7 new passes
> 
> 👍

Layout tests are not a good way to test PSON, since there aren't many cross-site navigations. We are not yet ready to enable PSON.
Comment 13 WebKit Commit Bot 2019-03-04 03:59:31 PST
Re-opened since this is blocked by bug 195273
Comment 14 Michael Catanzaro 2019-03-04 07:14:34 PST
Hm, I indeed never tested compositing mode. I only tested webkitgtk.org in MiniBrowser. I tested several other sites, but in Epiphany, without compositing mode. :(
Comment 15 Michael Catanzaro 2019-03-04 07:15:47 PST
(In reply to Michael Catanzaro from comment #14)
> I only tested webkitgtk.org in MiniBrowser.

And yeah, in retrospect that sounds pretty dumb, since there's no cross-site navigations involved there.
Comment 16 Carlos Garcia Campos 2019-06-06 00:37:10 PDT
Created attachment 371478 [details]
Patch

Let's try again now.
Comment 17 Carlos Garcia Campos 2019-06-06 00:48:00 PDT
Chris, do you plan to keep the WebProcessPool configuration option to enable/disable and/or the web preference? I wonder if we should add API to allow disabling PSON and how. What about the other configurations? alwaysKeepAndReuseSwappedProcesses is only for testing/debugging, right? and processSwapsOnWindowOpenWithOpener? I think we always use related views for window.open().
Comment 18 Carlos Garcia Campos 2019-06-06 00:53:02 PDT
*** Bug 195005 has been marked as a duplicate of this bug. ***
Comment 19 Carlos Garcia Campos 2019-06-06 04:34:08 PDT
Committed r246148: <https://trac.webkit.org/changeset/246148>
Comment 20 Michael Catanzaro 2019-06-06 06:24:19 PDT
Great work.

(In reply to Carlos Garcia Campos from comment #17)
> Chris, do you plan to keep the WebProcessPool configuration option to
> enable/disable and/or the web preference? I wonder if we should add API to
> allow disabling PSON and how. What about the other configurations?
> alwaysKeepAndReuseSwappedProcesses is only for testing/debugging, right? and
> processSwapsOnWindowOpenWithOpener? I think we always use related views for
> window.open().

I don't think we should expose any of these.
Comment 21 Chris Dumez 2019-06-06 08:18:31 PDT
(In reply to Carlos Garcia Campos from comment #17)
> Chris, do you plan to keep the WebProcessPool configuration option to
> enable/disable and/or the web preference? I wonder if we should add API to
> allow disabling PSON and how. 

Ideally, PSON becomes de default everywhere and there is no reason to disable it. I do not think we should expose API for this. Apple currently has private API (SPI) but hopefully it will get dropped.

> What about the other configurations?
> alwaysKeepAndReuseSwappedProcesses is only for testing/debugging, right?

Right, solely for debugging and I suspect we'll drop support for this soon because we're not using it and it has some maintenance cost.

> and
> processSwapsOnWindowOpenWithOpener? I think we always use related views for
> window.open().

This is an experiment at this point and does not work yet. No need to expose this.
Comment 22 Carlos Garcia Campos 2019-06-06 22:43:01 PDT
Perfect, thanks Chris!