Bug 61353 – WebSocket: Use fail() when WebSocketChannel has failed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yuta Kitamura]

Part two of bug 61115.

[Yuta Kitamura]

Created attachment 94590 [details]
Patch

[Kent Tamura]

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?

[Yuta Kitamura]

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.

[Yuta Kitamura]

Created attachment 94596 [details]
Patch v2 (Add clarification in ChangeLog)

[Kent Tamura]

Comment on attachment 94596 [details]
Patch v2 (Add clarification in ChangeLog)

ok, it sounds reasonable.

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Yuta Kitamura]

Looking at http/tests/websocket/tests/frame-length-overflow.html

[Yuta Kitamura]

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".

[Yuta Kitamura]

Created attachment 94752 [details]
Patch v3 (Fix test result)

[Yuta Kitamura]

(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.

[Kent Tamura]

Comment on attachment 94752 [details]
Patch v3 (Fix test result)

ok

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Yuta Kitamura]

(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.

[WebKit Commit Bot]

Comment on attachment 94752 [details]
Patch v3 (Fix test result)

Clearing flags on attachment: 94752

Committed r87282: <http://trac.webkit.org/changeset/87282>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.