136219 2014-08-25 03:45:48 -0700 Websocket state should be set to closed in didReceiveMessage call back 2014-09-15 17:45:18 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 shiva.jm webkit-unassigned ap commit-queue darin gyuyoung.kim kling ryuan.choi oldest_to_newest 1031093 0 shiva.jm 2014-08-25 03:45:48 -0700 According to W3C specification link: http://www.w3.org/TR/websockets/ , Websocket state should be set to closed in didReceiveMessage call back 1031103 1 237076 shiva.jm 2014-08-25 04:30:04 -0700 Created attachment 237076 Patch Set the Websocket state to closed in didReceiveMessage call back 1031159 2 237076 darin 2014-08-25 09:42:45 -0700 Comment on attachment 237076 Patch WebKit bug fixes require either a test case or an explanation of why they don’t have one. Please add a regression test. 1031353 3 237144 shiva.jm 2014-08-26 03:25:31 -0700 Created attachment 237144 Patch-Updated Updated patch with new layout test 1032184 4 237144 darin 2014-08-29 10:09:21 -0700 Comment on attachment 237144 Patch-Updated View in context: https://bugs.webkit.org/attachment.cgi?id=237144&action=review > Source/WebCore/Modules/websockets/WebSocket.cpp:536 > + m_state = CLOSED; Do we also need to call m_channel->disconnect()? Do we also need to call ActiveDOMObject::unsetPendingActivity? 1032454 5 shiva.jm 2014-09-01 01:25:08 -0700 (In reply to comment #4) > (From update of attachment 237144 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237144&action=review > > > Source/WebCore/Modules/websockets/WebSocket.cpp:536 > > + m_state = CLOSED; > > Do we also need to call m_channel->disconnect()? Do we also need to call ActiveDOMObject::unsetPendingActivity? Dear Darin, Thanks for inputs, i rechecked again, i think we no need to close the channel object here, also ActiveDOMObject. 1034065 6 237144 darin 2014-09-08 22:45:37 -0700 Comment on attachment 237144 Patch-Updated View in context: https://bugs.webkit.org/attachment.cgi?id=237144&action=review >>> Source/WebCore/Modules/websockets/WebSocket.cpp:536 >>> + m_state = CLOSED; >> >> Do we also need to call m_channel->disconnect()? Do we also need to call ActiveDOMObject::unsetPendingActivity? > > Dear Darin, > > Thanks for inputs, i rechecked again, i think we no need to close the channel object here, also ActiveDOMObject. I’m glad you think so. Why do you think so? 1034085 7 shiva.jm 2014-09-09 00:06:46 -0700 (In reply to comment #6) > (From update of attachment 237144 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237144&action=review > > >>> Source/WebCore/Modules/websockets/WebSocket.cpp:536 > >>> + m_state = CLOSED; > >> > >> Do we also need to call m_channel->disconnect()? Do we also need to call ActiveDOMObject::unsetPendingActivity? > > > > Dear Darin, > > > > Thanks for inputs, i rechecked again, i think we no need to close the channel object here, also ActiveDOMObject. > > I’m glad you think so. Why do you think so? Dear Darin, Here is my analysis, When i ran the new test cases error-event-ready-state.html, first we get onerror() call back, where the proper state will be set, after these onclose() will get called, so here the channel object will get closed and ActiveDOMObject as well. so we no need to release the resources in didReceiveMessageError() call back. I tried by closing channel object in didReceiveMessageError() call back. but it was failing some other tests, with error: FAIL: Timed out waiting for notifyDone to be called. pls share your inputs, if my understanding is not correct. 1035112 8 darin 2014-09-14 12:38:22 -0700 Alexey, you know this code better than I do. Could you review? 1035342 9 237144 ap 2014-09-15 17:39:49 -0700 Comment on attachment 237144 Patch-Updated The existing code is somewhat confusing, but I think that the patch a step in the right direction. The key consideration is that there is only one place where an error event is dispatched per the spec, and that's right before a close event. 1035345 10 237144 commit-queue 2014-09-15 17:45:12 -0700 Comment on attachment 237144 Patch-Updated Clearing flags on attachment: 237144 Committed r173642: <http://trac.webkit.org/changeset/173642> 1035346 11 commit-queue 2014-09-15 17:45:18 -0700 All reviewed patches have been landed. Closing bug. 237076 2014-08-25 04:30:04 -0700 2014-08-26 03:25:31 -0700 Patch bug-136219.patch text/plain 1306 shiva.jm SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE3MjkxNSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE0LTA4LTI1ICBTaGl2YWt1 bWFyIEpNICA8c2hpdmEuam1Ac2Ftc3VuZy5jb20+CisKKyAgICAgICAgV2ViIENvcmU6IFdlYnNv Y2tldCBzdGF0ZSBzaG91bGQgYmUgc2V0IHRvIGNsb3NlZCBpbiBkaWRSZWNlaXZlTWVzc2FnZSBj YWxsIGJhY2suCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMzYyMTkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBTZXQgdGhlIFdlYnNvY2tldCBzdGF0ZSB0byBjbG9zZWQgaW4gZGlkUmVjZWl2ZU1lc3NhZ2Ug Y2FsbCBiYWNrCisKKyAgICAgICAgKiBNb2R1bGVzL3dlYnNvY2tldHMvV2ViU29ja2V0LmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OldlYlNvY2tldDo6ZGlkUmVjZWl2ZU1lc3NhZ2VFcnJvcik6CisK IDIwMTQtMDgtMjUgIFNoaXZha3VtYXIgSk0gIDxzaGl2YS5qbUBzYW1zdW5nLmNvbT4KIAogICAg ICAgICBXZWIgQ29yZTogRml4IHVudXNlZCBwYXJhbWV0ZXIgYnVpbGQgd2FybmluZy4KSW5kZXg6 IFNvdXJjZS9XZWJDb3JlL01vZHVsZXMvd2Vic29ja2V0cy9XZWJTb2NrZXQuY3BwCj09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT0KLS0tIFNvdXJjZS9XZWJDb3JlL01vZHVsZXMvd2Vic29ja2V0cy9XZWJTb2NrZXQuY3BwCShy ZXZpc2lvbiAxNzI5MTUpCisrKyBTb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL3dlYnNvY2tldHMvV2Vi U29ja2V0LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTMzLDYgKzUzMyw3IEBAIHZvaWQgV2ViU29j a2V0OjpkaWRSZWNlaXZlQmluYXJ5RGF0YShQYXMKIHZvaWQgV2ViU29ja2V0OjpkaWRSZWNlaXZl TWVzc2FnZUVycm9yKCkKIHsKICAgICBMT0coTmV0d29yaywgIldlYlNvY2tldCAlcCBkaWRSZWNl aXZlRXJyb3JNZXNzYWdlKCkiLCB0aGlzKTsKKyAgICBtX3N0YXRlID0gQ0xPU0VEOwogICAgIEFT U0VSVChzY3JpcHRFeGVjdXRpb25Db250ZXh0KCkpOwogICAgIGRpc3BhdGNoRXZlbnQoRXZlbnQ6 OmNyZWF0ZShldmVudE5hbWVzKCkuZXJyb3JFdmVudCwgZmFsc2UsIGZhbHNlKSk7CiB9Cg== 237144 2014-08-26 03:25:31 -0700 2014-09-15 17:45:12 -0700 Patch-Updated bug-136219-updated.patch text/plain 3823 shiva.jm SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE3Mjk1MykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDE0LTA4LTI2ICBTaGl2YWt1 bWFyIEpNICA8c2hpdmEuam1Ac2Ftc3VuZy5jb20+CisKKyAgICAgICAgV2ViIENvcmU6IFdlYnNv Y2tldCBzdGF0ZSBzaG91bGQgYmUgc2V0IHRvIGNsb3NlZCBpbiBkaWRSZWNlaXZlTWVzc2FnZSBj YWxsIGJhY2suCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMzYyMTkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBTZXQgdGhlIFdlYnNvY2tldCBzdGF0ZSB0byBjbG9zZWQgaW4gZGlkUmVjZWl2ZU1lc3NhZ2Ug Y2FsbCBiYWNrLgorCisgICAgICAgIFRlc3Q6IGh0dHAvdGVzdHMvd2Vic29ja2V0L3Rlc3RzL2h5 YmkvZXJyb3ItZXZlbnQtcmVhZHktc3RhdGUuaHRtbAorCisgICAgICAgICogTW9kdWxlcy93ZWJz b2NrZXRzL1dlYlNvY2tldC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpXZWJTb2NrZXQ6OmRpZFJl Y2VpdmVNZXNzYWdlRXJyb3IpOgorCiAyMDE0LTA4LTI2ICBNYXJrIFJvd2UgIDxtcm93ZUBhcHBs ZS5jb20+CiAKICAgICAgICAgQnVpbGQgZml4IGFmdGVyIHIxNzI5NTEuCkluZGV4OiBTb3VyY2Uv V2ViQ29yZS9Nb2R1bGVzL3dlYnNvY2tldHMvV2ViU29ja2V0LmNwcAo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBT b3VyY2UvV2ViQ29yZS9Nb2R1bGVzL3dlYnNvY2tldHMvV2ViU29ja2V0LmNwcAkocmV2aXNpb24g MTcyOTUzKQorKysgU291cmNlL1dlYkNvcmUvTW9kdWxlcy93ZWJzb2NrZXRzL1dlYlNvY2tldC5j cHAJKHdvcmtpbmcgY29weSkKQEAgLTUzMyw2ICs1MzMsNyBAQCB2b2lkIFdlYlNvY2tldDo6ZGlk UmVjZWl2ZUJpbmFyeURhdGEoUGFzCiB2b2lkIFdlYlNvY2tldDo6ZGlkUmVjZWl2ZU1lc3NhZ2VF cnJvcigpCiB7CiAgICAgTE9HKE5ldHdvcmssICJXZWJTb2NrZXQgJXAgZGlkUmVjZWl2ZUVycm9y TWVzc2FnZSgpIiwgdGhpcyk7CisgICAgbV9zdGF0ZSA9IENMT1NFRDsKICAgICBBU1NFUlQoc2Ny aXB0RXhlY3V0aW9uQ29udGV4dCgpKTsKICAgICBkaXNwYXRjaEV2ZW50KEV2ZW50OjpjcmVhdGUo ZXZlbnROYW1lcygpLmVycm9yRXZlbnQsIGZhbHNlLCBmYWxzZSkpOwogfQpJbmRleDogTGF5b3V0 VGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL0NoYW5nZUxvZwkocmV2 aXNpb24gMTcyOTUzKQorKysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTQtMDgtMjYgIFNoaXZha3VtYXIgSk0gIDxzaGl2YS5qbUBzYW1z dW5nLmNvbT4KKworICAgICAgICBXZWIgQ29yZTogV2Vic29ja2V0IHN0YXRlIHNob3VsZCBiZSBz ZXQgdG8gY2xvc2VkIGluIGRpZFJlY2VpdmVNZXNzYWdlIGNhbGwgYmFjay4KKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEzNjIxOQorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEFkZGVkIHRlc3QgdG8gY2hlY2sg cmVhZHlTdGF0ZSBpcyBDTE9TRUQgd2l0aGluIG9uZXJyb3IgZXZlbnQuCisKKyAgICAgICAgKiBo dHRwL3Rlc3RzL3dlYnNvY2tldC90ZXN0cy9oeWJpL2Vycm9yLWV2ZW50LXJlYWR5LXN0YXRlLWV4 cGVjdGVkLnR4dDogQWRkZWQuCisgICAgICAgICogaHR0cC90ZXN0cy93ZWJzb2NrZXQvdGVzdHMv aHliaS9lcnJvci1ldmVudC1yZWFkeS1zdGF0ZS5odG1sOiBBZGRlZC4KKwogMjAxNC0wOC0yMyAg RmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAgICAgIEZUTCBzaG91bGQgYmUg YWJsZSB0byBkbyBwb2x5bW9ycGhpYyBjYWxsIGlubGluaW5nCkluZGV4OiBMYXlvdXRUZXN0cy9o dHRwL3Rlc3RzL3dlYnNvY2tldC90ZXN0cy9oeWJpL2Vycm9yLWV2ZW50LXJlYWR5LXN0YXRlLWV4 cGVjdGVkLnR4dAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9odHRwL3Rlc3RzL3dlYnNvY2tl dC90ZXN0cy9oeWJpL2Vycm9yLWV2ZW50LXJlYWR5LXN0YXRlLWV4cGVjdGVkLnR4dAkocmV2aXNp b24gMCkKKysrIExheW91dFRlc3RzL2h0dHAvdGVzdHMvd2Vic29ja2V0L3Rlc3RzL2h5YmkvZXJy b3ItZXZlbnQtcmVhZHktc3RhdGUtZXhwZWN0ZWQudHh0CShyZXZpc2lvbiAwKQpAQCAtMCwwICsx LDExIEBACitDT05TT0xFIE1FU1NBR0U6IFdlYlNvY2tldCBjb25uZWN0aW9uIHRvICd3czovL2xv Y2FsaG9zdDo4ODgwL25vbi1leGlzdGVudC11cmwnIGZhaWxlZDogVW5leHBlY3RlZCByZXNwb25z ZSBjb2RlOiA0MDQKK1Rlc3QgdGhhdCByZWFkeVN0YXRlIGlzIENMT1NFRCB3aXRoaW4gb25lcnJv ciBldmVudAorCitPbiBzdWNjZXNzLCB5b3Ugd2lsbCBzZWUgYSBzZXJpZXMgb2YgIlBBU1MiIG1l c3NhZ2VzLCBmb2xsb3dlZCBieSAiVEVTVCBDT01QTEVURSIuCisKK29uZXJyb3IoKSB3YXMgY2Fs bGVkLgorUEFTUyB3cy5yZWFkeVN0YXRlIGlzIDMKK1BBU1Mgc3VjY2Vzc2Z1bGx5UGFyc2VkIGlz IHRydWUKKworVEVTVCBDT01QTEVURQorCkluZGV4OiBMYXlvdXRUZXN0cy9odHRwL3Rlc3RzL3dl YnNvY2tldC90ZXN0cy9oeWJpL2Vycm9yLWV2ZW50LXJlYWR5LXN0YXRlLmh0bWwKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy93ZWJzb2NrZXQvdGVzdHMvaHliaS9lcnJvci1l dmVudC1yZWFkeS1zdGF0ZS5odG1sCShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMvaHR0cC90 ZXN0cy93ZWJzb2NrZXQvdGVzdHMvaHliaS9lcnJvci1ldmVudC1yZWFkeS1zdGF0ZS5odG1sCShy ZXZpc2lvbiAwKQpAQCAtMCwwICsxLDI1IEBACis8IURPQ1RZUEUgSFRNTD4KKzxodG1sPgorPGhl YWQ+Cis8c2NyaXB0IHNyYz0iLi4vLi4vLi4vLi4vanMtdGVzdC1yZXNvdXJjZXMvanMtdGVzdC5q cyI+PC9zY3JpcHQ+Cis8L2hlYWQ+Cis8Ym9keT4KKzxkaXYgaWQ9ImRlc2NyaXB0aW9uIj48L2Rp dj4KKzxkaXYgaWQ9ImNvbnNvbGUiPjwvZGl2PgorPHNjcmlwdCB0eXBlPSJ0ZXh0L2phdmFzY3Jp cHQiPgorZGVzY3JpcHRpb24oIlRlc3QgdGhhdCByZWFkeVN0YXRlIGlzIENMT1NFRCB3aXRoaW4g b25lcnJvciBldmVudCIpOworCit3aW5kb3cuanNUZXN0SXNBc3luYyA9IHRydWU7CisKK3ZhciB3 cyA9IG5ldyBXZWJTb2NrZXQoIndzOi8vbG9jYWxob3N0Ojg4ODAvbm9uLWV4aXN0ZW50LXVybCIp OworCit3cy5vbmVycm9yID0gZnVuY3Rpb24oZXZlbnQpIHsKKyAgICBkZWJ1Zygib25lcnJvcigp IHdhcyBjYWxsZWQuIik7CisgICAgc2hvdWxkQmUoIndzLnJlYWR5U3RhdGUiLCAiMyIpOworICAg IGZpbmlzaEpTVGVzdCgpOworfTsKKworPC9zY3JpcHQ+Cis8c2NyaXB0IHNyYz0iLi4vLi4vLi4v Li4vanMtdGVzdC1yZXNvdXJjZXMvanMtdGVzdC5qcyI+PC9zY3JpcHQ+Cis8L2JvZHk+Cis8L2h0 bWw+Cg==