Bug 73290 – [WebSocket] Add didUpdateBufferedAmount() callback to SocketStreamHandleClient

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Takashi Toyoshima]

Now, WebSocketChannel is exported as WebKit::WebSocket in Chromium port.
For now, bufferedAmount property needs some latency to get from plugins.
This callback allow client side cache its value and reduce IPC turnaround time latency.

[Takashi Toyoshima]

Created attachment 116915 [details]
Patch

[Yuta Kitamura]

Comment on attachment 116915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116915&action=review

> Source/WebCore/platform/network/SocketStreamHandleBase.cpp:42
> +    : m_handle(handle)

You don't have to save m_handle. As SocketStreamHandle is derived from SocketStreamHandleBase, it is safe to cast |this| to SocketStreamHandle* (i.e. static_cast<SocketStreamHandle*>(this) is always equal to |handle|). Compilers take care of offset adjustment.

> Source/WebCore/platform/network/SocketStreamHandleClient.h:49
> +        virtual void didUpdateBufferedAmount(SocketStreamHandle*, int bufferedAmount) { }

Is |int| large enough?

> Source/WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:2
> + * Copyright (C) 2011 Apple Inc.  All rights reserved.

Don't update copyright year of others.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:2
> + * Copyright (C) 2011 Brent Fulgham.  All rights reserved.

Ditto.

> Source/WebCore/platform/network/qt/SocketStreamHandleQt.cpp:2
> + * Copyright (C) 2011 Nokia Inc. All rights reserved.

Ditto.

> Source/WebCore/platform/network/win/SocketStreamHandleWin.cpp:2
> + * Copyright (C) 2011 Brent Fulgham.  All rights reserved.

Ditto.

[Kent Tamura]

Comment on attachment 116915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116915&action=review

> Source/WebCore/websockets/WebSocket.h:109
> +    virtual void didUpdateBufferedAmount(int bufferedAmount);

You had better append OVERRIDE.

> Source/WebCore/websockets/WebSocketChannel.h:84
> +    virtual void didUpdateBufferedAmount(SocketStreamHandle*, int bufferedAmount);

ditto.

[Takashi Toyoshima]

Created attachment 116941 [details]
Patch

[Takashi Toyoshima]

Comment on attachment 116915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116915&action=review

>> Source/WebCore/platform/network/SocketStreamHandleBase.cpp:42
>> +    : m_handle(handle)
> 
> You don't have to save m_handle. As SocketStreamHandle is derived from SocketStreamHandleBase, it is safe to cast |this| to SocketStreamHandle* (i.e. static_cast<SocketStreamHandle*>(this) is always equal to |handle|). Compilers take care of offset adjustment.

Thanks Yuta!
Now, my change is quite simpler.

>> Source/WebCore/platform/network/SocketStreamHandleClient.h:49
>> +        virtual void didUpdateBufferedAmount(SocketStreamHandle*, int bufferedAmount) { }
> 
> Is |int| large enough?

Originally, SocketStreamHandleBase::bufferedAmount() returns an int value.
But, size_t seems better than int because returned value comes from Vector::size() and it returns size_t.
On the other hand, I guess I must use unsigned long in WebSocketChannelClient because WebSocketChannel::bufferedAmount returns unsigned long value.

>> Source/WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:2
>> + * Copyright (C) 2011 Apple Inc.  All rights reserved.
> 
> Don't update copyright year of others.

Thanks.
But, these update will be removed in next change because I don't have to pass this pointer from them now.

>> Source/WebCore/websockets/WebSocket.h:109
>> +    virtual void didUpdateBufferedAmount(int bufferedAmount);
> 
> You had better append OVERRIDE.

Thanks.
I added OVERRIDE to all inherit methods in WebSocket and WebSocketChannel.

[Kent Tamura]

Comment on attachment 116941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116941&action=review

> Source/WebCore/websockets/WebSocket.h:93
> -    virtual const AtomicString& interfaceName() const;
> -    virtual ScriptExecutionContext* scriptExecutionContext() const;
> +    virtual const AtomicString& interfaceName() const OVERRIDE;
> +    virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE;

We had better NOT add OVERRIDE to functions other than didUpdateBufferAmount() in this patch.

> Source/WebCore/websockets/WebSocketChannelClient.h:49
> +        virtual void didUpdateBufferedAmount(unsigned long bufferedAmount) { }

Why is the argument type is unsigned long instead of size_t?

[Takashi Toyoshima]

Comment on attachment 116941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116941&action=review

