Bug 34633 – WebSocket bufferedAmount should not be 0 when send after close.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Fumitoshi Ukai]

WebSocket API says:
 The bufferedAmount attribute must return the number of bytes that have been queued but not yet sent. If the connection is closed, this attribute's value will only increase with each call to the send() method (the number does not reset to zero once the connection closes).

[Fumitoshi Ukai]

Created attachment 48206 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 48206 [details]
Patch

+        m_bufferedAmountAfterClose += message.utf8().length() + 2; // 2 for frameing

"Framing"

[Fumitoshi Ukai]

Committed r54694: <http://trac.webkit.org/changeset/54694>

[Eric Seidel (no email)]

This is causing crashes under Leopard.  See bug 34898.

I recommend we roll this out.

[Eric Seidel (no email)]

Reverted r54694 for reason:

This appears to have caused crashes on the Leopard bot.  See bug 34898.

Committed r54741: <http://trac.webkit.org/changeset/54741>

[Eric Seidel (no email)]

*** Bug 34898 has been marked as a duplicate of this bug. ***

[Fumitoshi Ukai]

(In reply to comment #6)
> *** Bug 34898 has been marked as a duplicate of this bug. ***

(In reply to comment #4)
> This is causing crashes under Leopard.  See bug 34898.

Thanks for crash report.

It shows it crashed at 

  m_client->didReceiveMessage(String::fromUTF8(msgStart, p - msgStart));

so m_client is 0 here.
This is because m_client could be 0 after some user callback called
(m_client->didConnect() was called before in this function).
Note that it is related to https://bugs.webkit.org/show_bug.cgi?id=34636 (onclose synchronously called. onopen -> close -> onclose).

I'll add check !m_client to fix the crash.

[Fumitoshi Ukai]

Created attachment 48737 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 48737 [details]
Patch

+                if (frameByte == 0x00 && !m_client)
                     m_client->didReceiveMessage(String::fromUTF8(msgStart, p - msgStart));

The check is backwards (should be m_client, not !m_client). Did you run regression tests with this change? If no test detects such a mistake, we should try to make one that does.

Also, do we even need to continue parsing when m_client becomes null? It seems that an early return would be a better solution.

[Fumitoshi Ukai]

(In reply to comment #9)
> (From update of attachment 48737 [details])
> +                if (frameByte == 0x00 && !m_client)
>                      m_client->didReceiveMessage(String::fromUTF8(msgStart, p -
> msgStart));
> 
> The check is backwards (should be m_client, not !m_client). Did you run
> regression tests with this change? If no test detects such a mistake, we should
> try to make one that does.

Oops. Thanks for catching this.

> Also, do we even need to continue parsing when m_client becomes null? It seems
> that an early return would be a better solution.

Ok, I add check !m_client after it calls m_client methods, because it calls user callback, user callback may call close() and onclose handler would set m_client = 0.

[Fumitoshi Ukai]

Created attachment 48952 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 48952 [details]
Patch

r=me

[Fumitoshi Ukai]

Committed r54927: <http://trac.webkit.org/changeset/54927>