Ensure that the Web Content process doesn't sleep during initialization

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Ensure that the Web Content process doesn't sleep during initialization

[Radar WebKit Bug Importer]

<rdar://problem/40548159>

[Tim Horton]

Created attachment 341256 [details]
Patch

[Tim Horton]

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.

[Chris Dumez]

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.

[Andy Estes]

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.

[Geoffrey Garen]

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.

[Tim Horton]

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.

[Tim Horton]

(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.

[Tim Horton]

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).

[Geoffrey Garen]

> 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.

[Geoffrey Garen]

> 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.

[Tim Horton]

Created attachment 341325 [details]
Patch

[Geoffrey Garen]

Comment on attachment 341325 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 341325 [details]
Patch

Clearing flags on attachment: 341325

Committed r232209: <https://trac.webkit.org/changeset/232209>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.