mORMot Open Source

oz

Member

Registered: 2015-09-02

Posts: 95

[issue] Serious issue with THttpApiWebSocketServer and Safari Browser

Hi,
there is a serious issue with THttpApiWebSocketServer connections consumed by Safari Browser Clients. I've tested with latest MacOS Desktop Browser and iPhone.

Steps to reproduce:
1. Open "Project31WinHttpEchoServer.dpr".
2. Change value "localhost" in constructor TSimpleWebsocketServer.Create to your (external) hostname for being able to test it with external devices.
3. Also add ",true" for registering hostname within http.sys

constructor TSimpleWebsocketServer.Create;
begin
  fServer := THttpApiWebSocketServer.Create(false, 8, 1);
  fServer.AddUrl('','8888', False, 'PUT YOUR PUBLIC HOSTNAME HERE', true);
  fServer.AddUrlWebSocket('whatever', '8888', False, 'PUT YOUR PUBLIC HOSTNAME HERE', true);
  // ManualFragmentManagement = false - so Server will join all packet fragments
  // automatically and call onMessage with full message content
  fServer.RegisterProtocol('meow', False, onAccept, onMessage, onConnect, onDisconnect);
  fServer.RegisterCompress(CompressDeflate);
  fServer.OnRequest := onHttpRequest;
  fServer.Clone(8);
end;

4. Compile and run Project

5. Open Firefox, navigate to "http://hostname:8888"
-> console shows "HTTP request to /"
"Connected 0"

6. Reload page
-> console shows "Disconnected 0 1001"
-> Ok.

7. Now open Safari and repeat steps 5 and 6.
-> no disconnect message appears
It seems like if those invalid connections aren't handled properly.

8. Enter "send 0" [ENTER] in console

An external exception is fired in procedure THttpApiWebSocketConnection.InternalSend, and from now on things start going wrong. In production all of our calls to "THttpApiWebSocketConnection.Send" are protected by "try...except" blocks, but this doesn't help. Windows decides to kill the running server process as soon as this situation with undetected closed connections appears.

@mpv / @ab: Could you have a look at this please!?

King regards,
oz.

mpv

Member

From: Ukraine

Registered: 2012-03-24

Posts: 1,544

Website

Re: [issue] Serious issue with THttpApiWebSocketServer and Safari Browser

Can you trace that sends the desktop Safati to the web socket connection during page reload? (In chrome/firefox it is possible by clicking on the web socket connection(network tab)  and look onto the frames)

oz

Member

Registered: 2015-09-02

Posts: 95

Re: [issue] Serious issue with THttpApiWebSocketServer and Safari Browser

With Safari it is quite the same as with Firefox/Chrome.
Here is what I did:

Open new tab, enter url: "http://omacwin10/8888"
-> Page is loaded
Console log:

Event {isTrusted: true, type: "open", target: WebSocket, currentTarget: WebSocket, eventPhase: 2, …}

Network/Websocket log:

WebSocket-connection established

-> Enter "Test1", click Send
Console output:

MessageEvent {isTrusted: true, origin: "ws://omacwin10:8888", lastEventId: "", source: null, data: "Test1", …}

Network/Websocket log:

[out]: Test1
[in]: Test1

-> Reload page:
Console output:

[NOTHING LOGGED]

Network/Websocket log:

Connection closed

So, it basically looks like if nothing is sent by Safari. The connection is simply closed by the browser without any data being sent. It looks like if that closed connection isn't recognized by the http.sys API based server.

King regards,
oz.

oz

Member

Registered: 2015-09-02

Posts: 95

Re: [issue] Serious issue with THttpApiWebSocketServer and Safari Browser

Pavel, I think i traced the problem down and have a fix available...

I've added a breakpoint to "function THttpApiWebSocketConnection.ProcessActions(ActionQueue: WEB_SOCKET_ACTION_QUEUE): boolean;":

Following two Actions happen when closing the tab with Firefox:

1) WEB_SOCKET_INDICATE_RECEIVE_COMPLETE_ACTION
  BufferType = WEB_SOCKET_CLOSE_BUFFER_TYPE

2) WEB_SOCKET_SEND_TO_NETWORK_ACTION

Following single action happens when closing the tab with Safari:

1) WEB_SOCKET_RECEIVE_FROM_NETWORK_ACTION
THttpApiWebSocketConnection.ReadData... is called.
Inside this function -> Err= ERROR_HANDLE_EOF -> Disconnect is called.

I've changed procedure ReadData to a function:

