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

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

Do other browsers support close event?

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.

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.

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

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.

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

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 on attachment 662592 [details] [diff] [review] patch 1 Canceling review, I think this is WONTFIX.

What all can be done while handling close events?

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

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

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.

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

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

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

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

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.

Could you reach out to them?

I leave the decision here to Bent.

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.

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

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

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

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

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!

Yes, that should do it.

Comment on attachment 8782779 [details] [diff] [review] close.patch Not fully green on try yet.

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.

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

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.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e4cb0bcf30a7 Don't dispatch close event in Workers, r=bkelly

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

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.