790919 - Don't dispatch close event, and remove onclose

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

The spec doesn't define onclose or close event, as far as I can see.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

http://lists.w3.org/Archives/Public/public-whatwg-archive/2009May/0406.html

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch 1

https://tbpl.mozilla.org/?tree=Try&rev=1436e08384f6 on try.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Do other browsers support close event?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think we should remove the close event. The reason it was removed from specifications is that it in a pretty indirect way exposes GC behavior. However it was requested by developers to do things like freeing up resources. Without a close event you have to rely on timeouts instead.

The GC issue is definitely real, but I think the risk to reward here is acceptable.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What I think we did when the 'close' event was removed from the spec was that we announced that we would keep it as a prototype API to see if the GC issue would result in problems. So far I don't think we've seen any problems.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

But if Gecko is the only implementation which has close event, we should probably remove it, IMHO.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

With the direction things are heading, with workers gaining more traction and getting more useful, I'd rather that we push for other UAs to implement this.

Comment 8

[Andrea Marchesini [:baku]]

I agree with your idea. Can you write an email to some public list proposing this?
Btw: it's always fun to remove code :)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

It is not clear to me what can be done during close event listener.
IMO network connections shouldn't be opened if the main page is already closed.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 662592 [details] [diff] [review]
patch 1

Canceling review, I think this is WONTFIX.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

What all can be done while handling close events?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

As far as communicating with the outside world goes, the only thing you can do is to place synchronous XHR requests.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

But does even that work? It shouldn't if the window owning the worker has gone away already.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, if the close handler runs in response to the user leaving the page, then you are correct. But if the close handler runs as a result of the worker calling self.close(), or as a result of a parent calling worker.terminate(), or as a result of the worker being GCed, then the loadgroup is still not closed.

Unrelated to workers, there's also been discussion about a way to have explicit API for firing off network requests from window.onunload (to avoid people doing sync XHR requests from onunload). *If* that goes through we should allow such explicit API also from worker.onclose. But that's really a separate discussion.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

What is the use case for worker itself calling close and yet wanting to do something in
close event handler? And what is the use case for calling terminate and expecting the worker to
do something?
Both cases should be handled differently.
In first case worker should just use XHR before calling .close() and in the latter
case page should send some message to worker to tell it to close, and then worker could
use XHR and after that do .close().

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Olli Pettay [:smaug] from comment #15)
> What is the use case for worker itself calling close and yet wanting to do
> something in
> close event handler? And what is the use case for calling terminate and
> expecting the worker to
> do something?

The use-cases are the same. Wanting to tell a server that the worker is going away such that it can free up resources.

> Both cases should be handled differently.

Why? Having to listen to multiple callbacks to cover the same usecases is generally the opposite of good design.

> in the latter case page should send some message to worker to tell it to close,
> and then worker could use XHR and after that do .close().

One of the important use-cases for .terminate is to be able to abort a longrunning calculation. Without .terminate() the worker code has to constantly return to the event loop which dramatically complicates the code. One of the main points of workers is to be able to write code that doesn't constantly return to the event loop.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Has any other browser implemented this? If not, we should fix this bug.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

If we keep close handler, is it permissible for the handler to add more events to the thread event queue?  What exactly is allowed?

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There hasn't been any discussion either way about onclose for many years. I'm still opposed to removing this until we reaffirm that other vendors really aren't going to implement.

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Could you reach out to them?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I leave the decision here to Bent.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I haven't had an opportunity to do this and I don't expect to be able to any time soon. But I agree with Jonas in comment 19.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: close.patch

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

FWIW, I just checked chrome, safari, and edge using Olli's workerconsole.  They all resolve `"onclose" in self` to `false`.

Its been 7 years since it was removed from the spec.  I think we can safely remove this.

I'll review later today.

Comment 25

[Andrea Marchesini [:baku]]

Attachment: close.patch

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8782393 [details] [diff] [review]
close.patch

Review of attachment 8782393 [details] [diff] [review]:
-----------------------------------------------------------------

