Add support for disabling/enabling termination to ChildProcess

[Anders Carlsson]

Add support for disabling/enabling termination to ChildProcess

[Anders Carlsson]

Created attachment 89452 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 89452 [details]
Patch

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

> Source/WebKit2/ChangeLog:28
> +        (WebKit::PluginProcess::getSitesWithData):
> +        Call disableTermination()/enableTermination().
> +
> +        (WebKit::PluginProcess::clearSiteData):
> +        Ditto.

Why do we need to wrap these functions in disable/enable calls?

> Source/WebKit2/Shared/ChildProcess.h:39
> +    void disableTermination();
> +    void enableTermination();

Maybe there should be comments here instead of/in addition to the ones for m_terminationCounter.

> Source/WebKit2/WebProcess/WebProcess.cpp:121
> +    : ChildProcess(0.0)

Can you get rid of the .0?

[Anders Carlsson]

(In reply to comment #2)
> (From update of attachment 89452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89452&action=review
> 
> > Source/WebKit2/ChangeLog:28
> > +        (WebKit::PluginProcess::getSitesWithData):
> > +        Call disableTermination()/enableTermination().
> > +
> > +        (WebKit::PluginProcess::clearSiteData):
> > +        Ditto.
> 
> Why do we need to wrap these functions in disable/enable calls?

Because without a call to enableTermination, the timer will never start.

> 
> > Source/WebKit2/Shared/ChildProcess.h:39
> > +    void disableTermination();
> > +    void enableTermination();
> 
> Maybe there should be comments here instead of/in addition to the ones for m_terminationCounter.

I'll add comments.

> 
> > Source/WebKit2/WebProcess/WebProcess.cpp:121
> > +    : ChildProcess(0.0)
> 
> Can you get rid of the .0?

Yes.

[Adam Roben (:aroben)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 89452 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=89452&action=review
> > 
> > > Source/WebKit2/ChangeLog:28
> > > +        (WebKit::PluginProcess::getSitesWithData):
> > > +        Call disableTermination()/enableTermination().
> > > +
> > > +        (WebKit::PluginProcess::clearSiteData):
> > > +        Ditto.
> > 
> > Why do we need to wrap these functions in disable/enable calls?
> 
> Because without a call to enableTermination, the timer will never start.

So there's some case where someone will create PluginProcess that has no web process connections, will then call one of these functions, and expect the process to exit on its own?

[Anders Carlsson]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 89452 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=89452&action=review
> > > 
> > > > Source/WebKit2/ChangeLog:28
> > > > +        (WebKit::PluginProcess::getSitesWithData):
> > > > +        Call disableTermination()/enableTermination().
> > > > +
> > > > +        (WebKit::PluginProcess::clearSiteData):
> > > > +        Ditto.
> > > 
> > > Why do we need to wrap these functions in disable/enable calls?
> > 
> > Because without a call to enableTermination, the timer will never start.
> 
> So there's some case where someone will create PluginProcess that has no web process connections, will then call one of these functions, and expect the process to exit on its own?

That is correct. The web process is not involved in getSitesWithData/clearSiteData.

[Anders Carlsson]

Committed r83767: <http://trac.webkit.org/changeset/83767>