Bug 34633 - WebSocket bufferedAmount should not be 0 when send after close.
Summary: WebSocket bufferedAmount should not be 0 when send after close.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 34898 (view as bug list)
Depends on: 34898
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-04 23:38 PST by Fumitoshi Ukai
Modified: 2010-02-17 21:32 PST (History)
2 users (show)

See Also:


Attachments
Patch (13.37 KB, patch)
2010-02-04 23:56 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (13.94 KB, patch)
2010-02-14 18:43 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (14.35 KB, patch)
2010-02-17 17:43 PST, Fumitoshi Ukai
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 2010-02-04 23:38:50 PST
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).
Comment 1 Fumitoshi Ukai 2010-02-04 23:56:59 PST
Created attachment 48206 [details]
Patch
Comment 2 Alexey Proskuryakov 2010-02-10 10:33:44 PST
Comment on attachment 48206 [details]
Patch

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

"Framing"
Comment 3 Fumitoshi Ukai 2010-02-11 18:13:13 PST
Committed r54694: <http://trac.webkit.org/changeset/54694>
Comment 4 Eric Seidel (no email) 2010-02-12 14:40:23 PST
This is causing crashes under Leopard.  See bug 34898.

I recommend we roll this out.
Comment 5 Eric Seidel (no email) 2010-02-12 15:21:35 PST
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>
Comment 6 Eric Seidel (no email) 2010-02-12 15:39:19 PST
*** Bug 34898 has been marked as a duplicate of this bug. ***
Comment 7 Fumitoshi Ukai 2010-02-14 18:42:44 PST
(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.
Comment 8 Fumitoshi Ukai 2010-02-14 18:43:15 PST
Created attachment 48737 [details]
Patch
Comment 9 Alexey Proskuryakov 2010-02-17 10:07:47 PST
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.
Comment 10 Fumitoshi Ukai 2010-02-17 17:37:25 PST
(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.
Comment 11 Fumitoshi Ukai 2010-02-17 17:43:44 PST
Created attachment 48952 [details]
Patch
Comment 12 Alexey Proskuryakov 2010-02-17 19:59:34 PST
Comment on attachment 48952 [details]
Patch

r=me
Comment 13 Fumitoshi Ukai 2010-02-17 21:32:28 PST
Committed r54927: <http://trac.webkit.org/changeset/54927>