Summary: | WebSocket close() causes a task to be queued to fire a close event | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fumitoshi Ukai <ukai> | ||||||
Component: | New Bugs | Assignee: | 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
Fumitoshi Ukai
2010-02-05 01:56:39 PST
Created attachment 48212 [details]
Patch
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 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?
(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(); 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. Created attachment 48789 [details]
Patch
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.
|