Certain WebProcesses should opt-out of the freezer

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Certain WebProcesses should opt-out of the freezer

These include:
-Prewarmed web processes
-Processes in the per-domain-process cache
-Processes in the back/forward cache list

[Brady Eidson]

(In reply to Brady Eidson from comment #0)
> Certain WebProcesses should opt-out of the freezer
> 
> These include:
> -Prewarmed web processes
> -Processes in the per-domain-process cache
> -Processes in the back/forward cache list

Actually that last one is not true. It's "processes that *only host* pages in the back/forward cache list" and such processes should also already be in the per-domain-process-cache.

Yay.

[Chris Dumez]

(In reply to Brady Eidson from comment #1)
> (In reply to Brady Eidson from comment #0)
> > Certain WebProcesses should opt-out of the freezer
> > 
> > These include:
> > -Prewarmed web processes
> > -Processes in the per-domain-process cache
> > -Processes in the back/forward cache list
> 
> Actually that last one is not true. It's "processes that *only host* pages
> in the back/forward cache list" and such processes should also already be in
> the per-domain-process-cache.
> 
> Yay.

Processes with suspended pages are NOT in the WebProcess cache, even when they have no WebPage on the WebProcess side. Also note that the WebProcess cache is currently not enabled on iOS.

[Brady Eidson]

(In reply to Chris Dumez from comment #2)
> (In reply to Brady Eidson from comment #1)
> > (In reply to Brady Eidson from comment #0)
> > > Certain WebProcesses should opt-out of the freezer
> > > 
> > > These include:
> > > -Prewarmed web processes
> > > -Processes in the per-domain-process cache
> > > -Processes in the back/forward cache list
> > 
> > Actually that last one is not true. It's "processes that *only host* pages
> > in the back/forward cache list" and such processes should also already be in
> > the per-domain-process-cache.
> > 
> > Yay.
> 
> Processes with suspended pages are NOT in the WebProcess cache, even when
> they have no WebPage on the WebProcess side.

This is confusing. You're saying a process that is hosting a suspended webkit.org page is not in the cache and therefore will not be chosen to host other webkit.org pages?

Why not?

> Also note that the WebProcess cache is currently not enabled on iOS.

Yah, I know, and I also know it could be (and even probably will be) some day :)

[Chris Dumez]

(In reply to Brady Eidson from comment #3)
> (In reply to Chris Dumez from comment #2)
> > (In reply to Brady Eidson from comment #1)
> > > (In reply to Brady Eidson from comment #0)
> > > > Certain WebProcesses should opt-out of the freezer
> > > > 
> > > > These include:
> > > > -Prewarmed web processes
> > > > -Processes in the per-domain-process cache
> > > > -Processes in the back/forward cache list
> > > 
> > > Actually that last one is not true. It's "processes that *only host* pages
> > > in the back/forward cache list" and such processes should also already be in
> > > the per-domain-process-cache.
> > > 
> > > Yay.
> > 
> > Processes with suspended pages are NOT in the WebProcess cache, even when
> > they have no WebPage on the WebProcess side.
> 
> This is confusing. You're saying a process that is hosting a suspended
> webkit.org page is not in the cache and therefore will not be chosen to host
> other webkit.org pages?
> 
> Why not?

A suspended page processes may get used for navigations if their domain matches but they are not in the WebProcessCache. We check both suspended pages and the WebProcessCache to find a suitable process.
In the WebProcessCache, we currently only put processes that were about to shutdown (i.e. because they have no pages / suspendedPages).

> 
> > Also note that the WebProcess cache is currently not enabled on iOS.
> 
> Yah, I know, and I also know it could be (and even probably will be) some
> day :)

Maybe ^^

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Brady Eidson from comment #3)
> > (In reply to Chris Dumez from comment #2)
> > > (In reply to Brady Eidson from comment #1)
> > > > (In reply to Brady Eidson from comment #0)
> > > > > Certain WebProcesses should opt-out of the freezer
> > > > > 
> > > > > These include:
> > > > > -Prewarmed web processes
> > > > > -Processes in the per-domain-process cache
> > > > > -Processes in the back/forward cache list
> > > > 
> > > > Actually that last one is not true. It's "processes that *only host* pages
> > > > in the back/forward cache list" and such processes should also already be in
> > > > the per-domain-process-cache.
> > > > 
> > > > Yay.
> > > 
> > > Processes with suspended pages are NOT in the WebProcess cache, even when
> > > they have no WebPage on the WebProcess side.
> > 
> > This is confusing. You're saying a process that is hosting a suspended
> > webkit.org page is not in the cache and therefore will not be chosen to host
> > other webkit.org pages?
> > 
> > Why not?
> 
> A suspended page processes may get used for navigations if their domain
> matches but they are not in the WebProcessCache. We check both suspended
> pages and the WebProcessCache to find a suitable process.
> In the WebProcessCache, we currently only put processes that were about to
> shutdown (i.e. because they have no pages / suspendedPages).


Note that when we decide to use a suspended page's process for a forward navigation, we often have to destroy the suspended page, this is a destructive optimization.

It is not exactly the same thing as process caching IMO.

[Chris Dumez]

(In reply to Brady Eidson from comment #1)
> (In reply to Brady Eidson from comment #0)
> > Certain WebProcesses should opt-out of the freezer
> > 
> > These include:
> > -Prewarmed web processes
> > -Processes in the per-domain-process cache
> > -Processes in the back/forward cache list
> 
> Actually that last one is not true. It's "processes that *only host* pages
> in the back/forward cache list" and such processes should also already be in
> the per-domain-process-cache.
> 
> Yay.

Seems like we could generalize to:
if (m_pageMap.isEmpty() && m_provisionalPages.isEmpty() && !isServiceWorkerProcess())
  // Then no need for the freezer.

[Brady Eidson]

Created attachment 365641 [details]
Patch

[Brady Eidson]

(In reply to Chris Dumez from comment #6)
> (In reply to Brady Eidson from comment #1)
> > (In reply to Brady Eidson from comment #0)
> > > Certain WebProcesses should opt-out of the freezer
> > > 
> > > These include:
> > > -Prewarmed web processes
> > > -Processes in the per-domain-process cache
> > > -Processes in the back/forward cache list
> > 
> > Actually that last one is not true. It's "processes that *only host* pages
> > in the back/forward cache list" and such processes should also already be in
> > the per-domain-process-cache.
> > 
> > Yay.
> 
> Seems like we could generalize to:
> if (m_pageMap.isEmpty() && m_provisionalPages.isEmpty() &&
> !isServiceWorkerProcess())
>   // Then no need for the freezer.

I don't think the provisional page check is quite right.

Also, serviceWorkerProcesses host pages (right?), so that explicit check is also needed.

[Brady Eidson]

(In reply to Brady Eidson from comment #8)
> 
> Also, serviceWorkerProcesses host pages (right?), so that explicit check is
> also needed.

I meant also *not* needed.

[Chris Dumez]

(In reply to Brady Eidson from comment #8)
> (In reply to Chris Dumez from comment #6)
> > (In reply to Brady Eidson from comment #1)
> > > (In reply to Brady Eidson from comment #0)
> > > > Certain WebProcesses should opt-out of the freezer
> > > > 
> > > > These include:
> > > > -Prewarmed web processes
> > > > -Processes in the per-domain-process cache
> > > > -Processes in the back/forward cache list
> > > 
> > > Actually that last one is not true. It's "processes that *only host* pages
> > > in the back/forward cache list" and such processes should also already be in
> > > the per-domain-process-cache.
> > > 
> > > Yay.
> > 
> > Seems like we could generalize to:
> > if (m_pageMap.isEmpty() && m_provisionalPages.isEmpty() &&
> > !isServiceWorkerProcess())
> >   // Then no need for the freezer.
> 
> I don't think the provisional page check is quite right.

Can you elaborate? :)

> 
> Also, serviceWorkerProcesses host pages (right?), so that explicit check is
> also needed.

serviceWorkerProcesses host pages? This is news to me but maybe Youenn added a dummy page in there to help with the networking?

[Brady Eidson]

(In reply to Chris Dumez from comment #10)
> (In reply to Brady Eidson from comment #8)
> > (In reply to Chris Dumez from comment #6)
> > > (In reply to Brady Eidson from comment #1)
> > > > (In reply to Brady Eidson from comment #0)
> > > > > Certain WebProcesses should opt-out of the freezer
> > > > > 
> > > > > These include:
> > > > > -Prewarmed web processes
> > > > > -Processes in the per-domain-process cache
> > > > > -Processes in the back/forward cache list
> > > > 
> > > > Actually that last one is not true. It's "processes that *only host* pages
> > > > in the back/forward cache list" and such processes should also already be in
> > > > the per-domain-process-cache.
> > > > 
> > > > Yay.
> > > 
> > > Seems like we could generalize to:
> > > if (m_pageMap.isEmpty() && m_provisionalPages.isEmpty() &&
> > > !isServiceWorkerProcess())
> > >   // Then no need for the freezer.
> > 
> > I don't think the provisional page check is quite right.
> 
> Can you elaborate? :)

If a process *only* has a provisional page in it (e.g. it fails these other checks) there's no value to freezing it.

> > 
> > Also, serviceWorkerProcesses host pages (right?), so that explicit check is
> > also needed.
> 
> serviceWorkerProcesses host pages? This is news to me but maybe Youenn added
> a dummy page in there to help with the networking?

It's got the dummy page id for messaging, but the dummy page itself has been removed. I'll add the isServiceWorker check

[Brady Eidson]

Created attachment 365647 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 365647 [details]
Patch

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

Assuming sufficient testing, r=me

> Source/WebKit/UIProcess/WebProcessProxy.cpp:201
> +void WebProcessProxy::validateFreezerStatus()
> +{
> +#if PLATFORM(IOS_FAMILY)
> +    bool value = !m_isPrewarmed && !m_isInProcessCache && !m_pageMap.isEmpty() && !isServiceWorkerProcess();
> +    if (m_currentIsFreezableValue != WTF::nullopt && m_currentIsFreezableValue == value)
> +        return;
> +
> +    m_currentIsFreezableValue = value;
> +    send(Messages::WebProcess::SetFreezable(value), 0);
> +#endif
> +}

Often, we do thinks like this by having platform-specific methods in a separate platform-specific file, rather than #ifefing whole method bodies. (Not a blocker for landing though.) Also, style nit: "value" is a very generic name for a variable. Might be better named isFreezable.

> Source/WebKit/WebProcess/WebProcess.cpp:1879
> +#if PLATFORM(IOS_FAMILY)
> +    auto result = memorystatus_control(MEMORYSTATUS_CMD_SET_PROCESS_IS_FREEZABLE, getpid(), freezable ? 1 : 0, nullptr, 0);
> +    ASSERT_UNUSED(result, !result);
> +#endif

ditto the previous comment about fully ifdef'd method bodies,

[Chris Dumez]

Comment on attachment 365647 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessProxy.h:139
> +    class WebPageProxyMap {

We do not really need this new Map type. Calls to validateFreezerStatus can simply be added to:
WebProcessProxy::addExistingWebPage() and WebProcessProxy::removeWebPage(). Those are already our choke points for adding / removing pages.

[WebKit Commit Bot]

Comment on attachment 365647 [details]
Patch

Clearing flags on attachment: 365647

Committed r243357: <https://trac.webkit.org/changeset/243357>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49136842>

[Brady Eidson]

(In reply to Chris Dumez from comment #14)
> Comment on attachment 365647 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365647&action=review
> 
> > Source/WebKit/UIProcess/WebProcessProxy.h:139
> > +    class WebPageProxyMap {
> 
> We do not really need this new Map type. Calls to validateFreezerStatus can
> simply be added to:
> WebProcessProxy::addExistingWebPage() and WebProcessProxy::removeWebPage().
> Those are already our choke points for adding / removing pages.

Relying on users of maps as chokepoints is fragile.

What you say is true today but there’s no guarantee it will remain so.

I’ve had to implement this pattern twice recently and have a refactor in mind coming up soon to obviate the need for this custom class while still being truly reliable going forward

[Chris Dumez]

(In reply to Brady Eidson from comment #18)
> (In reply to Chris Dumez from comment #14)
> > Comment on attachment 365647 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=365647&action=review
> > 
> > > Source/WebKit/UIProcess/WebProcessProxy.h:139
> > > +    class WebPageProxyMap {
> > 
> > We do not really need this new Map type. Calls to validateFreezerStatus can
> > simply be added to:
> > WebProcessProxy::addExistingWebPage() and WebProcessProxy::removeWebPage().
> > Those are already our choke points for adding / removing pages.
> 
> Relying on users of maps as chokepoints is fragile.
> 
> What you say is true today but there’s no guarantee it will remain so.
> 
> I’ve had to implement this pattern twice recently and have a refactor in
> mind coming up soon to obviate the need for this custom class while still
> being truly reliable going forward

If you are going to add so much code, than at least move maybeShutDown() in there too, no? I’d argue it is more important than opting out of the freezer.

[Chris Dumez]

Comment on attachment 365647 [details]
Patch

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

> Source/WebKit/WebProcess/WebProcess.messages.in:165
> +    SetFreezable(bool freezable)

This looks fragile. How do we know for sure the WebProcess will receive this IPC *before* it gets suspended?

[Brady Eidson]

(In reply to Chris Dumez from comment #19)
> (In reply to Brady Eidson from comment #18)
> > (In reply to Chris Dumez from comment #14)
> > > Comment on attachment 365647 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=365647&action=review
> > > 
> > > > Source/WebKit/UIProcess/WebProcessProxy.h:139
> > > > +    class WebPageProxyMap {
> > > 
> > > We do not really need this new Map type. Calls to validateFreezerStatus can
> > > simply be added to:
> > > WebProcessProxy::addExistingWebPage() and WebProcessProxy::removeWebPage().
> > > Those are already our choke points for adding / removing pages.
> > 
> > Relying on users of maps as chokepoints is fragile.
> > 
> > What you say is true today but there’s no guarantee it will remain so.
> > 
> > I’ve had to implement this pattern twice recently and have a refactor in
> > mind coming up soon to obviate the need for this custom class while still
> > being truly reliable going forward
> 
> If you are going to add so much code, than at least move maybeShutDown() in
> there too, no? I’d argue it is more important than opting out of the freezer.

Would be happy to do that.

[Ryan Haddad]

Called this out on IRC, but this change caused iOS Simulator Debug tests to exit early due this assertion failure:

    ASSERTION FAILED: !result
    /Volumes/Data/slave/ios-simulator-12-debug/build/Source/WebKit/WebProcess/WebProcess.cpp(1882) : void WebKit::WebProcess::setFreezable(bool)
    1   0x5d3cdbe39 WTFCrash
    2   0x5c8099d8b WTFCrashWithInfo(int, char const*, char const*, int)
    3   0x5c8e48d6c WebKit::WebProcess::setFreezable(bool)
    4   0x5c936a842 void IPC::callMemberFunctionImpl<WebKit::WebProcess, void (WebKit::WebProcess::*)(bool), std::__1::tuple<bool>, 0ul>(WebKit::WebProcess*, void (WebKit::WebProcess::*)(bool), std::__1::tuple<bool>&&, std::__1::integer_sequence<unsigned long, 0ul>)
    5   0x5c936a790 void IPC::callMemberFunction<WebKit::WebProcess, void (WebKit::WebProcess::*)(bool), std::__1::tuple<bool>, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<bool>&&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(bool))
    6   0x5c93677ae void IPC::handleMessage<Messages::WebProcess::SetFreezable, WebKit::WebProcess, void (WebKit::WebProcess::*)(bool)>(IPC::Decoder&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(bool))
    7   0x5c93616ce WebKit::WebProcess::didReceiveWebProcessMessage(IPC::Connection&, IPC::Decoder&)
    8   0x5c8e3de6b WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
    9   0x5c80e201c IPC::Connection::dispatchMessage(IPC::Decoder&)
    10  0x5c80d4601 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
    11  0x5c80e2de7 IPC::Connection::dispatchOneIncomingMessage()
    12  0x5c8103a78 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()()
    13  0x5c8103989 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call()
    14  0x5d3d0635d WTF::Function<void ()>::operator()() const
    15  0x5d3d65d0d WTF::RunLoop::performWork()
    16  0x5d3d665b4 WTF::RunLoop::performWork(void*)
    17  0x5ce455721 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
    18  0x5ce454f93 __CFRunLoopDoSources0
    19  0x5ce44f63f __CFRunLoopRun
    20  0x5ce44ee11 CFRunLoopRunSpecific
    21  0x101eb4322 -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
    22  0x101eb4492 -[NSRunLoop(NSRunLoop) run]
    23  0x5cfbb2812 _xpc_objc_main
    24  0x5cfbb4cbd xpc_main
    25  0x5c8599017 WebKit::XPCServiceMain(int, char const**)
    26  0x5c84ac0eb WKXPCServiceMain
    27  0x101dfda8e main
    28  0x5cf8e7575 start
    29  0x1

https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/builds/2938