Ensure that the Web Content process doesn't sleep during initialization
<rdar://problem/40548159>
Created attachment 341256 [details] Patch
Comment on attachment 341256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341256&action=review > Source/WebKit/UIProcess/WebProcessProxy.cpp:1426 > + m_backgroundActivityTokenForInitialization = nullptr; Need to check if we have to clear this if the process crashes in the meantime.
Comment on attachment 341256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341256&action=review >> Source/WebKit/UIProcess/WebProcessProxy.cpp:1426 >> + m_backgroundActivityTokenForInitialization = nullptr; > > Need to check if we have to clear this if the process crashes in the meantime. Yes, I believe we do need to.
Comment on attachment 341256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341256&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:983 > + // This must remain after the last message sent in this function, > + // otherwise the Web Content process could be suspended before > + // initialization is complete. > + process.didSendAllProcessInitializationMessages(); Could you use a ScopeExit for this to ensure this always runs last? I'm worried that maybe someone will add an early return in the future.
Comment on attachment 341256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341256&action=review >> Source/WebKit/UIProcess/WebProcessPool.cpp:983 >> + process.didSendAllProcessInitializationMessages(); > > Could you use a ScopeExit for this to ensure this always runs last? I'm worried that maybe someone will add an early return in the future. Why not just take the assertion inside WebProcessProxy::didFinishLaunching or some similar function in WebProcessProxy? That would get you a tighter abstraction. The background assertion can be an internal implementation detail of WebProcessProxy startup, and then we don't have to enforce order relative to commands issued by WebProcessPool. The contract WebProcessProxy should expose to clients is "If you launch me, I will always finish launching before sleeping. You don't have to do anything special to make this happen, and you also can't stop it from happening." >>> Source/WebKit/UIProcess/WebProcessProxy.cpp:1426 >>> + m_backgroundActivityTokenForInitialization = nullptr; >> >> Need to check if we have to clear this if the process crashes in the meantime. > > Yes, I believe we do need to. You can just clear the token unconditionally in the crash handler. Probably doesn't make a big difference, since a live background assertion for a crashed process probably does nothing. But still good hygiene.
Comment on attachment 341256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341256&action=review >> Source/WebKit/UIProcess/WebProcessPool.cpp:983 >> + process.didSendAllProcessInitializationMessages(); > > Could you use a ScopeExit for this to ensure this always runs last? I'm worried that maybe someone will add an early return in the future. Now there’s a good idea.
(In reply to Geoffrey Garen from comment #6) > Comment on attachment 341256 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341256&action=review > > >> Source/WebKit/UIProcess/WebProcessPool.cpp:983 > >> + process.didSendAllProcessInitializationMessages(); > > > > Could you use a ScopeExit for this to ensure this always runs last? I'm worried that maybe someone will add an early return in the future. > > Why not just take the assertion inside WebProcessProxy::didFinishLaunching > or some similar function in WebProcessProxy? > > That would get you a tighter abstraction. > > The background assertion can be an internal implementation detail of > WebProcessProxy startup, and then we don't have to enforce order relative to > commands issued by WebProcessPool. > > The contract WebProcessProxy should expose to clients is "If you launch me, > I will always finish launching before sleeping. You don't have to do > anything special to make this happen, and you also can't stop it from > happening." Good point! > >>> Source/WebKit/UIProcess/WebProcessProxy.cpp:1426 > >>> + m_backgroundActivityTokenForInitialization = nullptr; > >> > >> Need to check if we have to clear this if the process crashes in the meantime. > > > > Yes, I believe we do need to. > > You can just clear the token unconditionally in the crash handler. Probably > doesn't make a big difference, since a live background assertion for a > crashed process probably does nothing. But still good hygiene.
Comment on attachment 341256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341256&action=review >>>>> Source/WebKit/UIProcess/WebProcessPool.cpp:983 >>>>> + process.didSendAllProcessInitializationMessages(); >>>> >>>> Could you use a ScopeExit for this to ensure this always runs last? I'm worried that maybe someone will add an early return in the future. >>> >>> Why not just take the assertion inside WebProcessProxy::didFinishLaunching or some similar function in WebProcessProxy? >>> >>> That would get you a tighter abstraction. >>> >>> The background assertion can be an internal implementation detail of WebProcessProxy startup, and then we don't have to enforce order relative to commands issued by WebProcessPool. >>> >>> The contract WebProcessProxy should expose to clients is "If you launch me, I will always finish launching before sleeping. You don't have to do anything special to make this happen, and you also can't stop it from happening." >> >> Now there’s a good idea. > > Good point! Wait, no. I don’t think there *is* a place in WebProcessProxy that we know that other people (like WebProcessPool) are done sending initialization messages. This is all messy already (why does WebProcessPool::initializeNewWebProcess do so much work?!), but didFinishLaunching is waaaaay too early (as far as I can tell).
> Wait, no. I don’t think there *is* a place in WebProcessProxy that we know > that other people (like WebProcessPool) are done sending initialization > messages. I see. OK, here's a potential alternative to put at the end of initializeNewWebProcess: // Make sure the WebProcess finishes initialization before going to sleep. auto token = throttler().backgroundActivityToken(); process.isResponsive([token] { }); // We don't really care what the answer is; this is just a convenient way to verify that our previous messages have been processed.
> auto token = throttler().backgroundActivityToken(); > process.isResponsive([token] { }); // We don't really care what the answer > is; this is just a convenient way to verify that our previous messages have > been processed. If we want to be extra pedantic, we can even take the assertion before the first message, to ensure that the web process starts receiving messages right away.
Created attachment 341325 [details] Patch
Comment on attachment 341325 [details] Patch r=me
Comment on attachment 341325 [details] Patch Clearing flags on attachment: 341325 Committed r232209: <https://trac.webkit.org/changeset/232209>
All reviewed patches have been landed. Closing bug.