Bug 34636

Summary: WebSocket close() causes a task to be queued to fire a close event
Product: WebKit Reporter: Fumitoshi Ukai <ukai>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: ap, bfulgham, joenotcharles, toyoshim, wilander
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch abarth: review-

Description Fumitoshi Ukai 2010-02-05 01:56:39 PST
WebSocket close() causes a task to be queued to fire a close event
Comment 1 Fumitoshi Ukai 2010-02-05 01:57:21 PST
Created attachment 48212 [details]
Patch
Comment 2 Fumitoshi Ukai 2010-02-05 02:00:44 PST
On WebKit nightly build, WebSocket close() method calls onclose event handler synchronously, but WebSocket API spec says

The close() method must close the Web Socket connection or connection attempt, if any, and change the readyState attribute's value to CLOSED (2). If the connection is already closed, it must do nothing.

Closing the connection immediately causes a task to be queued to fire a close event, as described below.

So, running WebKitTools/Scripts/run-webkit-tests on attached layout tests fails for now.

Chromium works as expected.
Comment 3 Alexey Proskuryakov 2010-02-10 10:27:38 PST
Comment on attachment 48212 [details]
Patch

+    close_called = true;
+    shouldBeTrue("close_called");

I do not see what this check is good for.

You say that chromium works as expected. Did you test Safari?
Comment 4 Fumitoshi Ukai 2010-02-11 19:33:28 PST
(In reply to comment #3)
> (From update of attachment 48212 [details])
> +    close_called = true;
> +    shouldBeTrue("close_called");
> 
> I do not see what this check is good for.
> 
> You say that chromium works as expected. Did you test Safari?

In safari, it fails because onclose is called before close_called is set to true.
onclose endTest() at the end, so all lines after ws.close() in ws.onopen will not be executed because it is after layoutTestController.notifyDone();
Comment 5 Alexey Proskuryakov 2010-02-11 19:45:20 PST
If Safari fails this test, then we need a separate bug tracking this, and the test needs to be in Skipped list for most platforms.
Comment 6 Fumitoshi Ukai 2010-02-15 22:01:43 PST
Created attachment 48789 [details]
Patch
Comment 7 Adam Barth 2010-03-22 08:50:09 PDT
Comment on attachment 48789 [details]
Patch

What's the point of adding this test at this point in time?  I'm all for landing passing tests, but this test fails on most platforms.  A more appropriate course of action here is to fix the bug and land the patch at the same time.