Bug 200967 – [GTK] Make PSON optional

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

Created attachment 376848 [details]
log indicating the failure path

for the UI process to talk to the correct webprocess I rely on the
page-created signal in the webprocess. On this signal I can inform
the UI process that this webprocess is handling this particular pageid.

However the page-created signal fails to trigger when navigation is to
(or maybe from) a page which is served by the pagecache.

To test this I have a dummy extension which prints an INITIALIZED message
on webkit_web_extension_initialize and a g_messgage on the
page-created-callback. I turn on

WEBKIT_DEBUG=Process,ProcessSwapping,ProcessSuspension


The attachment shows a selection of the messages I from 

1. loading /tmp/1.html which has a single link to trac.webkit.org,
2. and then navigating to the link
3. and then navigating back

step 3 is loads the page from cache.
the new process is created 11867 - BUT ProcessSwap tells process 11830
to handle back navigation to (/tmp/1.html)
but a page-created callback is not signalled for this step.
my UI process thinks that pageID=6 is handled by process 11854 (from step 2)
and directs it to access its document. This fails as there is no document.
and that process crashes.

I hope this description is sufficiently clear, I'd be glad to 
supply more details as directed.

[Carlos Garcia Campos]

Maybe you are confused by pre-warmed process?

[Chris Dumez]