>> Source/WebCore/websockets/WebSocket.h:93
>> +    virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE;
> 
> We had better NOT add OVERRIDE to functions other than didUpdateBufferAmount() in this patch.

You mean adding OVERRIDE to other methods must be splited into another change?
If so, I'll make another change to add OVERRIDEs.

>> Source/WebCore/websockets/WebSocketChannelClient.h:49
>> +        virtual void didUpdateBufferedAmount(unsigned long bufferedAmount) { }
> 
> Why is the argument type is unsigned long instead of size_t?

WebSocketChannel::bufferedAmount() return unsigned long value.
That mean all interfaces exposed to WebCore::WebSocket follows the WebSocket API specification defining types.
So I decide to use unsigned long for WebSocketChannelClient to keep consistency with them.

[Kent Tamura]

Comment on attachment 116941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116941&action=review

>>> Source/WebCore/websockets/WebSocket.h:93
>>> +    virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE;
>> 
>> We had better NOT add OVERRIDE to functions other than didUpdateBufferAmount() in this patch.
> 
> You mean adding OVERRIDE to other methods must be splited into another change?
> If so, I'll make another change to add OVERRIDEs.

Yes.

>>> Source/WebCore/websockets/WebSocketChannelClient.h:49
>>> +        virtual void didUpdateBufferedAmount(unsigned long bufferedAmount) { }
>> 
>> Why is the argument type is unsigned long instead of size_t?
> 
> WebSocketChannel::bufferedAmount() return unsigned long value.
> That mean all interfaces exposed to WebCore::WebSocket follows the WebSocket API specification defining types.
> So I decide to use unsigned long for WebSocketChannelClient to keep consistency with them.

Ok, I understand.

[Takashi Toyoshima]

Created attachment 116946 [details]
Patch

[Takashi Toyoshima]

Thank you Kent,
I uploaded revised one which remove all OVERRIDEs.

[Takashi Toyoshima]

Adding OVERRIDEs in another change, I notice that we must care for the worker version of WebSocket.
Please wait for the next patch which support the worker implementation.

[Takashi Toyoshima]

Created attachment 116956 [details]
Patch

[David Levin]

Comment on attachment 116956 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116956&action=review

A few drive by comments.

> Source/WebCore/platform/network/SocketStreamHandleBase.cpp:2
> + * Copyright (C) 2011 Google Inc.  All rights reserved.

In WebKit typically just add the year (2009, 2011) as opposed to deleting the old year like what is done in Chromium.

> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:148
> +    m_pendingTasks.append(createCallbackTask(&ThreadableWebSocketChannelClientWrapper::didUpdateBufferedAmountCallback, AllowCrossThreadAccess(this), bufferedAmount));

You shouldn't use "AllowCrossThreadAccess" here and in fact should get rid of it throughout this file.

Here's why:
1. It was added to highlight the places where we were not doing anything to help with lifetime of the object being passed.
2. At the time it was added, ThreadSafeRefCounted* objects passed through createCallbackTask did not have the ref count increased, so AllowCrossThreadAccess needed to be added here.
3. ThreadableWebSocketChannelClientWrapper is designed to be ref counted across threads -- largely to make sure that its lifetime is preserved by this call.

So in short, putting AllowCrossThreadAccess means that ThreadableWebSocketChannelClientWrapper may get deleted before the callback is done leading to unpredictable crashes.

Feel free to create a bug that blocks: https://bugs.webkit.org/show_bug.cgi?id=67942 to get rid of AllowCrossThreadAccess in this file. (I've been wanting to do this but I also wanted to look at the files and make sure it was the right thing to do.  In this case, the patch motivated me to do so.)

[David Levin]

Comment on attachment 116956 [details]
Patch

cq- due to my comments (specifically the ref count related one).

[Takashi Toyoshima]

Comment on attachment 116956 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116956&action=review

>> Source/WebCore/platform/network/SocketStreamHandleBase.cpp:2
>> + * Copyright (C) 2011 Google Inc.  All rights reserved.
> 
> In WebKit typically just add the year (2009, 2011) as opposed to deleting the old year like what is done in Chromium.

Thanks, I fix it in the next change.

>> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:148
>> +    m_pendingTasks.append(createCallbackTask(&ThreadableWebSocketChannelClientWrapper::didUpdateBufferedAmountCallback, AllowCrossThreadAccess(this), bufferedAmount));
> 
> You shouldn't use "AllowCrossThreadAccess" here and in fact should get rid of it throughout this file.
> 
> Here's why:
> 1. It was added to highlight the places where we were not doing anything to help with lifetime of the object being passed.
> 2. At the time it was added, ThreadSafeRefCounted* objects passed through createCallbackTask did not have the ref count increased, so AllowCrossThreadAccess needed to be added here.
> 3. ThreadableWebSocketChannelClientWrapper is designed to be ref counted across threads -- largely to make sure that its lifetime is preserved by this call.
> 
> So in short, putting AllowCrossThreadAccess means that ThreadableWebSocketChannelClientWrapper may get deleted before the callback is done leading to unpredictable crashes.
> 
> Feel free to create a bug that blocks: https://bugs.webkit.org/show_bug.cgi?id=67942 to get rid of AllowCrossThreadAccess in this file. (I've been wanting to do this but I also wanted to look at the files and make sure it was the right thing to do.  In this case, the patch motivated me to do so.)

OK, I see.
I created a bug https://bugs.webkit.org/show_bug.cgi?id=73336 and set to block 67942.

[Takashi Toyoshima]

Created attachment 116999 [details]
Patch

[David Levin]

(In reply to comment #15)
> (From update of attachment 116956 [details])
> OK, I see.
> I created a bug https://bugs.webkit.org/show_bug.cgi?id=73336 and set to block 67942.

PS Feel free to do it too :). (It should be trivial and hopefully fix some crashes for you.)

[Kent Tamura]

Comment on attachment 116999 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116999&action=review

> Source/WebCore/websockets/WebSocket.cpp:510
> +    LOG(Network, "WebSocket %p didUpdateBufferedAmount %lld", this, bufferedAmount);

Why %lld for unsigned long value?

[David Levin]

Comment on attachment 116999 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116999&action=review

> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:148
> +    m_pendingTasks.append(createCallbackTask(&ThreadableWebSocketChannelClientWrapper::didUpdateBufferedAmountCallback, AllowCrossThreadAccess(this), bufferedAmount));

Please remove AllowCrossThreadAccess :).

[Takashi Toyoshima]

Created attachment 117104 [details]
Patch

[Takashi Toyoshima]

Comment on attachment 116999 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116999&action=review

>> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:148
>> +    m_pendingTasks.append(createCallbackTask(&ThreadableWebSocketChannelClientWrapper::didUpdateBufferedAmountCallback, AllowCrossThreadAccess(this), bufferedAmount));
> 
> Please remove AllowCrossThreadAccess :).

The change you reviewed in 73336 follows this change and will overwrite this.
I decide to split removing AllowCrossThreadAccess change to 73336.

>> Source/WebCore/websockets/WebSocket.cpp:510
>> +    LOG(Network, "WebSocket %p didUpdateBufferedAmount %lld", this, bufferedAmount);
> 
> Why %lld for unsigned long value?

Oops, sorry for careless mistake.

[David Levin]

(In reply to comment #21)
> (From update of attachment 116999 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116999&action=review
> 
> >> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:148
> >> +    m_pendingTasks.append(createCallbackTask(&ThreadableWebSocketChannelClientWrapper::didUpdateBufferedAmountCallback, AllowCrossThreadAccess(this), bufferedAmount));
> > 
> > Please remove AllowCrossThreadAccess :).
> 
> The change you reviewed in 73336 follows this change and will overwrite this.
> I decide to split removing AllowCrossThreadAccess change to 73336.

Oh, thanks. I thought it was just for existing items.... ok, I'm done here. I was just looking at some threaded related items.

[Kent Tamura]

Comment on attachment 117104 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117104&action=review

> Source/WebCore/websockets/WebSocket.cpp:510
> +    LOG(Network, "WebSocket %p didUpdateBufferedAmount %d", this, bufferedAmount);

Why %d?  I think it should be %lu.

[Takashi Toyoshima]

Comment on attachment 117104 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117104&action=review

> Source/WebCore/platform/network/SocketStreamHandleClient.h:3
> + * Copyright (C) 2011 Google Inc.  All rights reserved.

Fixed.

>> Source/WebCore/websockets/WebSocket.cpp:510
>> +    LOG(Network, "WebSocket %p didUpdateBufferedAmount %d", this, bufferedAmount);
> 
> Why %d?  I think it should be %lu.

Sorry for vague understanding on conversion specifiers.
I fixed it.

[Takashi Toyoshima]

Created attachment 117121 [details]
Patch

[Kent Tamura]

Comment on attachment 117121 [details]
Patch

Looks ok

[WebKit Review Bot]

Comment on attachment 117121 [details]
Patch

Clearing flags on attachment: 117121

Committed r101507: <http://trac.webkit.org/changeset/101507>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.