Bug 179838 – Terminate workers for a registration that no longer has any client registrations

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Brady Eidson]

Terminate workers for a registration that no longer has any clients

[Brady Eidson]

Created attachment 327256 [details]
Patch

[Chris Dumez]

Comment on attachment 327256 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServer.cpp:49
> +static Seconds terminationDelay(5.0);

static Seconds terminationDelay { 5_s };

> Source/WebCore/workers/service/server/SWServer.cpp:500
> +        if (registration && registration->identifier() == identifier && !registration->hasClientsUsingRegistration())

Having clients and having clients using the registration is not the same thing. This looks very aggressive to me.

[youenn fablet]

Comment on attachment 327256 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServer.cpp:49
> +static Seconds terminationDelay(5.0);

This timer value might be too small, no?

I also wonder whether tieing sw life to controlling clients is not too restrictive. 
Taking this example:
- several foo.com pages are opened.
- a foo.com service worker is live and does some web socket-like communication on behalf of all the foo.com pages (bandwidth optimization). It does not control any client.
- postMessage is used between foo.com pages and their service worker to exchange message updates.

Would it be safer as a first approach to terminate a service worker shortly after all foo.com pages are closed.

> Source/WebCore/workers/service/server/SWServer.cpp:495
> +void SWServer::registrationNowHasNoClients(SWServerRegistration& registration)

s/NoClient/NoClients ?

[Brady Eidson]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 327256 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327256&action=review
> 
> > Source/WebCore/workers/service/server/SWServer.cpp:49
> > +static Seconds terminationDelay(5.0);
> 
> static Seconds terminationDelay { 5_s };
> 
> > Source/WebCore/workers/service/server/SWServer.cpp:500
> > +        if (registration && registration->identifier() == identifier && !registration->hasClientsUsingRegistration())
> 
> Having clients and having clients using the registration is not the same
> thing. This looks very aggressive to me.

AAAAGGGH

yes, I know this, and uploaded the wrong one like a bonehead.

Ignore, new one incoming.

[Brady Eidson]

(In reply to youenn fablet from comment #3)
> Comment on attachment 327256 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327256&action=review
> 
> > Source/WebCore/workers/service/server/SWServer.cpp:49
> > +static Seconds terminationDelay(5.0);
> 
> This timer value might be too small, no?

Is it?

> 
> I also wonder whether tieing sw life to controlling clients is not too
> restrictive. 
> Taking this example:
> - several foo.com pages are opened.
> - a foo.com service worker is live and does some web socket-like
> communication on behalf of all the foo.com pages (bandwidth optimization).
> It does not control any client.
> - postMessage is used between foo.com pages and their service worker to
> exchange message updates.
> 
> Would it be safer as a first approach to terminate a service worker shortly
> after all foo.com pages are closed.

That's (in effect) what this does.

Once I upload the right one.

[Brady Eidson]

Created attachment 327264 [details]
Patch

[Brady Eidson]

(In reply to Brady Eidson from comment #5)
> (In reply to youenn fablet from comment #3)
> > Comment on attachment 327256 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=327256&action=review
> > 
> > > Source/WebCore/workers/service/server/SWServer.cpp:49
> > > +static Seconds terminationDelay(5.0);
> > 
> > This timer value might be too small, no?
> 
> Is it?
> 
> > 
> > I also wonder whether tieing sw life to controlling clients is not too
> > restrictive. 
> > Taking this example:
> > - several foo.com pages are opened.
> > - a foo.com service worker is live and does some web socket-like
> > communication on behalf of all the foo.com pages (bandwidth optimization).
> > It does not control any client.
> > - postMessage is used between foo.com pages and their service worker to
> > exchange message updates.
> > 
> > Would it be safer as a first approach to terminate a service worker shortly
> > after all foo.com pages are closed.
> 
> That's (in effect) what this does.
> 
> Once I upload the right one.

Nevermind, I see. foo.com can, of course, be served by a SW even if it doesn't have a client registration object.

This patch is indeed too aggressive.

We don't have any notion in the Storage process of "what pages that might be handled by a SW are open". We might need one.

[Brady Eidson]

Yah, this one is a bummer. The effect I was going for isn't acheivable with the info we have in the Storage process. Looking at alternatives, and taking suggestions.

[Chris Dumez]

(In reply to Brady Eidson from comment #8)
> Yah, this one is a bummer. The effect I was going for isn't acheivable with
> the info we have in the Storage process. Looking at alternatives, and taking
> suggestions.

I think there is some overlap with self.clients which Youenn is planning to work on and we need a list of clients for that.

[youenn fablet]

(In reply to Chris Dumez from comment #9)
> (In reply to Brady Eidson from comment #8)
> > Yah, this one is a bummer. The effect I was going for isn't acheivable with
> > the info we have in the Storage process. Looking at alternatives, and taking
> > suggestions.
> 
> I think there is some overlap with self.clients which Youenn is planning to
> work on and we need a list of clients for that.

Right, the plan is something like:
1. any new client (document right now) created would check the store map and register itself to storage process if its origin is in the store
2. any new registration from a new origin should build its list of clients by querying each web process

I'll start with 1.

[Brady Eidson]

Hmmmmmm is serviceWorkerStartedControllingClient valid for this?

Any time a connection closes we could check both m_connectionsWithClientRegistrations and m_clientsUsingRegistration for emptiness.

Thoughts?