Bug 98840 – [WebSocket] Add permessage-deflate extension (RFC 7692: Compression Extensions for WebSocket)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

This is a part of updating compression extension (https://bugs.webkit.org/show_bug.cgi?id=98393).

Add an WebSocketExtensionProcessor that handles "permessage-compress" extension.
The draft spec: http://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-01

This extension processor compresses/decompresses the payload of websocket messages using deflate algorithm.
The implementation of "permessage-compress" would be similar to WebSocketDeflateFramer. The main difference between them is compression/decompression unit("per-frame" vs "per-message").

[Kenichi Ishibashi]

Created attachment 167893 [details]
Work-In-Progress patch

[Kenichi Ishibashi]

(Cc yutak, tyoshino)

Could you take a quick look? I don't ask a formal review at this time, but I'd share what I'm going to add. Feedback are highly welcome. (esp. naming of classes and methods)

[Takeshi Yoshino]

Comment on attachment 167893 [details]
Work-In-Progress patch

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

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:66
> +// FXIME: Remove vendor prefix after the specification matured.

FIXME?

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:251
> +    m_compressOngoing = !frame.final;

How about using a flag say m_isFirstFragment that is set to true when the frame is the first fragment of a message?

[Yuta Kitamura]

Comment on attachment 167893 [details]
Work-In-Progress patch

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

I skimmed the code, and I think it's generally okay.

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:138
> +    if (expectedNumParameters != methodParameters.size()) {

Don't you need to take care of "s2c_max_window_bits" and "s2c_no_context_takeover" here?

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:243
> +    if (frame.final && !m_deflater->finish()) {

This code depends on WebSocketChannel's behavior that it does not fragment a message
to make compression context takeover mode work correctly. Am I correct?

If WebSocketChannel fragmented a message, Deflater would misbehave because
it is not aware of message boundary.

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.h:48
> +class CompressResultHolder {

"WebSocket" should be prefixed to the class name (to avoid name collision), or this
class should be an inner class of WebSocketMessageCompression.

I think this class is simple enough and a struct may be a better fit. Returning a
dynamically allocated object as a function result looks too heavyweight,
so just returning a result as a copy would be just fine.

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.h:71
> +class DecompressResultHolder {

Ditto.

> Source/WebCore/Modules/websockets/WebSocketMessageCompression.h:94
> +class WebSocketMessageCompression {

"Compression" may sound strange as a name for this class...

I can think of:
- WebSocketMessageCompressor (but it does decompression, too)
- WebSocketCompressionManager (but "Manager" may sound vague)
- WebSocketPerMessageCompression (ends with "Compression" but matches the extension name, so this may be okay)

I'm open to your sugggestions.

[Kenichi Ishibashi]

Comment on attachment 167893 [details]
Work-In-Progress patch

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

Thanks you for the comments!

>> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:66
>> +// FXIME: Remove vendor prefix after the specification matured.
> 
> FIXME?

Yes. Will fix.

>> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:138
>> +    if (expectedNumParameters != methodParameters.size()) {
> 
> Don't you need to take care of "s2c_max_window_bits" and "s2c_no_context_takeover" here?

No. We don't pass s2c_max_window_bits and s2c_no_context_takeover at opening handshake. In that case, the server should not include these parameters in the response.

>> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:243
>> +    if (frame.final && !m_deflater->finish()) {
> 
> This code depends on WebSocketChannel's behavior that it does not fragment a message
> to make compression context takeover mode work correctly. Am I correct?
> 
> If WebSocketChannel fragmented a message, Deflater would misbehave because
> it is not aware of message boundary.

I intended that the code doesn't depend on WebSocketChannel's behavior. We should call m_deflater->finish() only for the last fragmented frame. I might misunderstand the spec, but if frame.final is true, then the frame should be the last fragmented frame of a message.

>> Source/WebCore/Modules/websockets/WebSocketMessageCompression.cpp:251
>> +    m_compressOngoing = !frame.final;
> 
> How about using a flag say m_isFirstFragment that is set to true when the frame is the first fragment of a message?

Hmm, it requires more changes in WebSocketChannel. WebSocketChannel will need to be aware of what this extension does in more detail.

The code actually a bit tricky. I'll think a bit more about this.

>> Source/WebCore/Modules/websockets/WebSocketMessageCompression.h:48
>> +class CompressResultHolder {
> 
> "WebSocket" should be prefixed to the class name (to avoid name collision), or this
> class should be an inner class of WebSocketMessageCompression.
> 
> I think this class is simple enough and a struct may be a better fit. Returning a
> dynamically allocated object as a function result looks too heavyweight,
> so just returning a result as a copy would be just fine.

Will do.

>> Source/WebCore/Modules/websockets/WebSocketMessageCompression.h:94
>> +class WebSocketMessageCompression {
> 
> "Compression" may sound strange as a name for this class...
> 
> I can think of:
> - WebSocketMessageCompressor (but it does decompression, too)
> - WebSocketCompressionManager (but "Manager" may sound vague)
> - WebSocketPerMessageCompression (ends with "Compression" but matches the extension name, so this may be okay)
> 
> I'm open to your sugggestions.

Thanks. I'd prefer the last one. We might want to name it as WebSocketPerMessageCompressionExtensionProcessor, but it too long.

[Kenichi Ishibashi]

Created attachment 168147 [details]
Patch

[Kenichi Ishibashi]

Created attachment 168148 [details]
Tests to be added after this patch

[Build Bot]

Comment on attachment 168147 [details]
Patch

Attachment 168147 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14249675

[Kenichi Ishibashi]

Hi Yuta-san,

Could you take another look? This patch only for adding the new compression extension processor(Doesn't change behavior). I attached tests which will be added after we switch the extension.

As for flags in WebSocketPerMessageCompression, I didn't change the name and usage. I'd like to keep WebSocketChannel from being aware of what extensions does as much as possible.

[Kenichi Ishibashi]

Created attachment 168168 [details]
Fix compile error on clang

[Yuta Kitamura]

Comment on attachment 168168 [details]
Fix compile error on clang

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

r-'ing because of style and design issues.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:48
> +    static PassOwnPtr<CompressionMessageExtensionProcessor> create(WebSocketPerMessageCompression* compress)

|compress| is not optional, right? If so, I would prefer taking as a reference to a pointer. "ASSERT(m_compress)" will no longer be necessary.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:59
> +    CompressionMessageExtensionProcessor(WebSocketPerMessageCompression*);

explicit

And also I'd want a reference here, too.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:66
> +// FIXME: Remove vendor prefix after the specification matured.

nit: "matured" -> "has matured" or "matures".

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:110
> +        m_failureReason = "Method of permessage-compress does not match";

This message can be more friendly by saying that we only support "deflate" method and by putting the received |methodName| into the reason text.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:114
> +    int expectedNumParameters = 0;

nit: Abbreviation like "Num" is not desirable.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:118
> +        windowBits = parameter->value.toInt();

IIRC String::toInt() ignores the trailing garbage text (e.g. "15abcd" would be parsed as 15).

It's also good to explicitly handle the case of null |parameter->value|. In that case toInt() returns 0, so the check will fail anyway, but this is too sutble and easy to get missed in the future code change.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:159
> +    m_compress->resetCompressBuffer();

I don't get the idea why ResultHolder's destructor needs to reset the (de)compression status. And I find this behavior very unintuitive. I think reset(De)CompressBuffer() should be called explicitly by the user of WebSocketPerMessageCompression.

If you can remove this line, then m_compress is no longer necessary, and thus ResultHolder will become much simpler. You may even be able to remove two ResultHolder classes by fiddling with the function signature of (de)compress().

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:277
> +            result.fail("Received a frame that sets compressed bit during another decompression is ongoing");

nit: during -> while

[Kent Tamura]

Comment on attachment 168168 [details]
Fix compile error on clang

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

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:14
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its

Please use two-clauses version of copyright header.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:46
> +    WTF_MAKE_FAST_ALLOCATED;

Are objects of this class copyable? If not, we had better add WTF_MAKE_NONCOPYABLE(CompressionMessageExtensionProcessor);.

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:68
> +    : WebSocketExtensionProcessor("x-webkit-permessage-compress")

should be
 : WebSocketExtensionProcessor(ASCIILiteral("x-webkit-permessage-compress"))

See http://trac.webkit.org/wiki/EfficientStrings

> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:78
> +    return "x-webkit-permessage-compress; method=deflate";

Should be
   return ASCIILiteral("x-webkit-permessage-compress; method=deflate");

See http://trac.webkit.org/wiki/EfficientStrings

[Kenichi Ishibashi]

Comment on attachment 168168 [details]
Fix compile error on clang

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

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:48
>> +    static PassOwnPtr<CompressionMessageExtensionProcessor> create(WebSocketPerMessageCompression* compress)
> 
> |compress| is not optional, right? If so, I would prefer taking as a reference to a pointer. "ASSERT(m_compress)" will no longer be necessary.

Ok, will do.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:59
>> +    CompressionMessageExtensionProcessor(WebSocketPerMessageCompression*);
> 
> explicit
> 
> And also I'd want a reference here, too.

Will do.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:66
>> +// FIXME: Remove vendor prefix after the specification matured.
> 
> nit: "matured" -> "has matured" or "matures".

Will do.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:110
>> +        m_failureReason = "Method of permessage-compress does not match";
> 
> This message can be more friendly by saying that we only support "deflate" method and by putting the received |methodName| into the reason text.

Will do.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:114
>> +    int expectedNumParameters = 0;
> 
> nit: Abbreviation like "Num" is not desirable.

Will fix.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:118
>> +        windowBits = parameter->value.toInt();
> 
> IIRC String::toInt() ignores the trailing garbage text (e.g. "15abcd" would be parsed as 15).
> 
> It's also good to explicitly handle the case of null |parameter->value|. In that case toInt() returns 0, so the check will fail anyway, but this is too sutble and easy to get missed in the future code change.

Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:159
>> +    m_compress->resetCompressBuffer();
> 
> I don't get the idea why ResultHolder's destructor needs to reset the (de)compression status. And I find this behavior very unintuitive. I think reset(De)CompressBuffer() should be called explicitly by the user of WebSocketPerMessageCompression.
> 
> If you can remove this line, then m_compress is no longer necessary, and thus ResultHolder will become much simpler. You may even be able to remove two ResultHolder classes by fiddling with the function signature of (de)compress().

If we don't clear the buffer, we will occupy the buffer even it isn't used. This can be a problem when a websocket sent a huge message (and becomes idle).

The ResultHolder's are a kind of RAII class. They ensure that reset{Dec,C}ompressBuffer() get called. We can call reset{Dec,C}ompressBuffer() directly in WebSocketChannel, but in that way, WebSocketChannel becomes very fragile for changes. We will need to be very careful when we want to modify WebSocketChannel (so that these methods are certainly called) in the future.

I don't have strong opinion here. I'm fine removing these classes. What do you think?

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:277
>> +            result.fail("Received a frame that sets compressed bit during another decompression is ongoing");
> 
> nit: during -> while

Will do.

[Yuta Kitamura]

Comment on attachment 168168 [details]
Fix compile error on clang

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

>>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:118
>>> +        windowBits = parameter->value.toInt();
>> 
>> IIRC String::toInt() ignores the trailing garbage text (e.g. "15abcd" would be parsed as 15).
>> 
>> It's also good to explicitly handle the case of null |parameter->value|. In that case toInt() returns 0, so the check will fail anyway, but this is too sutble and easy to get missed in the future code change.
> 
> Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.

Re null case, I'm aware of such input is rejected below. My point is that it's not very obvious whether this is intentional. When you or someone else tries to update the code in the future, he or she will easily miss this case.

I believe there should be a comment at least. I think it's better to handle that case explicitly.

>>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:159
>>> +    m_compress->resetCompressBuffer();
>> 
>> I don't get the idea why ResultHolder's destructor needs to reset the (de)compression status. And I find this behavior very unintuitive. I think reset(De)CompressBuffer() should be called explicitly by the user of WebSocketPerMessageCompression.
>> 
>> If you can remove this line, then m_compress is no longer necessary, and thus ResultHolder will become much simpler. You may even be able to remove two ResultHolder classes by fiddling with the function signature of (de)compress().
> 
> If we don't clear the buffer, we will occupy the buffer even it isn't used. This can be a problem when a websocket sent a huge message (and becomes idle).
> 
> The ResultHolder's are a kind of RAII class. They ensure that reset{Dec,C}ompressBuffer() get called. We can call reset{Dec,C}ompressBuffer() directly in WebSocketChannel, but in that way, WebSocketChannel becomes very fragile for changes. We will need to be very careful when we want to modify WebSocketChannel (so that these methods are certainly called) in the future.
> 
> I don't have strong opinion here. I'm fine removing these classes. What do you think?

At the very least the class name "*ResultHolder" is not appropriate for classes that do nontrivial stuff like that. You should give a name that reflects the behavior, from which it's clear that the class resets the (de)compressor's context in the destructor.

Come to think of it, I started to think this RAII behavior and result holding functionality do not need to be combined together. It's probably clear to have classes that only reset context in the destructor, and (if necessary) classes (or structs) that hold the result of (de)compression.

[Kenichi Ishibashi]

Created attachment 168905 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 168168 [details]
Fix compile error on clang

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

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:14
>> + *     * Neither the name of Google Inc. nor the names of its
> 
> Please use two-clauses version of copyright header.

Done.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:46
>> +    WTF_MAKE_FAST_ALLOCATED;
> 
> Are objects of this class copyable? If not, we had better add WTF_MAKE_NONCOPYABLE(CompressionMessageExtensionProcessor);.

Done.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:68
>> +    : WebSocketExtensionProcessor("x-webkit-permessage-compress")
> 
> should be
>  : WebSocketExtensionProcessor(ASCIILiteral("x-webkit-permessage-compress"))
> 
> See http://trac.webkit.org/wiki/EfficientStrings

Done. Thanks for letting me know it.

>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:78
>> +    return "x-webkit-permessage-compress; method=deflate";
> 
> Should be
>    return ASCIILiteral("x-webkit-permessage-compress; method=deflate");
> 
> See http://trac.webkit.org/wiki/EfficientStrings

Done.

>>>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:118
>>>> +        windowBits = parameter->value.toInt();
>>> 
>>> IIRC String::toInt() ignores the trailing garbage text (e.g. "15abcd" would be parsed as 15).
>>> 
>>> It's also good to explicitly handle the case of null |parameter->value|. In that case toInt() returns 0, so the check will fail anyway, but this is too sutble and easy to get missed in the future code change.
>> 
>> Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.
> 
> Re null case, I'm aware of such input is rejected below. My point is that it's not very obvious whether this is intentional. When you or someone else tries to update the code in the future, he or she will easily miss this case.
> 
> I believe there should be a comment at least. I think it's better to handle that case explicitly.

I think the check is obvious and they won't make a mistake. (Do you think a null string can be a value in the range of 8-15?)
Sorry, but I would prefer not adding such a obvious comment to follow the WebKit way.

>>>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:159
>>>> +    m_compress->resetCompressBuffer();
>>> 
>>> I don't get the idea why ResultHolder's destructor needs to reset the (de)compression status. And I find this behavior very unintuitive. I think reset(De)CompressBuffer() should be called explicitly by the user of WebSocketPerMessageCompression.
>>> 
>>> If you can remove this line, then m_compress is no longer necessary, and thus ResultHolder will become much simpler. You may even be able to remove two ResultHolder classes by fiddling with the function signature of (de)compress().
>> 
>> If we don't clear the buffer, we will occupy the buffer even it isn't used. This can be a problem when a websocket sent a huge message (and becomes idle).
>> 
>> The ResultHolder's are a kind of RAII class. They ensure that reset{Dec,C}ompressBuffer() get called. We can call reset{Dec,C}ompressBuffer() directly in WebSocketChannel, but in that way, WebSocketChannel becomes very fragile for changes. We will need to be very careful when we want to modify WebSocketChannel (so that these methods are certainly called) in the future.
>> 
>> I don't have strong opinion here. I'm fine removing these classes. What do you think?
> 
> At the very least the class name "*ResultHolder" is not appropriate for classes that do nontrivial stuff like that. You should give a name that reflects the behavior, from which it's clear that the class resets the (de)compressor's context in the destructor.
> 
> Come to think of it, I started to think this RAII behavior and result holding functionality do not need to be combined together. It's probably clear to have classes that only reset context in the destructor, and (if necessary) classes (or structs) that hold the result of (de)compression.

Done.

[Yuta Kitamura]

Comment on attachment 168168 [details]
Fix compile error on clang

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

>>>>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:118
>>>>> +        windowBits = parameter->value.toInt();
>>>> 
>>>> IIRC String::toInt() ignores the trailing garbage text (e.g. "15abcd" would be parsed as 15).
>>>> 
>>>> It's also good to explicitly handle the case of null |parameter->value|. In that case toInt() returns 0, so the check will fail anyway, but this is too sutble and easy to get missed in the future code change.
>>> 
>>> Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.
>> 
>> Re null case, I'm aware of such input is rejected below. My point is that it's not very obvious whether this is intentional. When you or someone else tries to update the code in the future, he or she will easily miss this case.
>> 
>> I believe there should be a comment at least. I think it's better to handle that case explicitly.
> 
> I think the check is obvious and they won't make a mistake. (Do you think a null string can be a value in the range of 8-15?)
> Sorry, but I would prefer not adding such a obvious comment to follow the WebKit way.

I don't agree at all. This code is not obvious. I can easily imagine that, if, for example, 0 becomes a valid value as windowBits, I would fail to handle the null case correctly. You are mixing the null case and valid 0 implicitly. This code depends on how toInt() behaves on error, which isn't obvious from this snippet of code (how do you know the error value is not INT_MIN or INT_MAX?).

More importantly, this code makes you look as if you forgot to handle the null case. This was my first impression. Please let the code insist that you are aware of it.

I would say that it's even better if we have a different m_failureReason message for the null case (= no option parameter).

[Kenichi Ishibashi]

Comment on attachment 168168 [details]
Fix compile error on clang

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

>>>>>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:118
>>>>>> +        windowBits = parameter->value.toInt();
>>>>> 
>>>>> IIRC String::toInt() ignores the trailing garbage text (e.g. "15abcd" would be parsed as 15).
>>>>> 
>>>>> It's also good to explicitly handle the case of null |parameter->value|. In that case toInt() returns 0, so the check will fail anyway, but this is too sutble and easy to get missed in the future code change.
>>>> 
>>>> Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.
>>> 
>>> Re null case, I'm aware of such input is rejected below. My point is that it's not very obvious whether this is intentional. When you or someone else tries to update the code in the future, he or she will easily miss this case.
>>> 
>>> I believe there should be a comment at least. I think it's better to handle that case explicitly.
>> 
>> I think the check is obvious and they won't make a mistake. (Do you think a null string can be a value in the range of 8-15?)
>> Sorry, but I would prefer not adding such a obvious comment to follow the WebKit way.
> 
> I don't agree at all. This code is not obvious. I can easily imagine that, if, for example, 0 becomes a valid value as windowBits, I would fail to handle the null case correctly. You are mixing the null case and valid 0 implicitly. This code depends on how toInt() behaves on error, which isn't obvious from this snippet of code (how do you know the error value is not INT_MIN or INT_MAX?).
> 
> More importantly, this code makes you look as if you forgot to handle the null case. This was my first impression. Please let the code insist that you are aware of it.
> 
> I would say that it's even better if we have a different m_failureReason message for the null case (= no option parameter).

No, you can't imagine the value could be 0. Please refer /usr/include/zlib.h windowBits must be in the range 8..15.

Also please take a look at the newest patch. I think the check is obvious.

[Yuta Kitamura]

Comment on attachment 168168 [details]
Fix compile error on clang

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

>>>>>>> Source/WebCore/Modules/websockets/WebSocketPerMessageCompression.cpp:118
>>>>>>> +        windowBits = parameter->value.toInt();
>>>>>> 
>>>>>> IIRC String::toInt() ignores the trailing garbage text (e.g. "15abcd" would be parsed as 15).
>>>>>> 
>>>>>> It's also good to explicitly handle the case of null |parameter->value|. In that case toInt() returns 0, so the check will fail anyway, but this is too sutble and easy to get missed in the future code change.
>>>>> 
>>>>> Thanks, I didn't realize the case. Will handle trailing text. But I don't think we need to handle the case of null because following check rejects such case.
>>>> 
>>>> Re null case, I'm aware of such input is rejected below. My point is that it's not very obvious whether this is intentional. When you or someone else tries to update the code in the future, he or she will easily miss this case.
>>>> 
>>>> I believe there should be a comment at least. I think it's better to handle that case explicitly.
>>> 
>>> I think the check is obvious and they won't make a mistake. (Do you think a null string can be a value in the range of 8-15?)
>>> Sorry, but I would prefer not adding such a obvious comment to follow the WebKit way.
>> 
>> I don't agree at all. This code is not obvious. I can easily imagine that, if, for example, 0 becomes a valid value as windowBits, I would fail to handle the null case correctly. You are mixing the null case and valid 0 implicitly. This code depends on how toInt() behaves on error, which isn't obvious from this snippet of code (how do you know the error value is not INT_MIN or INT_MAX?).
>> 
>> More importantly, this code makes you look as if you forgot to handle the null case. This was my first impression. Please let the code insist that you are aware of it.
>> 
>> I would say that it's even better if we have a different m_failureReason message for the null case (= no option parameter).
> 
> No, you can't imagine the value could be 0. Please refer /usr/include/zlib.h windowBits must be in the range 8..15.
> 
> Also please take a look at the newest patch. I think the check is obvious.

I was speaking about an imaginary spec change. I would fail to update the code if such modification was made.

[Kenichi Ishibashi]

Created attachment 169296 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #20)
> Created an attachment (id=169296) [details]
> Patch

PTAL?

[Kenichi Ishibashi]

Created attachment 169815 [details]
Follow -04 version

[Lamarque V. Souza]

Is there anybody still working on this patch?

[Lamarque V. Souza]

Created attachment 200147 [details]
Patch

Patch rebased on top of current git revision. I have updated the unit tests as well, only permessage-compress-parameter.html (former deflate-frame-parameter.html) still does not work properly because WebSocket.extensions does not return method parameters (c2s_max_window_bits or c2s_no_context_takeover for instance), only server parameters (method=deflate). CompressionMessageExtensionProcessor::processResponse() is the place where server parameters and method parameters are parsed, with deflate-framer extension they are mixed, now they are split but there is no javascript method that can return method parameters so they can be unit tested in javascript. This patch is a working in progress, the ChangeLog still needs updating and I would like someone else to review the usage of m_perMessageCompression.resetCompressBuffer() and m_perMessageCompression.resetDecompressBuffer() in WebSocketChannel.cpp, specially if there are any other place they are needed.

[Build Bot]

Comment on attachment 200147 [details]
Patch

Attachment 200147 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/281009

New failing tests:
http/tests/websocket/tests/hybi/permessage-compress-parameter.html

[Build Bot]

Created attachment 200195 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Lamarque V. Souza]

Created attachment 201282 [details]
Patch

Port of the patch in https://code.google.com/p/chromium/issues/detail?id=229290, which supports -08 version.

[Lamarque V. Souza]

Created attachment 202471 [details]
Patch

Rebase and add unit tests.

[Build Bot]

Comment on attachment 202471 [details]
Patch

Attachment 202471 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/534011

[Build Bot]

Comment on attachment 202471 [details]
Patch

Attachment 202471 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/527196

[Lamarque V. Souza]

Created attachment 202484 [details]
Patch

Attempt to fix compilation on Mac platform.

[Anders Carlsson]

Comment on attachment 169815 [details]
Follow -04 version

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Brady Eidson]

Comment on attachment 202484 [details]
Patch

r- to clear stale patches from the review queue

[Sam Sneddon [:gsnedders]]

This eventually got published as https://datatracker.ietf.org/doc/html/rfc7692.

Notably, it seems like the per-frame extension (which we support, prefixed) got replaced by this: https://datatracker.ietf.org/doc/draft-tyoshino-hybi-permessage-compression/

[Radar WebKit Bug Importer]

<rdar://problem/96339750>

[Sam Sneddon [:gsnedders]]

Actually, this isn't relevant on the Apple ports any more due to the adoption of NSURLSession's WebSocket support, is it? (Except, I guess, on Big Sur, where we default to not using NSURLSession.)

And the other two network-layer platforms (CURL and SOUP) don't use the legacy WebSocket implementation either as far as I can tell, which means practically this can be closed.

Should if that's wrong!