Bug 34014 – WebSocket wrapper can be collected even if events are pending

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Created attachment 47218 [details]
test case

WebSocket inherits from ActiveDOMObject, but it never sets pending activity. As a result, it can be garbage collected even if has a network exchange ongoing, and is going to post events. This violates a basic JS principle - garbage collection should not affect program execution.

A fix for this would be to call setPendingActivity when WebSocket waits for data from the network (and match it with unsetPendingActivity).

[Fumitoshi Ukai]

Created attachment 47655 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 47655 [details]
Patch

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

[Alexey Proskuryakov]

Actually, I do not think that contextDestroyed() is the right place for cleanup - ActiveDOMObject has a stop() method for this. Is there a reason you didn't override that?

[Fumitoshi Ukai]

Created attachment 47806 [details]
Patch

[Fumitoshi Ukai]

(In reply to comment #2)
> (From update of attachment 47655 [details])
> +    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?

I implemented stop() method to close the connection, and contextDestoryed() just checks the connection was already closed.
It seems this fixes for bug 33248 as well.  I'll write layout test for it.

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

Thanks.  Fixed.

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

[Alexey Proskuryakov]

Comment on attachment 47806 [details]
Patch

Doesn't stop() need to call unsetPendingActivity()?

[Fumitoshi Ukai]

Created attachment 47807 [details]
Patch

[Fumitoshi Ukai]

(In reply to comment #6)
> (From update of attachment 47806 [details])
> Doesn't stop() need to call unsetPendingActivity()?

Ah, I think it needs.
Could you take a look again, please?

[Alexey Proskuryakov]

Comment on attachment 47807 [details]
Patch

ok, r=me

[Fumitoshi Ukai]

(In reply to comment #9)
> (From update of attachment 47807 [details])
> ok, r=me

Thanks.
Anyway, I tried to land it, but found several crashes with release build on Mac Leopard.  No crash with debug build.
I investigated it and found malloc claims some error in debug build.

DumpRenderTree(82014,0xa0830720) malloc: *** error for object 0x1364cc60: Non-aligned pointer being freed (2)
*** set a breakpoint in malloc_error_break to debug

and the object 0x1364cc60 is got by tryFastMalloc for m_buffer in WebSocketChannel.
WebSocketChannel doesn't move the pointer returned by tryFastMalloc (it just copies data in the allocated area) and I confirmed that tryFastMalloc actually returned 0x1364cc60.
Is this malloc bug?

[Fumitoshi Ukai]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 47807 [details] [details])
> > ok, r=me
> 
> Thanks.
> Anyway, I tried to land it, but found several crashes with release build on Mac
> Leopard.  No crash with debug build.
> I investigated it and found malloc claims some error in debug build.
> 
> DumpRenderTree(82014,0xa0830720) malloc: *** error for object 0x1364cc60:
> Non-aligned pointer being freed (2)
> *** set a breakpoint in malloc_error_break to debug
> 
> and the object 0x1364cc60 is got by tryFastMalloc for m_buffer in
> WebSocketChannel.
> WebSocketChannel doesn't move the pointer returned by tryFastMalloc (it just
> copies data in the allocated area) and I confirmed that tryFastMalloc actually
> returned 0x1364cc60.
> Is this malloc bug?

This could be fixed if we alloc 8-byte aligned size by tryFastMalloc for m_buffer in WebSocketChannel.
Is this contract of tryFastMalloc, so that client must request aligned size? (if so, should we assert this in tryFastMalloc?)
or is this tryFastMalloc bug?

[Alexey Proskuryakov]

I have never heard of such issues with FastMalloc.

[Eric Seidel (no email)]

Attachment 47807 [details] was posted by a committer and has review+, assigning to Fumitoshi Ukai for commit.

[Fumitoshi Ukai]

Created attachment 47878 [details]
Patch

[Fumitoshi Ukai]

(In reply to comment #12)
> I have never heard of such issues with FastMalloc.

Ah, I found a bug in the previous patch.
I need to protect WebSocketChannel in didReceivedData, because it would call some user callback which might close the connection and deallocate WebSocketChannel in it, but didReceivedData would call skipBuffer after that.

New patch fixed crash bugs found in previous patch.

[Fumitoshi Ukai]

Committed r54267: <http://trac.webkit.org/changeset/54267>