Summary: | WebSocket bufferedAmount should not be 0 when send after close. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fumitoshi Ukai <ukai> | ||||||||
Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, eric | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 34898 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Fumitoshi Ukai
2010-02-04 23:38:50 PST
Created attachment 48206 [details]
Patch
Comment on attachment 48206 [details]
Patch
+ m_bufferedAmountAfterClose += message.utf8().length() + 2; // 2 for frameing
"Framing"
Committed r54694: <http://trac.webkit.org/changeset/54694> This is causing crashes under Leopard. See bug 34898. I recommend we roll this out. 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> *** Bug 34898 has been marked as a duplicate of this bug. *** (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. Created attachment 48737 [details]
Patch
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.
(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. Created attachment 48952 [details]
Patch
Comment on attachment 48952 [details]
Patch
r=me
Committed r54927: <http://trac.webkit.org/changeset/54927> |