...
Created attachment 407904 [details] Patch
Created attachment 407918 [details] Patch
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.
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?
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
<rdar://problem/62264106>
Created attachment 408012 [details] Patch
Created attachment 408014 [details] Patch
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."
Created attachment 408023 [details] Patch
Delaying one runloop cycle may not be enough, ideally the page should be closed after transaction is committed, which can be many cycles later.
(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.
(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 :|
(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!
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/
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?
(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.
(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?
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?
(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.
> 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.
Created attachment 408463 [details] Patch
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.
Created attachment 408466 [details] Patch
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.
Created attachment 408468 [details] Patch
Created attachment 409142 [details] Patch for landing
Committed r267250: <https://trac.webkit.org/changeset/267250> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409142 [details].