(In reply to Carlos Garcia Campos from comment #1)
> Maybe you are confused by pre-warmed process?

I am not sure this has anything to do with pre-warmed processes. Based on the description, the issue seems to be related to PageCache and how it is implemented with Process-Swap-On-Navigation. When navigating back here, we swap back to the original process and leverage the PageCache entry in that process (this was important to keep PageCache functionality when we implemented process-swapping). The issue in question here seems specific to GTK here so I am not sure I will be able to help much. I am not familiar with webkit_web_extension_initialize / g_message. It seems like the developer wants to know we swapped back to another process and is currently not getting notified when the process is a "suspended" process (i.e. process kept around simply for PageCache purposes). I am happy to answer process-swap related questions if it helps GTK-port developers fix this issue.

[Carlos Garcia Campos]

(In reply to Chris Dumez from comment #2)
> (In reply to Carlos Garcia Campos from comment #1)
> > Maybe you are confused by pre-warmed process?
> 
> I am not sure this has anything to do with pre-warmed processes. Based on
> the description, the issue seems to be related to PageCache and how it is
> implemented with Process-Swap-On-Navigation. When navigating back here, we
> swap back to the original process and leverage the PageCache entry in that
> process (this was important to keep PageCache functionality when we
> implemented process-swapping). The issue in question here seems specific to
> GTK here so I am not sure I will be able to help much. I am not familiar
> with webkit_web_extension_initialize / g_message. It seems like the
> developer wants to know we swapped back to another process and is currently
> not getting notified when the process is a "suspended" process (i.e. process
> kept around simply for PageCache purposes). I am happy to answer
> process-swap related questions if it helps GTK-port developers fix this
> issue.

Thanks for your help Chris. I don't think this is specific to GTK, webkit_web_extension_initialize is equivalent to WKBundleInitialize and page-created signal is equivalent to InjectedBundle::Client::didCreatePage()

[Chris Dumez]

(In reply to Carlos Garcia Campos from comment #3)
> (In reply to Chris Dumez from comment #2)
> > (In reply to Carlos Garcia Campos from comment #1)
> > > Maybe you are confused by pre-warmed process?
> > 
> > I am not sure this has anything to do with pre-warmed processes. Based on
> > the description, the issue seems to be related to PageCache and how it is
> > implemented with Process-Swap-On-Navigation. When navigating back here, we
> > swap back to the original process and leverage the PageCache entry in that
> > process (this was important to keep PageCache functionality when we
> > implemented process-swapping). The issue in question here seems specific to
> > GTK here so I am not sure I will be able to help much. I am not familiar
> > with webkit_web_extension_initialize / g_message. It seems like the
> > developer wants to know we swapped back to another process and is currently
> > not getting notified when the process is a "suspended" process (i.e. process
> > kept around simply for PageCache purposes). I am happy to answer
> > process-swap related questions if it helps GTK-port developers fix this
> > issue.
> 
> Thanks for your help Chris. I don't think this is specific to GTK,
> webkit_web_extension_initialize is equivalent to WKBundleInitialize and
> page-created signal is equivalent to InjectedBundle::Client::didCreatePage()

Well, our injected bundle API is not public :)
Injected bundles unfortunately often do not play nice with process swapping. Some work may be needed on injected bundle side to help figure out that we're back to using this process (maybe via load notifications?).

Without understanding any of the complexity of process swapping or suspension
I tried commenting out 2 lines in UIProxy/WebPageProxy.cpp
WebPageProxy::suspendCurrentPageIfPossible

-    if (fromItem && m_preferences->usesPageCache())
-        fromItem->setSuspendedPage(suspendedPage.get());

and this seems to avoid the problem I was reporting.

It would be nice to have a description of ProcessSwapping and suspension on
say blog.webkit.org with some design notes.

[On another note I notice a commit introduced WebKit/UIProcess/WebProcessProxy.h:    void didCreateWebPageInProcess(WebCore::PageIdentifier); - 
which does not seem to be defined or used anywhere.]

[Chris Dumez]

(In reply to enometh from comment #5)
> Without understanding any of the complexity of process swapping or suspension
> I tried commenting out 2 lines in UIProxy/WebPageProxy.cpp
> WebPageProxy::suspendCurrentPageIfPossible
> 
> -    if (fromItem && m_preferences->usesPageCache())
> -        fromItem->setSuspendedPage(suspendedPage.get());
> 
> and this seems to avoid the problem I was reporting.

If GTK port has an API to disable PageCache, it would also work around your issue too and would not require a code change.

> 
> It would be nice to have a description of ProcessSwapping and suspension on
> say blog.webkit.org with some design notes.
> 
> [On another note I notice a commit introduced
> WebKit/UIProcess/WebProcessProxy.h:    void
> didCreateWebPageInProcess(WebCore::PageIdentifier); - 
> which does not seem to be defined or used anywhere.]

[Adrian Perez]

(In reply to Chris Dumez from comment #6)
> (In reply to enometh from comment #5)
> > Without understanding any of the complexity of process swapping or suspension
> > I tried commenting out 2 lines in UIProxy/WebPageProxy.cpp
> > WebPageProxy::suspendCurrentPageIfPossible
> > 
> > -    if (fromItem && m_preferences->usesPageCache())
> > -        fromItem->setSuspendedPage(suspendedPage.get());
> > 
> > and this seems to avoid the problem I was reporting.
> 
> If GTK port has an API to disable PageCache, it would also work around your
> issue too and would not require a code change.

IIUC this would be achieved using webkit_web_context_set_model(),
passing WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER.

[Michael Catanzaro]

Page cache is too important for back/forward performance to give up, though.

I'd try emitting fake page-created events when the page is unsuspended. It'd be weird for page-created to occur multiple times for the same page, but I guess unlikely to break anything that's not already broken.

[Carlos Garcia Campos]

Instead of disabling page cache we could disable the page suspension in 2.26 to ensure applications can still working without changes. If I understood the code correctly, suspended pages are used only for history navigation. Pages are suspended for 10 seconds, so that if you navigate back to a page that is still suspended the same process and page is used. I think we can live without that optimization for 2.26. I don't see how to provide backwards compatibility here otherwise, because emitting page-created when a page is unsuspended wouldn't work either, because apps assume it's not emitted twice for the same WebKitWebPage. In the case of ephy we would want to emit the PageCreated DBus signal, but not the rest of the things we do in the page created callback. 

In the case of trunk, I think we should add a new signal, and update applications to handle it. I'm still not sure how we could expose it in the api, though.

 - We could add page-attached that would also be emitted after page-created. Then we could split current page-created, and use page-attached to notify the UI process that the injected bundle connection should be used for that page. Apps would use this instead of page created to set the current injected bundle proxy.

 - Or we could expose is-suspended as a property than can be monitored. Apps would need to keep a list of injected bundle proxies for the same web view. When the page is unsuspended, the UI process is notified and the web view sets the unsuspended injected bundle proxy as the current one.

Again, for trunk I don't see any solution that doesn't require to update the applications. We should probably encourage apps not to use custom IPC for the communication from UI process to injected bundle and use webkit_web_view_run_javascript() instead.

[Carlos Garcia Campos]

hmm, it's not only suspended pages, there's also the process cache.

[Carlos Garcia Campos]

Disabling the process cache might have a bigger impact in performance when navigating to history items.

[Carlos Garcia Campos]

The other alternative would be to disable PSON entirely in 2.26.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #12)
> The other alternative would be to disable PSON entirely in 2.26.

I'd do it, but only if we also revert enough of Chris's work on the 2.26 branch to bring back single-process model for 2.26. Then we can have a smooth upgrade to 2.26 so this is no longer an emergency, and continue to discuss how to handle this situation for 2.28.

We should still try to sort this out now, but it'll be a lot easier to solve this at the very beginning of the next release cycle rather than the end of the current one.

We might need to seriously consider an API version bump, though hopefully it can be avoided.

(In reply to Carlos Garcia Campos from comment #10)
> hmm, it's not only suspended pages, there's also the process cache.

What's the impact of process cache on API users?

[Michael Catanzaro]

(In reply to enometh from comment #0)
> for the UI process to talk to the correct webprocess I rely on the
> page-created signal in the webprocess. On this signal I can inform
> the UI process that this webprocess is handling this particular pageid.

I'd be helpful to know app is broken, so we know what to test.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #14)
> (In reply to enometh from comment #0)
> > for the UI process to talk to the correct webprocess I rely on the
> > page-created signal in the webprocess. On this signal I can inform
> > the UI process that this webprocess is handling this particular pageid.
> 
> I'd be helpful to know app is broken, so we know what to test.

Epiphany is also affected by this, and it's not easy to fix/workaround (disabling page cache, which is not an option for epiphany). That's why I think maybe it's easier to disable PSON in 2.26.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #13)
> (In reply to Carlos Garcia Campos from comment #12)
> > The other alternative would be to disable PSON entirely in 2.26.
> 
> I'd do it, but only if we also revert enough of Chris's work on the 2.26
> branch to bring back single-process model for 2.26. Then we can have a
> smooth upgrade to 2.26 so this is no longer an emergency, and continue to
> discuss how to handle this situation for 2.28.

Unfortunately that's not that easy and we already have a workaround. I expect more apps to be broken because of this bug than apps depending on the single process model. I know it would be ideal if apps didn't have to do anything, but we have a very simple workaround for the single process model case. This bug is more complicated and the only way I see right now to not break apps is disabling PSON.

> We should still try to sort this out now, but it'll be a lot easier to solve
> this at the very beginning of the next release cycle rather than the end of
> the current one.

It's a pity that very few people try early unstable releases, and I didn't notice any issue because I don't use the remember password feature of ephy.

> We might need to seriously consider an API version bump, though hopefully it
> can be avoided.
> 
> (In reply to Carlos Garcia Campos from comment #10)
> > hmm, it's not only suspended pages, there's also the process cache.
> 
> What's the impact of process cache on API users?

I guess it will be slower to load pages when switching processes due to history navigation (which is always expected to be fast).

[Carlos Garcia Campos]

I've realized that we don't really need new API for the cases when page created is not emitted, but we need to handle it in the applications. Since it's not really possible to enable PSON without breaking backwards compatibility, I've decided to make it optional, so that only apps that are ready can enable it. For this particular case, the application needs to keep a map of web process connections to web views, using the page created signal the same way, but leaving connections in the map until they are closed. This way, when a previous process is reused we already have the connection in the map. Once patch in bug #201642 lands, we can monitor the page id changes to create the proxy for the current web process. You can see how I implemented this approach in the unit tests. I'm going to use this bug to add the new API to enable PSON. Note that this property will be removed once we bump the API version.

[Carlos Garcia Campos]

Created attachment 379885 [details]
Patch

[Adrian Perez]

Comment on attachment 379885 [details]
Patch

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

Patch r=me informally, with a small nit about documentation.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:457
> +     * Whether swap Web processes on cross-site navigations is enabled.

I think this deserves a short paragraph explaining intuitively what
the setting entails, and why it is a good thing. How about something
like this:

  When enabled, pages from each security origin will be handled by
  their own separate renderer processes, which are started (and
  terminated) on demand as the user navigates across different
  domains. This is an important security measure which helps prevent
  websites stealing data from other visited pages.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:468
> +            FALSE,

While it would be much better from a security POV to set this is to TRUE,
and let applications opt-out when needed, we'll have to live with PSON
disabled by default to please the Backwards Compatibility Demigods ¯\_(ツ)_/¯

[Carlos Garcia Campos]

Committed r250646: <https://trac.webkit.org/changeset/250646>