Bug 216131 – Webpages flash when getting closed

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 407904 [details]
Patch

[Sihui Liu]

Created attachment 407918 [details]
Patch

[Sihui Liu]

Looks like the bots are Okay, not sure if this would cause other issues. Put it up for review to collect some feedbacks on this approach.

[Tim Horton]

Comment on attachment 407918 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:490
> +void WebPageProxy::deferSendingCloseMessage()

Gonna need Chris or someone to weigh in on the actual change, but...

> Source/WebKit/UIProcess/WebPageProxy.cpp:1126
> +#if PLATFORM(COCOA)

...I don't think this is the kind of behavior that should differ between platforms. Can we just do it globally?

[Tim Horton]

Comment on attachment 407918 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        Defer sending close message to next runloop cycle.

Also, this needs way more words :D

[Sihui Liu]

<rdar://problem/62264106>

[Sihui Liu]

Created attachment 408012 [details]
Patch

[Sihui Liu]

Created attachment 408014 [details]
Patch

[Geoffrey Garen]

Comment on attachment 408014 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1070
> +void WebPageProxy::deferSendingCloseMessage()

I would name this "sendCloseMessageSoon". But also -- let's just inline this into the caller, since it's only a little code, and it has only one use.

> Source/WebKit/UIProcess/WebPageProxy.cpp:1072
> +    RunLoop::current().dispatch([destinationID = messageSenderDestinationID(), protectedProcess = m_process.copyRef()] {

I think it's worth a comment here. Something like: "Delay the close message until after the UI process has a chance to commit the current RunLoop's CA transaction; otherwise, the WebContent process can race and commit a flash of white, even after the application has removed the WebView from the view hierarchy."

[Sihui Liu]

Created attachment 408023 [details]
Patch

[Sihui Liu]

Delaying one runloop cycle may not be enough, ideally the page should be closed after transaction is committed, which can be many cycles later.

[Tim Horton]

(In reply to Sihui Liu from comment #11)
> Delaying one runloop cycle may not be enough, ideally the page should be
> closed after transaction is committed, which can be many cycles later.

How‽ It can be at most the next runloop.

[Sihui Liu]

(In reply to Tim Horton from comment #12)
> (In reply to Sihui Liu from comment #11)
> > Delaying one runloop cycle may not be enough, ideally the page should be
> > closed after transaction is committed, which can be many cycles later.
> 
> How‽ It can be at most the next runloop.

Discovered this when debugging https://bugs.webkit.org/show_bug.cgi?id=216258. The transaction is not committed until a few cycles later. IIUC, AppKit can disable the CA commit observer and uses their own commit observer, which is time-based. I am not sure if it is at most one cycle if the page close is caused by object getting released though :|

[Geoffrey Garen]

(In reply to Tim Horton from comment #12)
> (In reply to Sihui Liu from comment #11)
> > Delaying one runloop cycle may not be enough, ideally the page should be
> > closed after transaction is committed, which can be many cycles later.
> 
> How‽ It can be at most the next runloop.

Amazing. I (incorrectly) thought the same!

[Peng Liu]

Comment on attachment 408023 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1128
> +    // WebView from the view hierachy.

s/hierachy/hierarchy/

[Chris Dumez]

Comment on attachment 408023 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1133
>      m_process->removeWebPage(*this, WebProcessProxy::EndsUsingDataStore::Yes);

As soon as this code run, the process will either:
1. Shutdown because no other page is using the process
2. Keep running because other pages are using the process
3. Keep running but enter the process cache, if the process is cacheable and the cache is enabled (default on macOS).

In case 2, your code likely works as expected. In case 3, which is probably the common case, your code likely works too but it is new behavior to have a process in the process cache that may still have a WebPage inside it.
In case 1 though (which you can likely reproduce by disabling the process cache and making sure the process is only used by your current page), the process would exit right away. Wouldn't this mean that you fix does nothing in this case?

[Sihui Liu]

(In reply to Chris Dumez from comment #16)
> Comment on attachment 408023 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408023&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:1133
> >      m_process->removeWebPage(*this, WebProcessProxy::EndsUsingDataStore::Yes);
> 
> As soon as this code run, the process will either:
> 1. Shutdown because no other page is using the process
> 2. Keep running because other pages are using the process
> 3. Keep running but enter the process cache, if the process is cacheable and
> the cache is enabled (default on macOS).
> 
> In case 2, your code likely works as expected. In case 3, which is probably
> the common case, your code likely works too but it is new behavior to have a
> process in the process cache that may still have a WebPage inside it.
> In case 1 though (which you can likely reproduce by disabling the process
> cache and making sure the process is only used by your current page), the
> process would exit right away. Wouldn't this mean that you fix does nothing
> in this case?

You mean web process will exit after receiving this message or after sending this message? Either seems Okay because we just need UI process to commit view hierarchy change before this. 

Our current issue is transaction is committed in UI process at the end of a runloop cycle (or even later), and this message can be received by web process before UI process reaches to the commit point.

[Chris Dumez]

(In reply to Sihui Liu from comment #17)
> (In reply to Chris Dumez from comment #16)
> > Comment on attachment 408023 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=408023&action=review
> > 
> > > Source/WebKit/UIProcess/WebPageProxy.cpp:1133
> > >      m_process->removeWebPage(*this, WebProcessProxy::EndsUsingDataStore::Yes);
> > 
> > As soon as this code run, the process will either:
> > 1. Shutdown because no other page is using the process
> > 2. Keep running because other pages are using the process
> > 3. Keep running but enter the process cache, if the process is cacheable and
> > the cache is enabled (default on macOS).
> > 
> > In case 2, your code likely works as expected. In case 3, which is probably
> > the common case, your code likely works too but it is new behavior to have a
> > process in the process cache that may still have a WebPage inside it.
> > In case 1 though (which you can likely reproduce by disabling the process
> > cache and making sure the process is only used by your current page), the
> > process would exit right away. Wouldn't this mean that you fix does nothing
> > in this case?
> 
> You mean web process will exit after receiving this message or after sending
> this message? Either seems Okay because we just need UI process to commit
> view hierarchy change before this. 
> 
> Our current issue is transaction is committed in UI process at the end of a
> runloop cycle (or even later), and this message can be received by web
> process before UI process reaches to the commit point.

You are saying that it is not OK for the WebProcess to receive the Close IPC *before* the next event loop iteration. But WHY is it OK for the WebProcess to terminate before the next loop iteration?

[Sihui Liu]

Comment on attachment 408023 [details]
Patch

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

>>>> Source/WebKit/UIProcess/WebPageProxy.cpp:1133
>>>>      m_process->removeWebPage(*this, WebProcessProxy::EndsUsingDataStore::Yes);
>>> 
>>> As soon as this code run, the process will either:
>>> 1. Shutdown because no other page is using the process
>>> 2. Keep running because other pages are using the process
>>> 3. Keep running but enter the process cache, if the process is cacheable and the cache is enabled (default on macOS).
>>> 
>>> In case 2, your code likely works as expected. In case 3, which is probably the common case, your code likely works too but it is new behavior to have a process in the process cache that may still have a WebPage inside it.
>>> In case 1 though (which you can likely reproduce by disabling the process cache and making sure the process is only used by your current page), the process would exit right away. Wouldn't this mean that you fix does nothing in this case?
>> 
>> You mean web process will exit after receiving this message or after sending this message? Either seems Okay because we just need UI process to commit view hierarchy change before this. 
>> 
>> Our current issue is transaction is committed in UI process at the end of a runloop cycle (or even later), and this message can be received by web process before UI process reaches to the commit point.
> 
> You are saying that it is not OK for the WebProcess to receive the Close IPC *before* the next event loop iteration. But WHY is it OK for the WebProcess to terminate before the next loop iteration?

oh I looked at the wrong line! You were talking about the removePage line and WebProcess maybeShutDown(). Does it sound reasonable to extend the lifetime of web process by asking for no shutdown until the the message is sent?

[Chris Dumez]

(In reply to Chris Dumez from comment #18)
> (In reply to Sihui Liu from comment #17)
> > (In reply to Chris Dumez from comment #16)
> > > Comment on attachment 408023 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=408023&action=review
> > > 
> > > > Source/WebKit/UIProcess/WebPageProxy.cpp:1133
> > > >      m_process->removeWebPage(*this, WebProcessProxy::EndsUsingDataStore::Yes);
> > > 
> > > As soon as this code run, the process will either:
> > > 1. Shutdown because no other page is using the process
> > > 2. Keep running because other pages are using the process
> > > 3. Keep running but enter the process cache, if the process is cacheable and
> > > the cache is enabled (default on macOS).
> > > 
> > > In case 2, your code likely works as expected. In case 3, which is probably
> > > the common case, your code likely works too but it is new behavior to have a
> > > process in the process cache that may still have a WebPage inside it.
> > > In case 1 though (which you can likely reproduce by disabling the process
> > > cache and making sure the process is only used by your current page), the
> > > process would exit right away. Wouldn't this mean that you fix does nothing
> > > in this case?
> > 
> > You mean web process will exit after receiving this message or after sending
> > this message? Either seems Okay because we just need UI process to commit
> > view hierarchy change before this. 
> > 
> > Our current issue is transaction is committed in UI process at the end of a
> > runloop cycle (or even later), and this message can be received by web
> > process before UI process reaches to the commit point.
> 
> You are saying that it is not OK for the WebProcess to receive the Close IPC
> *before* the next event loop iteration. But WHY is it OK for the WebProcess
> to terminate before the next loop iteration?

Note that you can delay the process exit by relying WebProcessProxy::makeScopePreventingShutdown() but it will likely need some modifications so that it can be captured in a lambda.

[Geoffrey Garen]

> Does it sound reasonable to extend the
> lifetime of web process by asking for no shutdown until the the message is
> sent?

Yes, I think it's reasonable. Or at least, equivalent behavior is reasonable. We are asking the web process not to shutdown because we are still using its tiles to render our view. At least in our current architecture, where tiles are owned by the web process, I can't think of any other way to express the behavior we want.

[Sihui Liu]

Created attachment 408463 [details]
Patch

[Chris Dumez]

Comment on attachment 408463 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessProxy.h:297
> +    class ScopePreventingShutdown : public RefCounted<ScopePreventingShutdown> {

I this point, I would recommend replacing WebProcessProxy::m_shutdownPreventingScopeCount with a RefCounter (see existing code for examples), then we would not need this ScopePreventingShutdown class at all.

> Source/WebKit/UIProcess/WebProcessProxy.h:-316
> -    ScopePreventingShutdown makeScopePreventingShutdown() { return ScopePreventingShutdown { *this }; }

I think it looks nicer with a convenience getter.

[Sihui Liu]

Created attachment 408466 [details]
Patch

[Chris Dumez]

Comment on attachment 408466 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1160
> +    RunLoop::current().dispatch([destinationID = messageSenderDestinationID(), protectedProcess = m_process.copyRef(), token = m_process->shutdownPreventingScopeCount()] {

token is too generic. Maybe preventShutdownScope?

> Source/WebKit/UIProcess/WebPageProxy.cpp:3322
> +            auto preventNavigationProcessShutdownToken = processForNavigation->shutdownPreventingScopeCount();

I don't think we need to add this Token suffix.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:204
> +    , m_shutdownPreventingScopeCounter([this](RefCounterEvent) { maybeShutDown(); })

if (event == RefCounterEvent::decrement) maybeShutDown();

> Source/WebKit/UIProcess/WebProcessProxy.h:298
> +    typedef RefCounter<ShutdownPreventingScopeType> ShutdownPreventingScopeCounter;

using ShutdownPreventingScopeCounter = RefCounter<ShutdownPreventingScopeType>;

> Source/WebKit/UIProcess/WebProcessProxy.h:299
> +    ShutdownPreventingScopeCounter::Token shutdownPreventingScopeCount() { return m_shutdownPreventingScopeCounter.count(); }

I would call this shutdownPreventingScope(), I don't think "count" is helpful here.

[Sihui Liu]

Created attachment 408468 [details]
Patch

[Sihui Liu]

Created attachment 409142 [details]
Patch for landing

[EWS]

Committed r267250: <https://trac.webkit.org/changeset/267250>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409142 [details].