Networking process on iOS can be suspended and never exit

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Networking process on iOS can be suspended and never exit

This is for apps that use multiple WKProcessPools and sometimes deallocate a WKProcessPool that had an active Networking process.

In those cases, the Networking process can be suspended instead of terminated, and once suspended it can no longer respond to further messages.

Furthermore, calling RunLoop::main().stop() is not aggressive enough to actually terminate the process - _exit is bulletproof.

rdar://problem/20368630

[Brady Eidson]

Note: The bug exists on iOS, but the fix of having the Networking process explicitly kill itself instead of implicitly noticing an IPC::Connection close is reasonable to implement cross-platform

[Brady Eidson]

Created attachment 253060 [details]
Patch v1

[Brady Eidson]

Apparently big changes to ProcessThrottler in the last couple of days. Updating and fixing the build.

[Brady Eidson]

Created attachment 253063 [details]
Patch v2

[Brady Eidson]

Created attachment 253067 [details]
Patch v3

[Brady Eidson]

Comment on attachment 253067 [details]
Patch v3

Sam, Gavin, and I looked at this in person and came up with a plan for even more changes.  Removing r?

[Brady Eidson]

Created attachment 253235 [details]
New patch v1

[Darin Adler]

Comment on attachment 253235 [details]
New patch v1

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

r=me assuming you have a good way to test the new behavior

> Source/WebKit2/Shared/ChildProcess.cpp:166
> +    // FIXME: Subclasses of ChildProcess should all have a chance to do some explicit cleanup
> +    // before the process is terminated.

I think this FIXME is too vague. What kinds of cleanup specifically are you thinking about? While this comment may be correct, I’m not sure we should have a FIXME here unless we know of some specific cleanup that is not being done at this time. We don’t need a FIXME about having a hook that we wouldn’t have a reason to do at this time.

> Source/WebKit2/Shared/ChildProcess.h:100
> +    virtual void didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) override;

Can this override be private instead of protected?

[Brady Eidson]

(In reply to comment #8)
> Comment on attachment 253235 [details]
> New patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253235&action=review
> 
> r=me assuming you have a good way to test the new behavior

Indeed.

> 
> > Source/WebKit2/Shared/ChildProcess.cpp:166
> > +    // FIXME: Subclasses of ChildProcess should all have a chance to do some explicit cleanup
> > +    // before the process is terminated.
> 
> I think this FIXME is too vague. What kinds of cleanup specifically are you
> thinking about? While this comment may be correct, I’m not sure we should
> have a FIXME here unless we know of some specific cleanup that is not being
> done at this time. We don’t need a FIXME about having a hook that we
> wouldn’t have a reason to do at this time.

Fair enough.

> > Source/WebKit2/Shared/ChildProcess.h:100
> > +    virtual void didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) override;
> 
> Can this override be private instead of protected?

Nope - For now, the subclasses NetworkProcess and WebProcess call it directly from within their didReceiveMessage.

We'll be working on a way to make that unnecessary in the future, at which point it can be private.

[Brady Eidson]

http://trac.webkit.org/changeset/184503

[Brady Eidson]

Fixed broken API tests in followup: http://trac.webkit.org/changeset/184514