function THttpApiWebSocketConnection.ReadData(const WebsocketBufferData): integer;
var Err: HRESULT;
    fBytesRead: cardinal;
    aBuf: WEB_SOCKET_BUFFER_DATA absolute WebsocketBufferData;
begin
  result:=0;
  if fWSHandle = nil then
    exit;
  Err := Http.ReceiveRequestEntityBody(fProtocol.fServer.FReqQueue, fOpaqueHTTPRequestId, 0,
    aBuf.pbBuffer, aBuf.ulBufferLength, fBytesRead, @self.fOverlapped);
  case Err of
    ERROR_HANDLE_EOF:
    begin
//     Disconnect;
      Result:=-1;
    end;
    ERROR_IO_PENDING: ; //
    NO_ERROR: ;//
  else
    // todo: close connection
  end;
end;

and the calling function

function THttpApiWebSocketConnection.ProcessActions(ActionQueue: WEB_SOCKET_ACTION_QUEUE): boolean;
...
    case Action of
...
      WEB_SOCKET_RECEIVE_FROM_NETWORK_ACTION: begin
        for i := 0 to ulDataBufferCount - 1 do
          if ReadData(Buffer[i])=-1 then begin
            EnterCriticalSection(fProtocol.fSafe);
            try
              fProtocol.RemoveConnection(fIndex);
            finally
              LeaveCriticalSection(fProtocol.fSafe);
            end;
            fState := wsClosedByClient
            fBuffer:='';
            fCloseStatus := 1001;//Buffer[0].Reserved1;
            EWebSocketApi.RaiseOnError(hCompleteAction, WebSocketAPI.CompleteAction(fWSHandle, ActionContext, 0));
            result := False;
            exit;
          end;
        fLastActionContext := ActionContext;
        result := False;
        exit;
      end;
...

With this fix everything works as expected. Could you have a look at that fix and incorporate it into trunk if ok?

Cheers,
oz.

mpv

Member

From: Ukraine

Registered: 2012-03-24

Posts: 1,544

Website

Re: [issue] Serious issue with THttpApiWebSocketServer and Safari Browser

@oz - thanks for debugging. Now I understand the problem. I will refactor HttpApiWebSocket code on weekend. In additional to your proposal we need clean fPendingForClose sessions, because fProtocol.RemoveConnection(fIndex) will put the session in PendingForClose, but since we never receive close event in case of safari pending session list will grow to infinity...

oz

Member

Registered: 2015-09-02

Posts: 95

Re: [issue] Serious issue with THttpApiWebSocketServer and Safari Browser

@oz - thanks for debugging. Now I understand the problem. I will refactor HttpApiWebSocket code on weekend. In additional to your proposal we need clean fPendingForClose sessions, because fProtocol.RemoveConnection(fIndex) will put the session in PendingForClose, but since we never receive close event in case of safari pending session list will grow to infinity...

You're welcome, i'm glad being able to help/contribute...

I've just checked it again, you're wrong about PendingForClose List growing to inifinity.
fProtocol.RemoveConnection(fIndex) puts the connection to PendingForClose, that's right.
But fState ist set to fState := wsClosedByClient after that.
THttpApiWebSocketConnection.ProcessActions is called by  TSynThreadPoolHttpApiWebSocketServer.Task.
Inside TSynThreadPoolHttpApiWebSocketServer.Task, the pending connection is deleted by following block because fState=wsClosedByClient.

  if conn.fState in [wsClosedByClient,wsClosedByServer,wsClosedByGuard,wsClosedByShutdown] then begin
    conn.DoOnDisconnect;
    if conn.fState = wsClosedByClient then
      conn.Close(conn.fCloseStatus, Pointer(conn.fBuffer), length(conn.fBuffer));
    conn.Disconnect;
    EnterCriticalSection(conn.Protocol.fSafe);
    try
      conn.Protocol.fPendingForClose.Remove(conn);
    finally
      LeaveCriticalSection(conn.Protocol.fSafe);
    end;
    Dispose(conn);
  end;

Adding following debug output prooves this:

  writeln(conn.Protocol.fPendingForClose.Count, ' pending connections.'); -> 0 pending connections

King regards,
oz.

Last edited by oz (2018-04-19 08:21:27)

mpv

Member

From: Ukraine

Registered: 2012-03-24

Posts: 1,544

Website

Re: [issue] Serious issue with THttpApiWebSocketServer and Safari Browser

Please, check [18f0308]
Thanks for debugging and fix