Bug 61353 - WebSocket: Use fail() when WebSocketChannel has failed
Summary: WebSocket: Use fail() when WebSocketChannel has failed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yuta Kitamura
URL:
Keywords:
Depends on:
Blocks: 61115
  Show dependency treegraph
 
Reported: 2011-05-24 03:13 PDT by Yuta Kitamura
Modified: 2011-05-25 02:52 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.75 KB, patch)
2011-05-24 03:47 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v2 (Add clarification in ChangeLog) (5.21 KB, patch)
2011-05-24 06:02 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from cr-jail-4 (206.92 KB, application/zip)
2011-05-24 20:03 PDT, WebKit Commit Bot
no flags Details
Patch v3 (Fix test result) (6.52 KB, patch)
2011-05-25 01:34 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.35 MB, application/zip)
2011-05-25 01:53 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yuta Kitamura 2011-05-24 03:13:43 PDT
Part two of bug 61115.
Comment 1 Yuta Kitamura 2011-05-24 03:47:39 PDT
Created attachment 94590 [details]
Patch
Comment 2 Kent Tamura 2011-05-24 03:56:02 PDT
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 3 Yuta Kitamura 2011-05-24 05:34:24 PDT
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.
Comment 4 Yuta Kitamura 2011-05-24 06:02:49 PDT
Created attachment 94596 [details]
Patch v2 (Add clarification in ChangeLog)
Comment 5 Kent Tamura 2011-05-24 17:24:02 PDT
Comment on attachment 94596 [details]
Patch v2 (Add clarification in ChangeLog)

ok, it sounds reasonable.
Comment 6 WebKit Commit Bot 2011-05-24 20:03:50 PDT
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
Comment 7 WebKit Commit Bot 2011-05-24 20:03:53 PDT
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
Comment 8 Yuta Kitamura 2011-05-24 23:31:45 PDT
Looking at http/tests/websocket/tests/frame-length-overflow.html
Comment 9 Yuta Kitamura 2011-05-24 23:38:14 PDT
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".
Comment 10 Yuta Kitamura 2011-05-25 01:34:30 PDT
Created attachment 94752 [details]
Patch v3 (Fix test result)
Comment 11 Yuta Kitamura 2011-05-25 01:37:57 PDT
(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 12 Kent Tamura 2011-05-25 01:51:38 PDT
Comment on attachment 94752 [details]
Patch v3 (Fix test result)

ok
Comment 13 WebKit Review Bot 2011-05-25 01:53:14 PDT
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
Comment 14 WebKit Review Bot 2011-05-25 01:53:20 PDT
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
Comment 15 Yuta Kitamura 2011-05-25 02:05:33 PDT
(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 16 WebKit Commit Bot 2011-05-25 02:52:49 PDT
Comment on attachment 94752 [details]
Patch v3 (Fix test result)

Clearing flags on attachment: 94752

Committed r87282: <http://trac.webkit.org/changeset/87282>
Comment 17 WebKit Commit Bot 2011-05-25 02:52:58 PDT
All reviewed patches have been landed.  Closing bug.