Client closes transport on 'beforeunload' event even if user stays on the page

[athouary]

Describe the bug
Clients close the transport on 'beforeunload', even if the closing process was prevented.

To Reproduce

See my fiddle fork: https://github.com/athouary/socket.io-fiddle

Once on the page, randomly click on a blank space to make sure that the browser has detected a user interaction. Then try to close the tab. You should be asked if you really want to exit the page. Wait a few seconds, click cancel and see the error in the console: Uncaught Error: Transport not open.

Engine.IO server version: 5.0.0

Expected behavior
If the user decides to stay on the page instead of closing it, the socket transport should not be closed. I would suggest to close it on the 'unload' event instead of the 'beforeunload' one.

Platform:
I reproed the problem on Chrome 89 and Firefox 86, with a MacOS device. I'm quite sure it does not really matter as long as preventing the page from closing is possible.

[zxlin]

Related: socketio/socket.io-client#1451
Potentially related: socketio/socket.io#3838

The issue is in this commit: ed48b5d

The beforeunload event handler here closes everything down and prevents the user from emitting anything else (such as a last ack before page navigation).

@darrachequesne any suggestions on how to best handle this or a patch? Happy to contribute as well, let me know what you prefer

[darrachequesne]

This is indeed linked to ed48b5d.

The problem with using unload instead of beforeunload is that it does not fix the initial issue: socketio/socket.io#3639 (you still get a "disconnect" event on Firefox but not on Chrome)

I'm open to suggestions on how to fix this.

[zxlin]

@darrachequesne I'm not as familiar with browser specifics, but would something like a process.nextTick equivalent work here? I'm imagining such that user calls to emit (such as emitting to the server page change is happening) gets fired first, then in the next tick, the lib decides if it should close the socket. Thoughts?

[athouary]

@zxlin Delaying the transport closing might work for your use case but it won't if the page closing is canceled by the user via the browser confirmation prompt.

@darrachequesne Since I don't think there is any way to solve both socketio/socket.io#3639 and the current issue at the same time, the only solution I can think of is an option to prevent the lib from adding that beforeunload listener.

It would let the library consumer decide which drawback they accept, according to their usage. A good documentation will be needed so that consumer can be well aware of the consequences.

const socket = io(window.location, {
  closeBeforeUnload: false,
});

[zxlin]

@athouary good idea, another idea I just thought of is some libs has a pre-close event that gets fired, but if in the event handler, the user returns null explicitly (or like in browser e.preventDefault()) to stop the default behavior, that might be another possible way to fit both issues here.

The user could return null for example to reject closing or be given an opportunity to send out any last bits of data before closing. Thoughts?

[athouary]

@zxlin Good idea as well. It would be more flexible for the consumers to make the decision of the behavior they want right when the beforeunload event occurs. I think an actual Event would not work for the described pattern, but a callback may get the job done.

[darrachequesne]

A possible workaround would be to call event.stopImmediatePropagation(), in order to prevent the library from silently closing the connection:

window.addEventListener('beforeunload', (event) => {
  event.preventDefault();
  event.stopImmediatePropagation();
  event.returnValue = 'unused string';
});

What do you think?

Edited: stopImmediatePropagation instead of stopPropagation

[zxlin]

@darrachequesne is this workaround supposed to work currently? I just tried and it is not working for me, the socket was still closed. On Chrome 89.0.4389.82

[darrachequesne]

@zxlin sorry, I meant stopImmediatePropagation and not stopPropagation

You need to declare it before the socket creation:

window.addEventListener('beforeunload', (event) => {
  event.preventDefault();
  event.stopImmediatePropagation();
  event.returnValue = 'unused string';
});

const socket = io();

[zxlin]

@darrachequesne Thanks for that.

I can confirm this indeed works as a workaround for me, but it seems like it's not ideal for OP's case where the logic to prompt user for navigation confirmation may not be available prior to socket creation - leading to some wonky code where one'd have to check to see if stopImmediatePropagation is required in the event handler. I'm sort of in that bucket too where this is not needed on every page, only one of my pages need this so I'd have to add a check to see if I'm on that page.

Ideally a better solution prevails for the longer term. Happy to help test alternatives.

[athouary]

@darrachequesne Your workaround works very well. However, the requirement for the beforeunload listener with the stopImmediatePropagation to be set before the socket declaration may be quite hard to fulfill in some cases. It will also prevent other libraries beforeunload listeners from being triggered.

I'm implementing your solution as a workaround, but I still believe that an option or callback as described in previous comments would be more flexible and comfortable for the library consumers.

[thexeos]

Should this (new) behavior be dependent on user adding 'disconnect' event listeners on the client? Of course this would have to be documented, as adding the event listener some time later during development may suddenly break an application, but so would upgrading socket.io from previous version.

[darrachequesne]

The closeOnBeforeunload option was added in engine.io-client@5.1.0, in order to mitigate this issue.

Syntax:

const socket = require('engine.io-client')('ws://localhost', {
  closeOnBeforeunload: false // defaults to true
});

[javiergonzalezGenially]

Is there a version of socket.io-client that uses this version of engine-io-client?
The version I could find depends on ~5.0.0, so it resolves to 5.0.1

[texonidas]

I've run into a separate issue introduced by this. Currently in Chromium browsers, clicking a link with a tel or mailto handler triggers the beforeunload event, so any time a user clicks on those, all transports on the page are closed silently.

[darrachequesne]

@javiergonzalezGenially => socket.io-client@4.1.0

[javiergonzalezGenially]

Thanks! I just opened a PR to fix the TS typings