[webkit-reviews] review granted: [Bug 34014] WebSocket wrapper can be collected even if events are pending : [Attachment 47655] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 34014: WebSocket wrapper can be collected even if events are pending
https://bugs.webkit.org/show_bug.cgi?id=34014

Attachment 47655: Patch
https://bugs.webkit.org/attachment.cgi?id=47655&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+    LOG(Network, "WebSocket %p scriptExecutionContext destroyed", this);

There may be too many logs long term - very useful when writing the code, but
perhaps overly noisy when debugging other network-related issues.

We'll need to review the use of LOG() in WebSocket code as it matures.

I'd love to see per-function comments - the addition of contextDestroyed()
really needs an explanation of why it was needed. Actually, is it also a fix
for bug 33248?

+    if (m_channel)
+	 ActiveDOMObject::unsetPendingActivity(this);
+    m_channel = 0;

Accessing "this" after unsetPendingActivity seems dangerous. I don't see how
this can cause issues with current code, but that's the call that lets the
object get deleted eventually. I think that it would be good defensive
programming to move access to m_channel up.

r=me. Please consider addressing the comments about ChangeLog and m_channel.