Bug 201033 – [GTK][WPE] WebKitWebPage ID doesn't match

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Milan Crha]

This had been reported upstream here:
https://gitlab.gnome.org/GNOME/evolution/issues/587

Using Fedora rawhide with webkit2gtk3 2.25.x (from .2 to .4), when one tries to open two composer windows in Evolution the second composer lost track of the WebKitWebPage and claims that the WebKitWebProcess cannot find page with certain ID (interestingly, three independent runs and it's always number 22).

It's not only about the composer, I see the same when I run evolution in the Mail view, and switch to Contacts.

[Milan Crha]

Looking more closely to this, it seems that each WebKitWebView instance runs its own WebKitWebProcess. This wasn't the case, it used to run one WebKitWebProcess per each WebKitWebContext. Evolution uses two WebKitWebContext-s, one for all previews and one for all the composers. Each of these two WebKitWebProcess-es load different set of web-extension-s and evolution talks to those through these extensions over D-Bus. The value used in the call of webkit_web_context_set_web_extensions_initialization_user_data() when the corresponding WebKitWebContext is used to distinguish between these two WebKitWebProcess-es, just as it had been intended. Evolution relies on this behavior heavily.

What did change in WebKit in this regard, please?

[Milan Crha]

Maybe the default for webkit_web_context_set_web_process_count_limit() should be changed from 0 to 1, to keep backward compatibility? The properties of WebKitWebContext added in 2.26 do not seem related, thus it can be some fix around this count limit, I guess, which introduced the changed behaviour.

[Claudio Saavedra]

This might have gone under the radar, but the process limit has been made a no-op internally, which means that the API should be deprecated. See https://bugs.webkit.org/show_bug.cgi?id=193725

Instead there is a new API that might exposing, to set whether the process pool should use a single web process.

[Claudio Saavedra]

> Looking more closely to this, it seems that each WebKitWebView instance runs its own WebKitWebProcess.

This is something I need to investigate, as I think the default value of the internal SPI is FALSE so there shouldn't be only one web process per pool. I will check that.

[Milan Crha]

(In reply to Claudio Saavedra from comment #3)
> This might have gone under the radar, but the process limit has been made a
> no-op internally

I see, I can confirm it, I changed the value to '1' with no effect.

(In reply to Claudio Saavedra from comment #4)
> ..., as I think the default value of the internal SPI is FALSE so there
> shouldn't be only one web process per pool.

Looking into the patch attached in the bug you referenced it's correct. There is one 'YES' value, but I do not know what that is for. I do not see from the patch how I can change it. If the MiniBrowser/Safari can (according to the ChangeLog there), then I should be able to do it too, right?

Still, the previous behaviour was to have one process per WebKitWebContext. Keeping this the default would help for applications which do not recompile on every WebKitGTK+ update.

[Claudio Saavedra]

You cannot change it because we haven't exposed yet this SPI to a public API in the GTK and WPE ports.

[Milan Crha]

Hrm. I know this had been discovered/reported too late in the release cycle, but is there any chance (and hope) to have a fix in 2.26.0, please? I'd prefer to change the default (at least for the ports where it cannot be changed by the API user), but if new API will be added then I'm fine with it too, at least partly (my concern here is that, at least in Fedora, the webkit2gtk3 is updated down in three stable releases, while this particular change will break all the older application versions which won't be rebuilt with required fix).

[Claudio Saavedra]

Created attachment 377009 [details]
Patch

[Claudio Saavedra]

This is still a preliminary patch as I still need to add deprecation guards for the limit API and update the API tests as well, but feel free to give this a try for now. I will see if I can finish it tomorrow.

[Claudio Saavedra]

Also, I think the default in the now-to-be-deprecated API is 0, meaning no limit. So I think a default value for this should be True unless I am misunderstanding something. My patch is inconsistent in that but I'll fix it tomorrow.

[Michael Catanzaro]

See also: bug #193749

[Michael Catanzaro]

Comment on attachment 377009 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377009&action=review

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1626
> +void webkit_web_context_set_uses_single_web_process(WebKitWebContext* context, gboolean usesSingleWebProcess)

Problem is as I mentioned in the other bug, you can't call this function until after the WebKitWebContext has been constructed, by which time it's too late to change the process limit. It doesn't actually work, does it?

[Claudio Saavedra]

Then I'll make it a construct-only property, that should work. But I need to check the other bug tomorrow.

[Milan Crha]

(In reply to Claudio Saavedra from comment #10)
> Also, I think the default in the now-to-be-deprecated API is 0, meaning no
> limit.

Right, that's what the documentation says, though looking into bug #193725 comment #2, if I read it correctly, then this:

-    if (m_processes.size() < maximumNumberOfProcesses())
-        return createNewWebProcess(websiteDataStore);

was always false for the default '0' returned by maximumNumberOfProcesses(), thus there had been "always" (+/- other conditions about page count) reused the old process, if existed. That is, the default behaviour was similar as if the maximumNumberOfProcesses() was 1. The actual behaviour of the code confirms it.

To have 0 as an unlimited count of processes it should look like:

    if (!maximumNumberOfProcesses() || m_processes.size() < maximumNumberOfProcesses())
        return createNewWebProcess(websiteDataStore);

I'll give a try to your patch, it'll only take some time, till webkit is built here.

[Claudio Saavedra]

If your observation is correct, then we had a serious bug there. Since for security reasons a single web process per context is not a good idea, I'm afraid you'll have to patch your application to go back to the behavior desired.

My patch won't work for the reasons Michael explained earlier; this is set too late to work.

[Claudio Saavedra]

Created attachment 377109 [details]
Patch

[Claudio Saavedra]

This is still preliminary but feel free to try.

[Milan Crha]

(In reply to Milan Crha from comment #14)
> I'll give a try to your patch, it'll only take some time, till webkit is
> built here.

With only a quick test the previous patch (comment #8) fixed evolution. I'll try the new (comment #16) too.

[Milan Crha]

(In reply to Milan Crha from comment #18)
> I'll try the new (comment #16) too.

The second patch doesn't work, I get the same behaviour as without it.

[Claudio Saavedra]

(In reply to Milan Crha from comment #19)
> (In reply to Milan Crha from comment #18)
> > I'll try the new (comment #16) too.
> 
> The second patch doesn't work, I get the same behaviour as without it.

Did you change the value of the "single-web-process" in the WebKitWebContext? You need to. It's a construct-only property so you have to set it during creation, ie.:

WebKitWebContext *context = g_object_new (WEBKIT_TYPE_WEB_CONTEXT, 
                                          ...some non-default properties...,
                                          "single-web-process", TRUE, NULL);

[Milan Crha]

> Did you change the value of the "single-web-process" in the WebKitWebContext?

Nope, I didn't. The first patch worked without any change on the client side, with which I kind of counted. This is good for backward compatibility too.

[Milan Crha]

> It's a construct-only property

It means I cannot use webkit_web_context_get_default() any more. Not a big deal, neither ideal, just another obstacle.

[Milan Crha]

(In reply to Milan Crha from comment #21)
> > Did you change the value of the "single-web-process" in the WebKitWebContext?
> 
> Nope, I didn't. The first patch worked without any change on the client
> side, with which I kind of counted. This is good for backward compatibility
> too.

Right, with that added it works.

What would be your suggestions with respect of dealing with these spontaneous page ID changes and (more importantly) new WebKitWebProcess-es with respect of reliable web-extension connection over D-Bus? A very silly workaround would be to use one WebKitWebContext for each WebKitWebView (because I can pass the initial arguments to the web extension, thus tell it what service name it should use), but it won't fix it everywhere, right?

[Michael Catanzaro]

(In reply to Milan Crha from comment #14)
> Right, that's what the documentation says, though looking into bug #193725
> comment #2, if I read it correctly, then this:
> 
> -    if (m_processes.size() < maximumNumberOfProcesses())
> -        return createNewWebProcess(websiteDataStore);
> 
> was always false for the default '0' returned by maximumNumberOfProcesses(),
> thus there had been "always" (+/- other conditions about page count) reused
> the old process, if existed. That is, the default behaviour was similar as
> if the maximumNumberOfProcesses() was 1. The actual behaviour of the code
> confirms it.

Huh, this really needs to be investigated. I'm confident such a bug does not exist, but it certainly looks like there is a problem with this code.

[Michael Catanzaro]

(In reply to Milan Crha from comment #23)
> Right, with that added it works.
> 
> What would be your suggestions with respect of dealing with these
> spontaneous page ID changes and (more importantly) new WebKitWebProcess-es
> with respect of reliable web-extension connection over D-Bus? A very silly
> workaround would be to use one WebKitWebContext for each WebKitWebView
> (because I can pass the initial arguments to the web extension, thus tell it
> what service name it should use), but it won't fix it everywhere, right?

Just include the page ID in any IPC messages and discard the message if it doesn't correspond to a currently-valid page ID? This is what Epiphany does and it doesn't cause any problems. It should be very rare for a page ID to become invalid during an IPC call.

[Michael Catanzaro]

Comment on attachment 377109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377109&action=review

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:467
> +        g_param_spec_boolean(

I think I would expose WebKitProcessModel here instead (you'll need to make sure whatever header that's declared in is added to the mkenums generation), then we can deprecate webkit_web_context_set_process_model() but don't have to deprecate WebKitProcessModel. Or maybe that's a bad idea. I'd wait for Carlos Garcia before spending time on this, in case he wants something different.

I'd certainly recommend solving bug #193749 at the same time, to ensure a coherent solution.

[Carlos Garcia Campos]

Comment on attachment 377109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377109&action=review

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:357
> +    configuration.setUsesSingleWebProcess(priv->usesSingleWebProcess);

I think this is a temporary setting, to be used only internally, that will be removed eventually, so we shouldn't expose it. Chris, could you confirm it? I think I asked you about this, but I'm not sure it was this setting...

[Carlos Garcia Campos]

I wonder if we could make an exception for custom protocols to not swap process on navigation between them, since the contents are provided by the application in the end.

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #27)
> Comment on attachment 377109 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377109&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:357
> > +    configuration.setUsesSingleWebProcess(priv->usesSingleWebProcess);
> 
> I think this is a temporary setting, to be used only internally, that will
> be removed eventually, so we shouldn't expose it. Chris, could you confirm
> it? I think I asked you about this, but I'm not sure it was this setting...

Chris? ^

[Chris Dumez]

(In reply to Carlos Garcia Campos from comment #29)
> (In reply to Carlos Garcia Campos from comment #27)
> > Comment on attachment 377109 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=377109&action=review
> > 
> > > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:357
> > > +    configuration.setUsesSingleWebProcess(priv->usesSingleWebProcess);
> > 
> > I think this is a temporary setting, to be used only internally, that will
> > be removed eventually, so we shouldn't expose it. Chris, could you confirm
> > it? I think I asked you about this, but I'm not sure it was this setting...
> 
> Chris? ^

Unless you have a specific need for it, I would discourage you from exposing it in the API. This setting is indeed private and defeats the work we're doing to put different origins in separate processes.

[Carlos Garcia Campos]

(In reply to Chris Dumez from comment #30)
> (In reply to Carlos Garcia Campos from comment #29)
> > (In reply to Carlos Garcia Campos from comment #27)
> > > Comment on attachment 377109 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=377109&action=review
> > > 
> > > > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:357
> > > > +    configuration.setUsesSingleWebProcess(priv->usesSingleWebProcess);
> > > 
> > > I think this is a temporary setting, to be used only internally, that will
> > > be removed eventually, so we shouldn't expose it. Chris, could you confirm
> > > it? I think I asked you about this, but I'm not sure it was this setting...
> > 
> > Chris? ^
> 
> Unless you have a specific need for it, I would discourage you from exposing
> it in the API. This setting is indeed private and defeats the work we're
> doing to put different origins in separate processes.

And what do you think about making custom protocols an exception and not swap processes when navigating between them?

[Milan Crha]

I'm still afraid of backward compatibility. This changes a lot for applications which have their own web extensions. You should bump the soname version or something like that, because such change deserves it ( https://gitlab.gnome.org/GNOME/evolution/issues/587#note_587156 ).

You have custom protocols and trusted origins. In case of Evolution every origin is trusted. All the extra work the changes bring in is unnecessary in case of Evolution and similar applications.

I do not understand one thing, quite important thing from my point of view. When the WebKitWebView has loaded a page with iframe-s from different origins and you spawn a new WebKitWebProcess for each iframe, and when the application wants to modify DOM in any of these iframe-s, for which it uses D-Bus, where the web extension spawns a service on expected name, does this new behaviour mean that:
a) each new WebKitWebProcess will load all web-extensions
b) each iframe will have its own WebKitWebPage ID
c) the extension tight to the main WebKitWebView's page (which provides
   the only page ID the application can know of) will not be able to
   read/write DOM changes in the iframe-s
, right ? In other words, should one keep a list of web extensions for a single WebKitWebView and decide to which of them the request should be sent based on some fuzzy logic ('fuzzy logic' in a meaning 'who knows how to decide which one in the list is the right one')? Not talking that this list can change dynamically, as the WeBkiWebView reloads its content?

I'm really confused by this, not talking about the complexity it adds to the WebKitGTK+ users.

By the way, evolution-data-server supports so called backend-per-process, which can spawn factory subprocesses for each backend (calendar/addressbook/...) if needed. That was the default. It was also for similar reasons as for which you do this (the initial idea behind the change was: 'if one backend causes crash, the rest will continue running), but it was requested to change the default behaviour and to not run the subprocesses, due to increased memory (and other resources) requirements. That to be able to run this on hardware with limited resources. Thus even the backend-per-process was a good idea, it was abandoned later. More can be seen here, if you are interested:
https://bugzilla.gnome.org/show_bug.cgi?id=793031

[Chris Dumez]

(In reply to Carlos Garcia Campos from comment #31)
> (In reply to Chris Dumez from comment #30)
> > (In reply to Carlos Garcia Campos from comment #29)
> > > (In reply to Carlos Garcia Campos from comment #27)
> > > > Comment on attachment 377109 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=377109&action=review
> > > > 
> > > > > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:357
> > > > > +    configuration.setUsesSingleWebProcess(priv->usesSingleWebProcess);
> > > > 
> > > > I think this is a temporary setting, to be used only internally, that will
> > > > be removed eventually, so we shouldn't expose it. Chris, could you confirm
> > > > it? I think I asked you about this, but I'm not sure it was this setting...
> > > 
> > > Chris? ^
> > 
> > Unless you have a specific need for it, I would discourage you from exposing
> > it in the API. This setting is indeed private and defeats the work we're
> > doing to put different origins in separate processes.
> 
> And what do you think about making custom protocols an exception and not
> swap processes when navigating between them?

Process Swapping has nothing to do with protocols at the moment, it is merely based on domain names. I would not expect process-swapping to occur between custom protocols, assuming they either use the same domain name (or do not have a domain name).
Adding Brady and Alex in cc in case they have input.

[Chris Dumez]

(In reply to Milan Crha from comment #32)
> I'm still afraid of backward compatibility. This changes a lot for
> applications which have their own web extensions. You should bump the soname
> version or something like that, because such change deserves it (
> https://gitlab.gnome.org/GNOME/evolution/issues/587#note_587156 ).
> 
> You have custom protocols and trusted origins. In case of Evolution every
> origin is trusted. All the extra work the changes bring in is unnecessary in
> case of Evolution and similar applications.
> 
> I do not understand one thing, quite important thing from my point of view.
> When the WebKitWebView has loaded a page with iframe-s from different
> origins and you spawn a new WebKitWebProcess for each iframe, and when the
> application wants to modify DOM in any of these iframe-s, for which it uses
> D-Bus, where the web extension spawns a service on expected name, does this
> new behaviour mean that:
> a) each new WebKitWebProcess will load all web-extensions
> b) each iframe will have its own WebKitWebPage ID
> c) the extension tight to the main WebKitWebView's page (which provides
>    the only page ID the application can know of) will not be able to
>    read/write DOM changes in the iframe-s
> , right ? In other words, should one keep a list of web extensions for a
> single WebKitWebView and decide to which of them the request should be sent
> based on some fuzzy logic ('fuzzy logic' in a meaning 'who knows how to
> decide which one in the list is the right one')? Not talking that this list
> can change dynamically, as the WeBkiWebView reloads its content?
> 
> I'm really confused by this, not talking about the complexity it adds to the
> WebKitGTK+ users.
> 
> By the way, evolution-data-server supports so called backend-per-process,
> which can spawn factory subprocesses for each backend
> (calendar/addressbook/...) if needed. That was the default. It was also for
> similar reasons as for which you do this (the initial idea behind the change
> was: 'if one backend causes crash, the rest will continue running), but it
> was requested to change the default behaviour and to not run the
> subprocesses, due to increased memory (and other resources) requirements.
> That to be able to run this on hardware with limited resources. Thus even
> the backend-per-process was a good idea, it was abandoned later. More can be
> seen here, if you are interested:
> https://bugzilla.gnome.org/show_bug.cgi?id=793031

No, process swapping on navigation is not for stability, it is for security and it has become increasingly important because of things like Spectre.
Note that we've had process-per-tab for a long time and that this matches more closely the backend-per-process model you're talking about.

[Milan Crha]

Looking into the WebKitGTK+ debugger, the Resources tab, I see there are opened:

   mail:​/​/accountname/path/to/message/id?​mode=0&​arguments
      mail:​/​/accountname/path/to/message/id?​part_id=.message.....
      mail:​/​/accountname/path/to/message/id?​part_id=.message.....

where the first is the main view's URL, the second and third are iframe src attributes, for two different parts of the viewed mail message. The other resources referenced in this message are:

   evo-file:///usr/share/evolution/theme/webview.css
   gtk-stock://pan-down-symbolic?size=4
   gtk-stock://go-next?size=4
   gtk-stock://x-evolution-arrow-down?size=4


The gtk-stock:// URL changes the "host" part according to the image it wants to receive. There can be also used evo-http:// and evo-https:// URLs, which consist of the external URLs provided by the message sender (like external images, CSS files and so on). Those download can be enabled/disabled in Preferences, or they can be loaded on demand by the user, but they are downloaded by Evolution itself, not by WebKitGTK+.

If I understand it properly, then the other resources are not a problem, they are not web pages, thus they do not cause WebKitWebProcess/Page change. That the all HTML pages are from mail:​/​/accountname/path/... means that that won't change the Page/ID too, at least until user moves to other account (the URL can be mail://imap1/Inbox/123 in one IMAP account and mail://imap2/test/321 in another account). Is that correct? I'm asking that, because if I'm going to add some workaround in Evolution for this (pity it had been discovered such late in the release cycle (I know you have it there for a long time)), then I'd like to reproduce this and test that my code will work as expected.

[Chris Dumez]

(In reply to Milan Crha from comment #35)
> Looking into the WebKitGTK+ debugger, the Resources tab, I see there are
> opened:
> 
>    mail:​/​/accountname/path/to/message/id?​mode=0&​arguments
>       mail:​/​/accountname/path/to/message/id?​part_id=.message.....
>       mail:​/​/accountname/path/to/message/id?​part_id=.message.....
> 
> where the first is the main view's URL, the second and third are iframe src
> attributes, for two different parts of the viewed mail message. The other
> resources referenced in this message are:
> 
>    evo-file:///usr/share/evolution/theme/webview.css
>    gtk-stock://pan-down-symbolic?size=4
>    gtk-stock://go-next?size=4
>    gtk-stock://x-evolution-arrow-down?size=4
> 
> 
> The gtk-stock:// URL changes the "host" part according to the image it wants
> to receive. There can be also used evo-http:// and evo-https:// URLs, which
> consist of the external URLs provided by the message sender (like external
> images, CSS files and so on). Those download can be enabled/disabled in
> Preferences, or they can be loaded on demand by the user, but they are
> downloaded by Evolution itself, not by WebKitGTK+.
> 
> If I understand it properly, then the other resources are not a problem,
> they are not web pages, thus they do not cause WebKitWebProcess/Page change.
> That the all HTML pages are from mail:​/​/accountname/path/... means that
> that won't change the Page/ID too, at least until user moves to other
> account (the URL can be mail://imap1/Inbox/123 in one IMAP account and
> mail://imap2/test/321 in another account). Is that correct? I'm asking that,
> because if I'm going to add some workaround in Evolution for this (pity it
> had been discovered such late in the release cycle (I know you have it there
> for a long time)), then I'd like to reproduce this and test that my code
> will work as expected.

For the workaround on your side, why don't you simply disable process-swap-on-navigation?

[Milan Crha]

(In reply to Chris Dumez from comment #36)
> For the workaround on your side, why don't you simply disable
> process-swap-on-navigation?

I'd like to give it a try, but how is that accessed in the WebKitGTK+ world, please? Simple `git grep process-swap-on-navigation` in the root checkout of WebKit sources on the master branch gives several hits in multiple ChangeLog file and only one hit in a code, and that in the comment:

> Source/WebCore/loader/FrameLoader.cpp:    // If the request came from a previous process due to process-swap-on-navigation then we should not modify the request.

The developer documentation for WebKitWebContext [1] doesn't give me anything of that name either. I'm sorry, but I do not develop WebKit, thus if it's any internal name for some feature, then I'm not aware of its meaning in the outside world.

[1] https://webkitgtk.org/reference/webkit2gtk/stable/WebKitWebContext.html

[Carlos Garcia Campos]

Comment on attachment 377109 [details]
Patch

r- because we don't really want to expose this setting.

[Carlos Garcia Campos]

(In reply to Chris Dumez from comment #36)
> (In reply to Milan Crha from comment #35)
> > Looking into the WebKitGTK+ debugger, the Resources tab, I see there are
> > opened:
> > 
> >    mail:​/​/accountname/path/to/message/id?​mode=0&​arguments
> >       mail:​/​/accountname/path/to/message/id?​part_id=.message.....
> >       mail:​/​/accountname/path/to/message/id?​part_id=.message.....
> > 
> > where the first is the main view's URL, the second and third are iframe src
> > attributes, for two different parts of the viewed mail message. The other
> > resources referenced in this message are:
> > 
> >    evo-file:///usr/share/evolution/theme/webview.css
> >    gtk-stock://pan-down-symbolic?size=4
> >    gtk-stock://go-next?size=4
> >    gtk-stock://x-evolution-arrow-down?size=4
> > 
> > 
> > The gtk-stock:// URL changes the "host" part according to the image it wants
> > to receive. There can be also used evo-http:// and evo-https:// URLs, which
> > consist of the external URLs provided by the message sender (like external
> > images, CSS files and so on). Those download can be enabled/disabled in
> > Preferences, or they can be loaded on demand by the user, but they are
> > downloaded by Evolution itself, not by WebKitGTK+.
> > 
> > If I understand it properly, then the other resources are not a problem,
> > they are not web pages, thus they do not cause WebKitWebProcess/Page change.
> > That the all HTML pages are from mail:​/​/accountname/path/... means that
> > that won't change the Page/ID too, at least until user moves to other
> > account (the URL can be mail://imap1/Inbox/123 in one IMAP account and
> > mail://imap2/test/321 in another account). Is that correct? I'm asking that,
> > because if I'm going to add some workaround in Evolution for this (pity it
> > had been discovered such late in the release cycle (I know you have it there
> > for a long time)), then I'd like to reproduce this and test that my code
> > will work as expected.
> 
> For the workaround on your side, why don't you simply disable
> process-swap-on-navigation?

We haven't exposed that option in the API because PSON was supposed to be mandatory for security reasons, and the internal setting was supposed to be private for testing. If that's not the case, and the setting won't be removed in the future I'll add an option to disable it in the API, there might be apps that don't really need it like evolution. Otherwise, what we can do for now, is adding an env var (yes, I also hate env vars) to disable it (only in 2.26 branch), so that apps can disable it while we figure out the issues during the next release cycle.

[Milan Crha]

Okay, anything would be better than this situation, even env var. Still without backward compatibility, if I understand it correctly.

I gave this a try, I mean a try on the evolution side. My idea was: if I cannot influence the number of WebProcess-es being run, then I'll create a WebContext for each WebView on which I'll set user data for the WebExtension, thus my extension will open on expected name. That should work, as long as I won't reuse the same WebContext on multiple WebView-s.

The result is kind of funny (I'm on git master at commit  dbcee3d87d966240becdba723dc38f12208dd256, aka git-svn-id: http://svn.webkit.org/repository/webkit/trunk@249275 268f45cc-cd09-0410-ab3c-d52691b4dbfc according to git log). I do not cache webkit_web_view_get_page_id(), I use it directly when issuing D-Bus requests towards my extension. I see the created WebExtension has been notified about created WebPage with ID 6 and that's the only one about which the WebExtension had been notified so far. Then the client side issues D-Bus calls with WebPage ID 5 (not 6). Those are initial requests on the WebPage, you can get an idea what they do from their names: EWebViewEnsureBodyClass, AddCSSRuleIntoStyleSheet and so on. These are run asynchronously, after the WebExtension registers itself on the D-Bus. These are important calls, thus I need them to happen, not to ignore their failure (for nonexistent page ID). The WebKitWebView documentation doesn't have "page-id" property (in which case I could theoretically listen for "notify::page-id" GObject signal for changes), neither there's a (GObject) signal for page-id changes, thus I've basically no idea when the page ID for the WebView changed. Being able to detect it, I can re-run my commands on the new page ID (even it would be awful from the coding and performance point of view). I do not know that the page ID changed on the WebExtension side, there was not created any WebPage with ID 5, only with ID 6 (otherwise I could add my own D-Bus signal on my WebExtension). This WebPage had been created before D-Bus name had been acquired, if it matters.

The biggest problem, the webkit_web_view_get_page_id() still returns number 5, even after such long time, after I wrote this comment.

[Milan Crha]

Created attachment 377687 [details]
test-wk2.c - reproducer client side

The first line contains a comment with a command to build & run it. Press Reload button at the top to begin the fun.

There's also printed on the console what page ID the WebView uses every 3 seconds. It doesn't change at all.

One thing I do not understand, but maybe it's intentional: when I press the Reload button 5 times, there are left running 4 WebKitWebProcess processes. They do not close as soon as they are useless. Is that intentional, like if they could be reused, or it's another overlook?

[Milan Crha]

Created attachment 377688 [details]
test-wk2-extension.c - a WebExtension

The first line contains a command line to build it.

It's a complementary WebExtension for the test-wk2.c above, which shows what is happening on the WebKitWebProcess(-es) side.

[Carlos Garcia Campos]

Milan, could you try simply disabling PSON in you wk build? You need to edit Source/WebKit/Shared/WebPreferencesDefaultValues.h and remove PLATFORM(GTK) from:

#if PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(GTK) || PLATFORM(WPE)
#define DEFAULT_PROCESS_SWAP_ON_CROSS_SITE_NAVIGATION_ENABLED true
#else
#define DEFAULT_PROCESS_SWAP_ON_CROSS_SITE_NAVIGATION_ENABLED false
#endif

[Michael Catanzaro]

Milan, you really need to use a private D-Bus connection, like I suggested in your GitLab issue, regardless. There's no reason you need to be using the session bus to implement a private Evolution feature, right? If you use private connections to each web extension, then you no longer need to use page IDs to identify web processes and your entire problem goes away (I think). Really, it seems like a silly problem to have.

Nevertheless, I'm very sympathetic to your concerns about this API break, and it's possible other applications might break too. A soname bump would be insufficient, because old Evolution compiled against new WebKit would not work. PSON really requires an API version bump, but an API version bump could imperil WebKit's fragile relationship with Debian security. We can be almost certain that a WebKit update that requires even a minor change in Evolution, like setting an environment variable or disabling a hypothetical public API PSON setting, would cause Debian to reject future WebKit updates. So I don't see any good solutions here.

Perhaps we should disable PSON at build time for 2.26 to buy time, but we don't want to keep doing that indefinitely.

(In reply to Milan Crha from comment #35)
> Looking into the WebKitGTK+ debugger, the Resources tab, I see there are
> opened:
> 
>    mail:​/​/accountname/path/to/message/id?​mode=0&​arguments
>       mail:​/​/accountname/path/to/message/id?​part_id=.message.....
>       mail:​/​/accountname/path/to/message/id?​part_id=.message.....
> 
> where the first is the main view's URL, the second and third are iframe src
> attributes, for two different parts of the viewed mail message. The other
> resources referenced in this message are:
> 
>    evo-file:///usr/share/evolution/theme/webview.css
>    gtk-stock://pan-down-symbolic?size=4
>    gtk-stock://go-next?size=4
>    gtk-stock://x-evolution-arrow-down?size=4
> 
> 
> The gtk-stock:// URL changes the "host" part according to the image it wants
> to receive.

Well Alex is the expert on URI parsing. I'm not sure whether these URIs have different hosts, or no host at all. Certainly the mail:// and evo-file:// URIs look like URIs with paths and no hosts. I guess PSON should consider URIs without hosts to be the same origin.

[Alex Christensen]

<script>alert(new URL("gtk-stock://pan-down-symbolic?size=4").hostname)</script>
The host is "pan-down-symbolic".  Use three slashes if you want there to be an empty host.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #45)
> Use three slashes if you want there to be an empty host.

Milan, can you try this? It should probably suffice.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #44)
> We can be
> almost certain that a WebKit update that requires even a minor change in
> Evolution, like setting an environment variable or disabling a hypothetical
> public API PSON setting, would cause Debian to reject future WebKit updates.
> So I don't see any good solutions here.

Maybe if we work with a friendly Debian maintainer to ensure Evolution gets patched before 2.26.0, then we can cross our fingers and hope no other apps are affected. Let's be extra careful and give the transition to 2.26 in Debian more time than usual, so we can smoke out any other affected apps in Fedora first.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #28)
> I wonder if we could make an exception for custom protocols to not swap
> process on navigation between them, since the contents are provided by the
> application in the end.

I thought it was a great idea, but it's not because the content could ultimately come from the untrusted source even though it's directly provided by the application. E.g. loading ephy-reader://example.com and ephy-reader://malicious.com in the same view without a process swap would be vulnerable even though it's a custom protocol.

[Milan Crha]

(In reply to Carlos Garcia Campos from comment #43)
> Milan, could you try simply disabling PSON in you wk build?

No, it did not help. I still get "Invalid page ID: 5". I only changed webkit sources and reverted my test changes on the evolution side; I did not fetch latest changes, thus I'm at the same state as comment #40 (I'll do it now). Though I think the test doesn't prove anything, because page IDs simply do not match, which I consider a regression, because it wasn't the case before (in time of comment #18).

(In reply to Michael Catanzaro from comment #44)
> Milan, you really need to use a private D-Bus connection, like I suggested
> in your GitLab issue, regardless.

I'm not sure what that would change. There is still the same problem, one WebKitWebView on the client side, and multiple WebKitWebProcess-es on the other side, some of them stale, while the WebView needs to talk to its counterpart on the WebProcess side, at least one of them. Giving unique D-Bus name for each WebProcess is odd, because I need to know on the WebView side to which proxy I should talk (and expect responses). Having it on a private D-Bus bus would help to what? To be able to "introspect" all the existing connections on it and "pick randomly" one of them, eventually send the message (and listen for responses) on all of them? I'm sorry, but that feels like a hit&miss game, not a deterministic programming. And having opened as many private D-Bus connections as many WebKitWebView instances are opened also doesn't feel the best approach.

[Milan Crha]

(In reply to Michael Catanzaro from comment #48)
> E.g. loading ephy-reader://example.com and ephy-reader://malicious.com in the
> same view without a process swap would be vulnerable even though it's a custom
> protocol.

Like if the web page author would like to address only epiphany browser with added URL of scheme he/she knows epiphany is using for trusted content? Hih, that would be interesting.

Thinking of it this way, the data being downloaded, even downloaded by Evolution itself, is trusted only in a way of a user setting, that can not download external content by default, even users can let Evolution do it and to pass it to WebKitWebView. It's for evo-http:// and evo-https:// schemes. The mail:// scheme is semi-trusted. It modifies text/html parts to not reference external data directly; in creates HTML content from text/plain and other parts as needed. All the other custom schemes Evolution uses can be considered trusted. I mean, even I said Evolution uses trusted content only, it was not accurate.

[Carlos Garcia Campos]

It's so late in the release cycle that I think the best approach is to disable PSON in 2.26 and work in all these issues (see also bug #200967) in trunk for 2.28. What do you think?

[Milan Crha]

Works for me, but I'm not sure whether it's a question for me :)

[Milan Crha]

(In reply to Milan Crha from comment #49)
> ... I still get "Invalid page ID: 5". I only changed webkit
> sources and reverted my test changes on the evolution side; I did not fetch
> latest changes, thus I'm at the same state as comment #40 (I'll do it now).

Still the same problem with git checkout at commit aec39837fb5478153424253f0a1871c5f2b2d7aa, aka git-svn-id: http://svn.webkit.org/repository/webkit/trunk@249376 268f45cc-cd09-0410-ab3c-d52691b4dbfc. The extension page ID is 6, while the WebView's page ID is 5.

It's with the change from comment #43. This off-by-one should be fixed as well. I believe both sides should use the same page ID, no? It was the case till recently, at least.

[Carlos Garcia Campos]

(In reply to Milan Crha from comment #53)
> (In reply to Milan Crha from comment #49)
> > ... I still get "Invalid page ID: 5". I only changed webkit
> > sources and reverted my test changes on the evolution side; I did not fetch
> > latest changes, thus I'm at the same state as comment #40 (I'll do it now).
> 
> Still the same problem with git checkout at commit
> aec39837fb5478153424253f0a1871c5f2b2d7aa, aka git-svn-id:
> http://svn.webkit.org/repository/webkit/trunk@249376
> 268f45cc-cd09-0410-ab3c-d52691b4dbfc. The extension page ID is 6, while the
> WebView's page ID is 5.
> 
> It's with the change from comment #43. This off-by-one should be fixed as
> well. I believe both sides should use the same page ID, no? It was the case
> till recently, at least.

I thought this was caused by process swapping...

[Carlos Garcia Campos]

So, your problem doesn't come from PSON directly, but from the fact that we removed the single process model because of PSON?

[Milan Crha]

(In reply to Carlos Garcia Campos from comment #55)
> So, your problem doesn't come from PSON directly, but from the fact that we
> removed the single process model because of PSON?

I'd say both. As I wrote in comment #49, in time of comment #18 the page IDs matched on both sides until I had more than one WebKitWebView instance opened for one WebKitWebContext (which ran multiple WebProcess-es and my D-Bus connection to the loaded extension didn't know about the other WebProcess). Right now page IDs do not match on both sides at all - that's why I call it a regression. If you've a pointer where to look in the sources, I can give it a try.

[Carlos Garcia Campos]

That's more complicated to revert then, because process limit is gone, and single process model would be now a construct only property of the context (we want a temporary solution, so new API is not an option). So, an env var to enable single process model would work for now, at least for evo. Then we need to review how evo is using the web extensions to understand what the problem is, what needs to be fixed/added in wk, and what to fix in evo too. I also think you shouldn't use the session bus for private communications between the same application. A private p2p connection like ephy does seems more appropriate for evo.

[Milan Crha]

Evolution currently distinguishes between previews and editors. Editors use their own WebKitWebContext, one for all of them, while previews use the default WebKitWebContext. My test changes I tried to do above stopped using the default WebContext. The way this works for editors (I'm willing to change it for previews as well due to this bug), is that the GUI process creates the WebContext on demand and reuses it, while it sets also user data for the WebExtension, containing expected D-Bus service name, where the GUI process listens and sends its requests. These requests contain page ID-s, thus the WebExtension side knows to which WebKitWebPage the request belongs. Once the WebKitWebProcess runs, the WebExtension reads the D-Bus name and registers itself on the session bus. (Problem number 1: when there are multiple WebProcess-es, each tries to register the same D-Bus service name, which results in failure for second and other WebProcess-es.) All of this worked fine until now, because the WebKit behaviour was that only one WebProcess was created for one WebContext (regardless the process-limit value, which evolution code doesn't touch and lefts it to default '0'; it's unrelated, only if it worked, then the issue with multiple WebProcess-es would be found earlier). When the WebExtension wants to send a D-Bus signal to the GUI part, it also includes the web page ID, as returned by WebKitWebPage, thus the GUI editor can distinguish whether the signal was meant for its WebKitWebView or for another WebKitWebView. (Problem number 2: web page IDs do not match now, which breaks all the logic in the code.)

That describes roughly how it works in Evolution. There are expectations, which can break the code when they are not satisfied. It worked, because the behaviour of Webkit was as expected by the code.

I'm fine to change things on the Evolution side, I hope it's obvious, it's only not ideal for backward compatibility, as we agreed above. I do not see how private D-Bus connection could help with the current situation, but I'll check what Epiphany does, then maybe I'll understand.

[Carlos Garcia Campos]

single process model was implemented as multiple process model with limit of 1. Removing the process limit, made the single process model no-op. There's still an internal option to use a single process model, but it's different, that's why it requires to be a construct-only property.

[Milan Crha]

Just for the record, WebKitWebPage::id != WebKitWebView::page-id (aka page ID mismatch introduced semi-recently) is causing problems to Epiphany too.

I could not test 3.33.92, because it needs too new glib, thus I tried 3.33.1, but the body of embed/ephy-web-view.c:page_created_cb() looks the same, also depending on the proper page ID provided, the callbacks and view->web_process_extension are never assigned, due to this mismatch. I added at the very top this debug print:

   printf ("%s: view:%p: expects:%" G_GUINT64_FORMAT " received:%" G_GUINT64_FORMAT "\n", __FUNCTION__, view, webkit_web_view_get_page_id (WEBKIT_WEB_VIEW (view)), page_id);

and I see on the console:

   page_created_cb: view:0x15a6b50: expects:7 received:8
   page_created_cb: view:0x15a6b50: expects:7 received:114
   page_created_cb: view:0x1644940: expects:113 received:114

The 113/114 is for the second tab I opend there. As the signal callbacks are not connected I cannot open an https:// page, whose server uses self-signed or otherwise "invalid" certificate. I can click "Technical info->Accept Risk and Proceed" red button, but it only makes a spinner at the tab spinning and nothing changes. Clicking "Go Back" blue button on this "Security violation" page works.

[Michael Catanzaro]

Well it seems you're right. A private connection wouldn't fix this because it's inherently racy. So now we know what's causing https://gitlab.gnome.org/GNOME/epiphany/issues/871.

This seems really hard to solve, and there's no time.

[Michael Catanzaro]

Actually this might be good, because it looks like a more straightforward bug. WebKitWebProcess::page-created should be emitted for every process swap. The latest ID visible to the web process (via webkit_web_page_get_id()) should match the ID visible to the UI process (via webkit_web_view_get_page_id()) unless a process swap has just occurred. It's OK for that page_created_cb() function to fail and return early if a process swap has just occurred, but if so, it should be called again imminently and succeed because another page created message should be incoming from the web process. For them to get completely desynced, as you show, there must be some bug, not an inherent problem with the new process model.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #62)
> Actually this might be good, because it looks like a more straightforward
> bug.

Well probably it's not good, just a race. The page ID coming from the web process is probably the new ID, and the UI process just hasn't noticed yet. If I guess right, the UI process's page ID will be updated momentarily, but when that happens it's already too late. The web process message has already been discarded due to the mismatch.

[Milan Crha]

(In reply to Michael Catanzaro from comment #61)
> Well it seems you're right. A private connection wouldn't fix this because
> it's inherently racy. So now we know what's causing
> https://gitlab.gnome.org/GNOME/epiphany/issues/871.

Is it with 3 weeks old WebKit master/trunk? The issue had been filled 3 weeks ago, according to gitlab.

(In reply to Michael Catanzaro from comment #63)
> If I guess right, the UI process's page ID will be updated momentarily, but
> when that happens it's already too late.

No, it's not updated at all. Try comment #41 + comment #42.

[Milan Crha]

(In reply to Milan Crha from comment #64)
> Is it with 3 weeks old WebKit master/trunk? The issue had been filled 3
> weeks ago, according to gitlab.

Nevermind. Gitlab says August 5, but if I try WebKitGTK+ with git checkout at commit ca8dc42af2fbce678b44fa83e61c4cb3ebbe23a0, aka from Aug 23 07:25:00 2019, then there is no page ID mismatch, thus it's not it.

[Michael Catanzaro]

Well I added some debug. When the page changes due to a process swap, the new page has the same ID as the old page. So that's really nice. It means this should work, in theory.

(In reply to Milan Crha from comment #60)
>    page_created_cb: view:0x15a6b50: expects:7 received:8
>    page_created_cb: view:0x15a6b50: expects:7 received:114
>    page_created_cb: view:0x1644940: expects:113 received:114

Expected result would actually be something like:

>    page_created_cb: view:0x15a6b50: expects:7 received:7
>    page_created_cb: view:0x15a6b50: expects:7 received:113
>    page_created_cb: view:0x1644940: expects:113 received:113

because each view receives the page created message for every page, and just discards messages that aren't intended for it.

So I see there is a race here, because 8 and 114 are being received and the associated functionality is all broken. I just can't reproduce. It might be a timing issue. But in theory, because the page ID stays the same even when the page is swapped out for another, it should be fixable.

[Michael Catanzaro]

Could you use SVN revision numbers when talking about specific WebKit commits, please? That makes it easier to see what commit you are talking about.

[Milan Crha]

(In reply to Michael Catanzaro from comment #67)
> Could you use SVN revision numbers when talking about specific WebKit
> commits, please?

Yes, of course. That's the:

   git-svn-id: http://svn.webkit.org/repository/webkit/trunk@249042 268f45cc-cd09-0410-ab3c-d52691b4dbfc

from the end of the `git log`, right? It's for:

(In reply to Milan Crha from comment #65)
> ... at commit ca8dc42af2fbce678b44fa83e61c4cb3ebbe23a0 ...

[Michael Catanzaro]

(In reply to Milan Crha from comment #53)
> Still the same problem with git checkout at commit
> aec39837fb5478153424253f0a1871c5f2b2d7aa, aka git-svn-id:
> http://svn.webkit.org/repository/webkit/trunk@249376
> 268f45cc-cd09-0410-ab3c-d52691b4dbfc. The extension page ID is 6, while the
> WebView's page ID is 5.
> 
> It's with the change from comment #43. This off-by-one should be fixed as
> well. I believe both sides should use the same page ID, no? It was the case
> till recently, at least.

I'm trying your test case with 2.25.4 (r248153) but I see 5 on both sides. It's possible this is a timing issue that just happens on your computer but not mine?

[Milan Crha]

(In reply to Michael Catanzaro from comment #69)
> It's possible this is a timing issue that just happens on your computer but
> not mine?

I do not think so. I see correct page IDs on both sides with r249042 too, even with r249174. The page ID thing regressed only (semi-)recently.

[Chris Dumez]

As of very recently (last week), the WebPageProxy ids and the WebPage ids are now completely unrelated. The WebPage id now also changes on every process swap. Note that Cocoa has code to support communication between the UIProcess and the injected bundle. The way I made it work is that the UIProcess side listens for IPC using the WebPageProxy id as IPC id. Because this id is constant throughout the lifetime of the view, this allows the UIProcess side object to receive IPC from all injected bundles (previous and new process when process swapping). On the WebProcess side, the object listens for IPC using the WebPage id as identifier, so we know which exact object to route it to. On the UIProcess side, we always send messages to the process currently associated with the WebPageProxy and use the WebPage id that is currently associated to the WebPageProxy. This allows multiple to 1 communication. The WebPage proxy always only talks to the committed process’ bundle, but can receive IPC from all injected bundles associated with the WebView.

[Carlos Garcia Campos]

(In reply to Chris Dumez from comment #71)
> As of very recently (last week), the WebPageProxy ids and the WebPage ids
> are now completely unrelated.

I haven't merged those commits in our stable branch, FWIW.

> The WebPage id now also changes on every
> process swap. Note that Cocoa has code to support communication between the
> UIProcess and the injected bundle. The way I made it work is that the
> UIProcess side listens for IPC using the WebPageProxy id as IPC id. Because
> this id is constant throughout the lifetime of the view, this allows the
> UIProcess side object to receive IPC from all injected bundles (previous and
> new process when process swapping). On the WebProcess side, the object
> listens for IPC using the WebPage id as identifier, so we know which exact
> object to route it to. On the UIProcess side, we always send messages to the
> process currently associated with the WebPageProxy and use the WebPage id
> that is currently associated to the WebPageProxy. This allows multiple to 1
> communication. The WebPage proxy always only talks to the committed process’
> bundle, but can receive IPC from all injected bundles associated with the
> WebView.

Thanks for your help Chris.

[Milan Crha]

I see, that might be r249275, aka bug #201233. I'm going to switch to webkit-2.16 branch to see whether it's unaffected.

[Milan Crha]

Err, I meant webkit-2.26 branch, of course.

[Michael Catanzaro]

(In reply to Chris Dumez from comment #71)
> As of very recently (last week), the WebPageProxy ids and the WebPage ids
> are now completely unrelated.

OK, that will break the world. :(

[Michael Catanzaro]

So webkit_web_page_get_id() will need to be reimplemented in some way that gets its ID from the WebPageProxy in the UI process, rather than its WebPage.

Ideally this would happen without sync IPC, but that might be hard. :/

[Chris Dumez]

(In reply to Michael Catanzaro from comment #76)
> So webkit_web_page_get_id() will need to be reimplemented in some way that
> gets its ID from the WebPageProxy in the UI process, rather than its WebPage.
> 
> Ideally this would happen without sync IPC, but that might be hard. :/

That’s easy, the WebPage already has a getter to gets the id of its WebPageProxy identifier. No IPC needed.

[Chris Dumez]

(In reply to Michael Catanzaro from comment #75)
> (In reply to Chris Dumez from comment #71)
> > As of very recently (last week), the WebPageProxy ids and the WebPage ids
> > are now completely unrelated.
> 
> OK, that will break the world. :(

I think I will need details to understand why. So far I have not heard of any breakage on the Cocoa side (admittedly it is still early) but we have very intrusive clients using injected bundles.

[Michael Catanzaro]

(In reply to Chris Dumez from comment #77)
> That’s easy, the WebPage already has a getter to gets the id of its
> WebPageProxy identifier. No IPC needed.

OK, then it should be no problem to fix.

(In reply to Chris Dumez from comment #78)
> I think I will need details to understand why. So far I have not heard of
> any breakage on the Cocoa side (admittedly it is still early) but we have
> very intrusive clients using injected bundles.

API users expect the WebKitWebPage's page ID to correspond to the WebKitWebView's page ID. It's the only way to associate a WebKitWebPage with its corresponding WebKitWebView, since they exist in separate processes.

This entire bug report is about problems caused when the page ID doesn't match like it's expected to. Evolution uses the ID to direct messages to the right web process. If it doesn't match, the message gets sent to nowhere. Or, the Epiphany example above shows that Epiphany uses it to filter out web process messages intended for other web views, so the web view only processes the messages intended for it.

[Chris Dumez]

(In reply to Michael Catanzaro from comment #79)
> (In reply to Chris Dumez from comment #77)
> > That’s easy, the WebPage already has a getter to gets the id of its
> > WebPageProxy identifier. No IPC needed.
> 
> OK, then it should be no problem to fix.
> 
> (In reply to Chris Dumez from comment #78)
> > I think I will need details to understand why. So far I have not heard of
> > any breakage on the Cocoa side (admittedly it is still early) but we have
> > very intrusive clients using injected bundles.
> 
> API users expect the WebKitWebPage's page ID to correspond to the
> WebKitWebView's page ID. It's the only way to associate a WebKitWebPage with
> its corresponding WebKitWebView, since they exist in separate processes.
> 
> This entire bug report is about problems caused when the page ID doesn't
> match like it's expected to. Evolution uses the ID to direct messages to the
> right web process. If it doesn't match, the message gets sent to nowhere.
> Or, the Epiphany example above shows that Epiphany uses it to filter out web
> process messages intended for other web views, so the web view only
> processes the messages intended for it.

The bug title says PSON but looking at the very initial report, it looks like the issue is that there was code that expected a single WebProcess for all WebViews. This does not really seem PSON related. I guess it is because we dropped the process limit api. That said, we still support single process mode, so I am not sure why evolution cannot use that.

[Michael Catanzaro]

Right, this isn't about PSON after all. I'll edit the title.

(In reply to Chris Dumez from comment #77)
> That’s easy, the WebPage already has a getter to gets the id of its
> WebPageProxy identifier. No IPC needed.

Hm, problem is the WebPageProxyIdentifer returned by WebPage::webPageProxyIdentifier is not going to match the Identifier returned by WebPageProxy::identifier, is it? That's what we need to match.

[Chris Dumez]

(In reply to Michael Catanzaro from comment #81)
> Right, this isn't about PSON after all. I'll edit the title.
> 
> (In reply to Chris Dumez from comment #77)
> > That’s easy, the WebPage already has a getter to gets the id of its
> > WebPageProxy identifier. No IPC needed.
> 
> Hm, problem is the WebPageProxyIdentifer returned by
> WebPage::webPageProxyIdentifier is not going to match the Identifier
> returned by WebPageProxy::identifier, is it? That's what we need to match.

Sure they will match. Why wouldn’t they?

[Chris Dumez]

(In reply to Chris Dumez from comment #82)
> (In reply to Michael Catanzaro from comment #81)
> > Right, this isn't about PSON after all. I'll edit the title.
> > 
> > (In reply to Chris Dumez from comment #77)
> > > That’s easy, the WebPage already has a getter to gets the id of its
> > > WebPageProxy identifier. No IPC needed.
> > 
> > Hm, problem is the WebPageProxyIdentifer returned by
> > WebPage::webPageProxyIdentifier is not going to match the Identifier
> > returned by WebPageProxy::identifier, is it? That's what we need to match.
> 
> Sure they will match. Why wouldn’t they?

A WebPageProxy can be associated with several WebPages throughout its lifetime but a given WebPage is always associated with a single WebPageProxy / view.

[Michael Catanzaro]

(In reply to Chris Dumez from comment #82)
> Sure they will match. Why wouldn’t they?

In that case, I think we can change webkit_web_page_get_id() to return the WebPageProxyIdentifier instead of the PageIdentifier, and then we should be good.

I see webkit_web_view_get_page_id() is already returning the WebPageProxyIdentifier, not the PageIdentifier, so that explains why the view doesn't return different page IDs after a process swap, like I was expecting it would. Nice.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #84)
> In that case, I think we can change webkit_web_page_get_id() to return the
> WebPageProxyIdentifier instead of the PageIdentifier, and then we should be
> good.

Yeah this works. Milan, please test:

diff --git a/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp b/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp
index c9204b23981..385dada2fc9 100644
--- a/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp
+++ b/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp
@@ -764,7 +764,7 @@ guint64 webkit_web_page_get_id(WebKitWebPage* webPage)
 {
     g_return_val_if_fail(WEBKIT_IS_WEB_PAGE(webPage), 0);
 
-    return webPage->priv->webPage->pageID().toUInt64();
+    return webPage->priv->webPage->webPageProxyIdentifier().toUInt64();
 }
 
 /**

That's not going to fully fix your problem, because you filed this bug before this page ID trouble was introduced. But it's step one.

[Michael Catanzaro]

(In reply to Chris Dumez from comment #80)
> The bug title says PSON but looking at the very initial report, it looks
> like the issue is that there was code that expected a single WebProcess for
> all WebViews. This does not really seem PSON related. I guess it is because
> we dropped the process limit api. That said, we still support single process
> mode, so I am not sure why evolution cannot use that.

P.S. The existing single process API is broken (see comment #12) and you told us not to add a non-broken version (in comment #30).

[Milan Crha]

(In reply to Michael Catanzaro from comment #85)
> Yeah this works. Milan, please test:

I cannot test it at the moment, I switched to the webkit-2.26 branch, where Chris' changes are not included. Compiling webkitgtk+ is kind of nightmare here, thus I will test it some time later. You can give it a try with Epiphany, when it comes to it, or with the provided test programs above.

> That's not going to fully fix your problem, because you filed this bug
> before this page ID trouble was introduced. But it's step one.

Right, it's not. Carlos gave me another patch to force single process behaviour with an environment variable, which fixes the initial problem for me.

diff --git a/Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp b/Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp
index 22a864591df..bf301edfe0b 100644
--- a/Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp
+++ b/Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp
@@ -348,6 +348,9 @@ ALLOW_DEPRECATED_DECLARATIONS_END
     } else if (!priv->localStorageDirectory.isNull())
         configuration.setLocalStorageDirectory(FileSystem::stringFromFileSystemRepresentation(priv->localStorageDirectory.data()));
 
+    const char* useSingleWebProcess = getenv("WEBKIT_USE_SINGLE_WEB_PROCESS");
+    if (useSingleWebProcess && strcmp(useSingleWebProcess, "0"))
+        configuration.setUsesSingleWebProcess(true);
     priv->processPool = WebProcessPool::create(configuration);
 
     if (!priv->websiteDataManager)

[Milan Crha]

(In reply to Milan Crha from comment #87)
> (In reply to Michael Catanzaro from comment #85)
> > Yeah this works. Milan, please test:
> 
> I cannot test it at the moment, I switched to the webkit-2.26 branch, where
> Chris' changes are not included. Compiling webkitgtk+ is kind of nightmare
> here, thus I will test it some time later.

Unless I made any error, it looks like with your patch from comment #85 the webkit_web_page_get_id() returns the same ID as webkit_web_view_get_page_id(), but webkit_web_extension_get_page() returns NULL for this ID.

[Michael Catanzaro]

(In reply to Milan Crha from comment #87)
> I cannot test it at the moment, I switched to the webkit-2.26 branch, where
> Chris' changes are not included. Compiling webkitgtk+ is kind of nightmare
> here, thus I will test it some time later. You can give it a try with
> Epiphany, when it comes to it, or with the provided test programs above.

I checked Epiphany before posting the patch. It works. (Epiphany does not use webkit_web_extension_get_page().) But Evolution has additional problems, right?

(In reply to Milan Crha from comment #88)
> Unless I made any error, it looks like with your patch from comment #85 the
> webkit_web_page_get_id() returns the same ID as
> webkit_web_view_get_page_id(), but webkit_web_extension_get_page() returns
> NULL for this ID.

Oops, but fortunately that's also easy to fix:

diff --git a/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp b/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp
index e306e3fef26..a614d654f64 100644
--- a/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp
+++ b/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp
@@ -224,7 +224,7 @@ WebKitWebPage* webkit_web_extension_get_page(WebKitWebExtension* extension, guin
     WebKitWebExtensionPrivate* priv = extension->priv;
     WebPageMap::const_iterator end = priv->pages.end();
     for (WebPageMap::const_iterator it = priv->pages.begin(); it != end; ++it)
-        if (it->key->pageID().toUInt64() == pageID)
+        if (it->key->webPageProxyIdentifier().toUInt64() == pageID)
             return it->value.get();
 
     return 0;

[Milan Crha]

(In reply to Michael Catanzaro from comment #89)
> But Evolution has additional problems, right?

Evolution has two problems:
1) it expects to have one WebKitWebProcess per one WebKitWebContext;
2) it relies on WebKitWebView::page-id match WebKitWebPage::id.

The first is this bug, the second is a regression after bug #201233. As it's too late in the release cycle an r249421 adds a workaround for 2.26 for the 1), giving more time to solve this in a better way - if on WebKitGTK+ side, Evolution side or both sides is to be figured out.

> Oops, but fortunately that's also easy to fix:

Okay, that fixes evolution for the master branch as well (the 2) is not part of the webkit-2.26 branch).

Looking into the changes:

> -        if (it->key->pageID().toUInt64() == pageID)
> +        if (it->key->webPageProxyIdentifier().toUInt64() == pageID)

basically replacing pageID() with webPageProxyIdentifier() when checking against 'pageID', would it not make more sense to flip the two in the original change, thus keep consistency in the arguments and underlying code? I'm pretty sure that somebody will consider this a bug when webPageProxyIdentifier() is compared with pageID, when there exists pageID() member, some time in the future. Maybe we should move the discussion to the original bug #201233 about this.

[Michael Catanzaro]

(In reply to Milan Crha from comment #90)
> Evolution has two problems:
> 1) it expects to have one WebKitWebProcess per one WebKitWebContext;
> 2) it relies on WebKitWebView::page-id match WebKitWebPage::id.
> 
> The first is this bug

Right but (1) is not actually a bug, it's just the new intended behavior since 2.25.1 and Evolution will need to change somehow.

> 2) it relies on WebKitWebView::page-id match WebKitWebPage::id.

Hopefully my two patches fix this.

(In reply to Milan Crha from comment #90)
> Looking into the changes:
> 
> > -        if (it->key->pageID().toUInt64() == pageID)
> > +        if (it->key->webPageProxyIdentifier().toUInt64() == pageID)
> 
> basically replacing pageID() with webPageProxyIdentifier() when checking
> against 'pageID', would it not make more sense to flip the two in the
> original change, thus keep consistency in the arguments and underlying code?
> I'm pretty sure that somebody will consider this a bug when
> webPageProxyIdentifier() is compared with pageID, when there exists pageID()
> member, some time in the future. Maybe we should move the discussion to the
> original bug #201233 about this.

Good point. I don't know.

If we use the pageID members instead of WebPageProxyIdentifier, then the same WebKitWebView will change page IDs, which could occur at unexpected times and break applications.

If we use WebPageProxyIdentifier instead of pageID, the WebKitWebView will only ever expose a single ID, which is nice, but multiple WebKitWebPages will have the same ID, which might also break applications.

I don't know which is least-risky or less-likely to cause problems in practice.

[Milan Crha]

(In reply to Michael Catanzaro from comment #91)
> If we use the pageID members instead of WebPageProxyIdentifier, then the
> same WebKitWebView will change page IDs, which could occur at unexpected
> times and break applications.
> 
> If we use WebPageProxyIdentifier instead of pageID, the WebKitWebView will
> only ever expose a single ID, which is nice, but multiple WebKitWebPages
> will have the same ID, which might also break applications.

From what I saw (see comment #41) there are up to four WebKitWebProcess-es running with the same page ID at the same time (with the svn trunk, not with webkit-2.26 branch).

[Michael Catanzaro]

Yeah....

I guess multiple WebKitWebPages with the same ID is almost certain to cause problems (e.g. imagine storing them in a map keyed by page ID), so I suppose we should do the opposite of what my patches propose, and change WebKitWebView to return pageID instead of its Identifier, rather than changing WebKitWebPage and WebKitWebExtension.

[Milan Crha]

(In reply to Milan Crha from comment #87)
> +    const char* useSingleWebProcess =
> getenv("WEBKIT_USE_SINGLE_WEB_PROCESS");
> +    if (useSingleWebProcess && strcmp(useSingleWebProcess, "0"))
> +        configuration.setUsesSingleWebProcess(true);

With Michaels concerns about backward compatibility, and the above change already landed (r249421), what about flipping the above test, thus the default will be to use the SingleWebProcess, thus no old application will need any change, but new applications will be able to enable the multi-web processes using the same variable? Something like this:

 -    if (useSingleWebProcess && strcmp(useSingleWebProcess, "0"))
 -        configuration.setUsesSingleWebProcess(true);
 +    configuration.setUsesSingleWebProcess(!useSingleWebProcess || strcmp(useSingleWebProcess, "0"))

[Carlos Garcia Campos]

(In reply to Milan Crha from comment #94)
> (In reply to Milan Crha from comment #87)
> > +    const char* useSingleWebProcess =
> > getenv("WEBKIT_USE_SINGLE_WEB_PROCESS");
> > +    if (useSingleWebProcess && strcmp(useSingleWebProcess, "0"))
> > +        configuration.setUsesSingleWebProcess(true);
> 
> With Michaels concerns about backward compatibility, and the above change
> already landed (r249421), what about flipping the above test, thus the
> default will be to use the SingleWebProcess, thus no old application will
> need any change, but new applications will be able to enable the multi-web
> processes using the same variable? Something like this:
> 
>  -    if (useSingleWebProcess && strcmp(useSingleWebProcess, "0"))
>  -        configuration.setUsesSingleWebProcess(true);
>  +    configuration.setUsesSingleWebProcess(!useSingleWebProcess ||
> strcmp(useSingleWebProcess, "0"))

That would break applications setting multi-process model, we want to encourage apps using single process model to adapt to multiprocess.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #84)
> (In reply to Chris Dumez from comment #82)
> > Sure they will match. Why wouldn’t they?
> 
> In that case, I think we can change webkit_web_page_get_id() to return the
> WebPageProxyIdentifier instead of the PageIdentifier, and then we should be
> good.
> 
> I see webkit_web_view_get_page_id() is already returning the
> WebPageProxyIdentifier, not the PageIdentifier, so that explains why the
> view doesn't return different page IDs after a process swap, like I was
> expecting it would. Nice.

That's a bug introduced in r249275. webkit_web_view_get_page_id() is expected to return the webPageID() not the identifier(), that would be webkit_web_view_get_id() that we don't really want to expose.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #89)
> (In reply to Milan Crha from comment #87)
> > I cannot test it at the moment, I switched to the webkit-2.26 branch, where
> > Chris' changes are not included. Compiling webkitgtk+ is kind of nightmare
> > here, thus I will test it some time later. You can give it a try with
> > Epiphany, when it comes to it, or with the provided test programs above.
> 
> I checked Epiphany before posting the patch. It works. (Epiphany does not
> use webkit_web_extension_get_page().) But Evolution has additional problems,
> right?
> 
> (In reply to Milan Crha from comment #88)
> > Unless I made any error, it looks like with your patch from comment #85 the
> > webkit_web_page_get_id() returns the same ID as
> > webkit_web_view_get_page_id(), but webkit_web_extension_get_page() returns
> > NULL for this ID.
> 
> Oops, but fortunately that's also easy to fix:
> 
> diff --git
> a/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp
> b/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp
> index e306e3fef26..a614d654f64 100644
> --- a/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp
> +++ b/Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp
> @@ -224,7 +224,7 @@ WebKitWebPage*
> webkit_web_extension_get_page(WebKitWebExtension* extension, guin
>      WebKitWebExtensionPrivate* priv = extension->priv;
>      WebPageMap::const_iterator end = priv->pages.end();
>      for (WebPageMap::const_iterator it = priv->pages.begin(); it != end;
> ++it)
> -        if (it->key->pageID().toUInt64() == pageID)
> +        if (it->key->webPageProxyIdentifier().toUInt64() == pageID)
>              return it->value.get();
>  
>      return 0;

I don't think this is correct, here we want the page identifier, which is what webkit_web_view_get_page_id() should return.

[Milan Crha]

(In reply to Carlos Garcia Campos from comment #95)
> That would break applications setting multi-process model, we want to
> encourage apps using single process model to adapt to multiprocess.

I understood there is no such setting, that's why the env variable had been added (otherwise I'd use it and turn it off using WebKitGTK+ API). While I can understand your concern for new applications, Michaels concern about backward compatibility is also strong. Yes, it'll postpone the problem only one major release forward, but it's better than breaking the world now, when things are too new. Not talking that multiprocess never worked out-of-the-box anyway (comment #14). Eventually, make the deprecation less cruel (r249419), though old applications would still need to use the deprecated API to get the old behaviour (mine concern, an ideal state, is to not need to change anything in legacy applications, no matter what WebKitGTK+ they'll run (not compile) against).

I do not know how this works in Debian, but for Fedora the major release (2.26) would usually go only to Fedora 31, but webkit2gtk3 maintainers update it also for Fedora 30 and Fedora 29. It's fine as long as the update doesn't change soname versions and doesn't break existing applications. You cannot expect all the applications in Fedora 30 and 29 to be updated due to changes in WebKitGTK+, neither with soname version, nor with core functionality. That's even not allowed, when it comes to it. Michael wants to be able to do the updates in all three Fedoras and in Debian - without custom patches, I believe.

[Carlos Garcia Campos]

(In reply to Milan Crha from comment #98)
> (In reply to Carlos Garcia Campos from comment #95)
> > That would break applications setting multi-process model, we want to
> > encourage apps using single process model to adapt to multiprocess.
> 
> I understood there is no such setting, that's why the env variable had been
> added (otherwise I'd use it and turn it off using WebKitGTK+ API). While I
> can understand your concern for new applications, Michaels concern about
> backward compatibility is also strong. Yes, it'll postpone the problem only
> one major release forward, but it's better than breaking the world now, when
> things are too new. Not talking that multiprocess never worked
> out-of-the-box anyway (comment #14). Eventually, make the deprecation less
> cruel (r249419), though old applications would still need to use the
> deprecated API to get the old behaviour (mine concern, an ideal state, is to
> not need to change anything in legacy applications, no matter what
> WebKitGTK+ they'll run (not compile) against).

There's webkit_web_context_set_process_model() that receives a WebKitProcessModel. WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS was the default, that's why you didn't have to set it explicitly, but it's now deprecated. Normally, in case of deprecation, the old behavior is preserved, but in this case the feature has been removed for security reasons. I know this can be considered an API break, though. So, let's figure out if there are more apps that depend on the single process model to see what to do. If it's only evo, it's already fixed and we only need to ensure that for the next release cycle evo can work in multiprocess model.

> I do not know how this works in Debian, but for Fedora the major release
> (2.26) would usually go only to Fedora 31, but webkit2gtk3 maintainers
> update it also for Fedora 30 and Fedora 29. It's fine as long as the update
> doesn't change soname versions and doesn't break existing applications. You
> cannot expect all the applications in Fedora 30 and 29 to be updated due to
> changes in WebKitGTK+, neither with soname version, nor with core
> functionality. That's even not allowed, when it comes to it. Michael wants
> to be able to do the updates in all three Fedoras and in Debian - without
> custom patches, I believe.

Right, that's why we first need to figure out what other apps are broken.

[Carlos Garcia Campos]

btw, I think we should move the discussion about single process model deprecation somewhere else, and keep this bug for the page id mismatch in current trunk. Bug #200967 already covers the page-created issue when loading history items from the page cache.

[Milan Crha]

For the record, I just reopened the Evolution bug (comment #0), which I'll use for changes on the Evolution side to work properly with multiple WebKitWebProcess-es being spawn for a single WebKitWebView.

[Carlos Garcia Campos]

I think we can close this. Bug #201642 handles the new API to monitor page id changes and bug #200967 will make PSON optional and disabled by default to keep backwards compatibility. Feel free to reopen if there are still problems with the ids.