Kris, can you review the change to subprocess_worker_common.js here?  We would like to get rid of the worker onclose event since its not a web compatible feature.  How dependent is subprocess.js on this feature?

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Another reason we need to do this is we are firing close events on service worker threads.  It seems dangerous to expose our grace timer behavior this exactly.

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8782393 [details] [diff] [review]
close.patch

Review of attachment 8782393 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/subprocess/subprocess_worker_common.js
@@ -212,5 @@
> -
> -onclose = event => {
> -  io.shutdown();
> -
> -  self.postMessage({msg: "close"});

We still need to send this message to the the parent thread so it stops blocking shutdown, so it should probably be done from `io.shutdown` in subprocess_worker_unix.js and subprocess_worker_win.js instead.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kris Maglione [:kmag] from comment #28)
> We still need to send this message to the the parent thread so it stops
> blocking shutdown, so it should probably be done from `io.shutdown` in
> subprocess_worker_unix.js and subprocess_worker_win.js instead.

It seems the subprocess_worker_unix.js only calls io.shutdown() when an error is encountered.  Is there another mechanism that calls it during a normal shutdown process?  Can you describe how the worker thread normally closes here?  Does the window drop its ref, call terminate(), or is it supposed to call self.close() at some point?

Comment 30

[Kris Maglione [:kmag]]

(In reply to Ben Kelly [:bkelly] from comment #29)
> (In reply to Kris Maglione [:kmag] from comment #28)
> > We still need to send this message to the the parent thread so it stops
> > blocking shutdown, so it should probably be done from `io.shutdown` in
> > subprocess_worker_unix.js and subprocess_worker_win.js instead.
> 
> It seems the subprocess_worker_unix.js only calls io.shutdown() when an
> error is encountered.  Is there another mechanism that calls it during a
> normal shutdown process?  Can you describe how the worker thread normally
> closes here?  Does the window drop its ref, call terminate(), or is it
> supposed to call self.close() at some point?

subprocess_common calls it when the parent process sends a shutdown message to the worker, io.shutdown() calls self.close(), then a message is sent to the parent process to let it know that the worker has finished shutting down.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

So we need to add:

  self.postMessage({msg: "close"});

Here:

  https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/subprocess/subprocess_worker_unix.js#503
  https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/subprocess/subprocess_worker_win.js#602

Thanks!

Comment 32

[Kris Maglione [:kmag]]

Yes, that should do it.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: close.patch

Comment 34

[Andrea Marchesini [:baku]]

Comment on attachment 8782779 [details] [diff] [review]
close.patch

Not fully green on try yet.

Comment 35

[Andrea Marchesini [:baku]]

Attachment: close2.patch

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8782914 [details] [diff] [review]
close2.patch

Review of attachment 8782914 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Flagging Kris to review the subprocess changes.

::: dom/webidl/WorkerGlobalScope.webidl
@@ +56,1 @@
>  

nit: remove this blank line as well?

::: dom/workers/RuntimeService.h
@@ -204,5 @@
>  
> -  static uint32_t
> -  GetContentCloseHandlerTimeoutSeconds()
> -  {
> -    return sDefaultJSSettings.content.maxScriptRuntime;

I think we can remove maxScriptRuntime as well.  It doesn't appear to be used from anything else in workers.  The pref is used by main thread script, though.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Andrea, can you send an "intent to unship" message to dev-platform?

Comment 38

[Andrea Marchesini [:baku]]

The reason why this patch is not landed yet is because there is a intermittent failure test that happens any 1 or 2 push on try.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4cb0bcf30a7
Don't dispatch close event in Workers, r=bkelly

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4cb0bcf30a7

Comment 41

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/workers-will-no-longer-dispatch-close-event/

Comment 42

[Eric Shepherd [:sheppy]]

Documentation has been updated:

https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope
https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope/onclose
https://developer.mozilla.org/en-US/docs/Web/Events/close
https://developer.mozilla.org/en-US/Firefox/Releases/51

Let me know if I’ve missed anything.