Summary: | WebSocket: Use fail() when WebSocketChannel has failed | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yuta Kitamura <yutak> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Yuta Kitamura <yutak> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ap, commit-queue, dglazkov, tkent, ukai, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 61115 | ||||||||||||||
Attachments: |
|
Description
Yuta Kitamura
2011-05-24 03:13:43 PDT
Created attachment 94590 [details]
Patch
Comment on attachment 94590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94590&action=review > Source/WebCore/ChangeLog:8 > + No change in behavior, thus no new tests. Is it true? To me, the patch looks to change console messages in some cases. > Source/WebCore/ChangeLog:12 > + We should not close the socket stream if m_closed is true. This is a behavior change, isn't it? Comment on attachment 94590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94590&action=review >> Source/WebCore/ChangeLog:8 >> + No change in behavior, thus no new tests. > > Is it true? > To me, the patch looks to change console messages in some cases. You are right. However, the messages changed (or added) in this patch are not covered in the existing LayoutTests, because they depend on fastMalloc's failure and, as far as I know, it's pretty hard to make it fail reliably. So I think I need to rephrase the above text to mention these facts. >> Source/WebCore/ChangeLog:12 >> + We should not close the socket stream if m_closed is true. > > This is a behavior change, isn't it? Actually, no, becuase SocketStreamHandle::close() does nothing and returns immediately if that socket stream is already closed. So, this change is just adding a shortcut, and does not change the behavior. I will update the ChangeLog on this, too. Created attachment 94596 [details]
Patch v2 (Add clarification in ChangeLog)
Comment on attachment 94596 [details]
Patch v2 (Add clarification in ChangeLog)
ok, it sounds reasonable.
Comment on attachment 94596 [details] Patch v2 (Add clarification in ChangeLog) Rejecting attachment 94596 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-..." exit_code: 2 Last 500 characters of output: tests/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ........... 749.90s total testing time 23615 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 15 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8730709 Created attachment 94731 [details]
Archive of layout-test-results from cr-jail-4
The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-4 Port: Mac Platform: Mac OS X 10.6.7
Looking at http/tests/websocket/tests/frame-length-overflow.html It seems I just need to apply the following (it didn't show up because there was a Chromium test expectation for this test...). --- /tmp/layout-test-results/http/tests/websocket/tests/frame-length-overflow-expected.txt 2011-05-24 19:31:57.000000000 -0700 +++ /tmp/layout-test-results/http/tests/websocket/tests/frame-length-overflow-actual.txt 2011-05-24 19:31:57.000000000 -0700 @@ -1,3 +1,4 @@ +CONSOLE MESSAGE: line 0: WebSocket frame length too large Make sure WebSocket does not crash and report error when it sees length overflow On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". Created attachment 94752 [details]
Patch v3 (Fix test result)
(In reply to comment #10) > Created an attachment (id=94752) [details] > Patch v3 (Fix test result) Kent-san, this patch was already reviewed by you, but could you take a brief look at this again? I updated the test result of http/tests/websocket/tests/frame-length-overflow.html, and added a ChangeLog entry for LayoutTests. Wording of WebCore/ChangeLog was also slightly changed. There's no code change. Comment on attachment 94752 [details]
Patch v3 (Fix test result)
ok
Comment on attachment 94752 [details] Patch v3 (Fix test result) Attachment 94752 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8726997 New failing tests: inspector/profiler/cpu-profiler-profiling.html Created attachment 94755 [details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #13) > (From update of attachment 94752 [details]) > Attachment 94752 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/8726997 > > New failing tests: > inspector/profiler/cpu-profiler-profiling.html This seems irrelevant; let me try CQ anyway. Comment on attachment 94752 [details] Patch v3 (Fix test result) Clearing flags on attachment: 94752 Committed r87282: <http://trac.webkit.org/changeset/87282> All reviewed patches have been landed. Closing bug. |