Bug 78079

Created attachment 126007 [details] Patch

(In reply to comment #1) > Created an attachment (id=126007) [details] > Patch It's difficult to write layout tests because there is no behavior change so far. I added a chromium's unit test for server response parsing. It's port specific, but it would be better than not writing tests.

Comment on attachment 126007 [details] Patch Attachment 126007 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11460275

Created attachment 126012 [details] Patch

Comment on attachment 126007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126007&action=review [My comments are only informational as I'm not a WebKit reviewer yet.] > Source/WebCore/websockets/WebSocketHandshake.cpp:707 > + if (!m_extensions.processHeaderValue(serverWebSocketExtensions)) { optional: If you could provide detailed reason of failure, the script authors will be able to diagnose their issues better. > Source/WebCore/websockets/WebSocketHandshakeResponse.cpp:91 > + // they are connected with commna. typo: commna > Source/WebCore/websockets/WebSocketHandshakeResponse.cpp:96 > + HTTPHeaderMap::iterator iterator = m_headerFields.find(name); > + if (iterator != m_headerFields.end() && equalIgnoringCase(iterator->first, "sec-websocket-extensions")) > + m_headerFields.set(name, iterator->second + "," + value); > + else > + m_headerFields.add(name, value); It may be a bad idea to simply concatenate all header values with commas. Consider the following (broken) case: Sec-WebSocket-Extensions: foo; param="bar Sec-WebSocket-Extensions: " We want to signal an error when we receive headers like this, instead of treating these two lines as a valid extension line 'foo; param="bar,"'. > Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:1 > +/* Maybe you could make use of TestWebKitAPI to write unit tests like this. I'm not sure which (TestWebKitAPI or Chromium tests) is better for this purpose, though. > Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:133 > + HashMap<String, String>& expectedParameters = expected[i].parameters; > + HashMap<String, String>& parsedParameters = m_parsedParameters[i]; const reference should be better.

Created attachment 126036 [details] Patch

Comment on attachment 126007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126007&action=review Thank you for review! >> Source/WebCore/websockets/WebSocketHandshake.cpp:707 >> + if (!m_extensions.processHeaderValue(serverWebSocketExtensions)) { > > optional: If you could provide detailed reason of failure, the script authors will be able to diagnose their issues better. Added WebSocketExtensions::failureReason(). It will be called if WebSocketExtensions::processHeaderValue() fails. >> Source/WebCore/websockets/WebSocketHandshakeResponse.cpp:91 >> + // they are connected with commna. > > typo: commna Removed. See below. >> Source/WebCore/websockets/WebSocketHandshakeResponse.cpp:96 >> + m_headerFields.add(name, value); > > It may be a bad idea to simply concatenate all header values with commas. Consider the following (broken) case: > > Sec-WebSocket-Extensions: foo; param="bar > Sec-WebSocket-Extensions: " > > We want to signal an error when we receive headers like this, instead of treating these two lines as a valid extension line 'foo; param="bar,"'. Thank you for pointing this out. Removed this change and revised the patch to parse and check the header value every time the header appears. The revised patch also removes WebSocketHandshake::serverWebSocketExtensions(), which is no longer needed. >> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:1 >> +/* > > Maybe you could make use of TestWebKitAPI to write unit tests like this. I'm not sure which (TestWebKitAPI or Chromium tests) is better for this purpose, though. Didn't move the test into TestWebKitAPI for now. If TestWebKitAPI is better for this test, I'll move it. >> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:133 >> + HashMap<String, String>& parsedParameters = m_parsedParameters[i]; > > const reference should be better. Done.

Comment on attachment 126036 [details] Patch Attachment 126036 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11460366 New failing tests: http/tests/websocket/tests/hybi/handshake-fail-by-extensions-header.html

Created attachment 126186 [details] added test rebaseline

Comment on attachment 126186 [details] added test rebaseline Attachment 126186 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11463664 New failing tests: platform/chromium/compositing/layout-width-change.html

Hi Alexey, Kent-san, Could you please review the patch?

Comment on attachment 126186 [details] added test rebaseline View in context: https://bugs.webkit.org/attachment.cgi?id=126186&action=review > Source/WebCore/websockets/WebSocketExtensions.cpp:58 > + void consumeSpaces(); The name should not start with "consume", and should be skipSpaces() or something because other consumeFoo() functions return a bool value. > Source/WebCore/websockets/WebSocketExtensions.cpp:60 > + bool isControl(char); > + bool isSeparator(char); They should be static function members, or static functions which are not members of ExtensionParser. > Source/WebCore/websockets/WebSocketExtensions.cpp:77 > +// These functions basically follow the grammer defined in Section 2.2 of RFC 2616. "These" is unclear. > Source/WebCore/websockets/WebSocketExtensions.cpp:97 > + for (const char* current = separatorCharacters; *current; ++current) { > + if (*current == character) > + return true; > + } > + return false; char* p = strchr(separatorCharacters, character); return p && *p; is enough. > Source/WebCore/websockets/WebSocketExtensions.cpp:107 > + m_currentToken = String(start, m_current - start); What's the encoding of the token? > Source/WebCore/websockets/WebSocketExtensions.cpp:151 > + m_extensions.append(adoptPtr(new Extension(processor))); > +} We should have an assertion of validity of processor->handshakeString(). > Source/WebCore/websockets/WebSocketExtensions.cpp:173 > + // If we don't send Sec-WebSocket-Extensions header, the server should not return the header A comment should end with '.' > Source/WebCore/websockets/WebSocketExtensions.cpp:183 > + // Parse extension-token ditto. > Source/WebCore/websockets/WebSocketExtensions.cpp:185 > + m_failureReason = "Sec-WebSocket-Extensions header is invalid"; Should it end with '.'? > Source/WebCore/websockets/WebSocketExtensions.cpp:190 > + // Parse extension-parameters if exists A comment should end with '.' > Source/WebCore/websockets/WebSocketExtensions.cpp:200 > + if (parser.consumeQuotedString() || parser.consumeToken()) I know this code works correctly now, but I needed to read the content of consumeQuotedString() carefully in order to confirm consumeQuotedString() never fail with unexpected ExtensionParser::m_current. I recommend introduce ExtensionParser::consumeQuotedStringOrToken(). > Source/WebCore/websockets/WebSocketExtensions.cpp:225 > + // There is no extension which can process the response A comment should end with '.' > Source/WebCore/websockets/WebSocketExtensions.h:44 > +class WebSocketExtensionProcessor { The class should be moved to its own file. An implementation of WebSocketExtensionProcessor doesn't need to know about WebSocketExtensions class. > Source/WebCore/websockets/WebSocketExtensions.h:48 > + const String extensionToken() const { return m_extensionToken; } Whey it returns "const String"? > Source/WebCore/websockets/WebSocketExtensions.h:71 > +class WebSocketExtensions { I don't like the name "WebSocketExtensions". It's not a simple container of extensions. Maybe "WebSocketExtensionController", "WebSocketExtensionDispatcher", or something. > Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:47 > + virtual const String handshakeString() { return extensionToken(); } > + virtual bool processResponse(const HashMap<String, String>&); should append OVERRIDE. > Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:50 > + The blank line is not needed. > Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:66 > + WebSocketExtensionsTest() > + { > + } > + > + void SetUp() > + { > + } > + > + void TearDown() > + { > + } You may write like WebSocketExtensionTest() { } > Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:71 > + THe blank line is not needed.

Comment on attachment 126186 [details] added test rebaseline View in context: https://bugs.webkit.org/attachment.cgi?id=126186&action=review Looks almost fine to me. > Source/WebCore/websockets/WebSocketExtensions.cpp:224 > + for (index = 0; index < m_extensions.size(); ++index) { > + Extension* extension = m_extensions[index].get(); > + if (equalIgnoringCase(extensionToken, extension->m_processor->extensionToken()) && !extension->m_responseHandled) { > + extension->m_responseHandled = true; > + if (extension->m_processor->processResponse(extensionParameters)) > + break; > + m_failureReason = extension->m_processor->failureReason(); > + return false; > + } > + } I have two questions: (1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"? (2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it).

Created attachment 126255 [details] Patch

Comment on attachment 126186 [details] added test rebaseline View in context: https://bugs.webkit.org/attachment.cgi?id=126186&action=review Hi Kent-san, Yuta-san, Thank you for review! Addressed your comments. >> Source/WebCore/websockets/WebSocketExtensions.cpp:58 >> + void consumeSpaces(); > > The name should not start with "consume", and should be skipSpaces() or something because other consumeFoo() functions return a bool value. Renamed as skipSpaces(). >> Source/WebCore/websockets/WebSocketExtensions.cpp:60 >> + bool isSeparator(char); > > They should be static function members, or static functions which are not members of ExtensionParser. These functions integrated into a static function isTokenCharacter(). >> Source/WebCore/websockets/WebSocketExtensions.cpp:77 >> +// These functions basically follow the grammer defined in Section 2.2 of RFC 2616. > > "These" is unclear. Moved the comment in the declaration of ExtensionParser class. >> Source/WebCore/websockets/WebSocketExtensions.cpp:97 >> + return false; > > char* p = strchr(separatorCharacters, character); > return p && *p; > > is enough. Done. >> Source/WebCore/websockets/WebSocketExtensions.cpp:107 >> + m_currentToken = String(start, m_current - start); > > What's the encoding of the token? The spec refers http://tools.ietf.org/html/rfc2616#section-4.2, and it says token consists of any US-ASCII characters (0 - 127) except for control characters (0 - 31 and 127). We can assume the encoding is ASCII. >> Source/WebCore/websockets/WebSocketExtensions.cpp:151 >> +} > > We should have an assertion of validity of processor->handshakeString(). Done. >> Source/WebCore/websockets/WebSocketExtensions.cpp:173 >> + // If we don't send Sec-WebSocket-Extensions header, the server should not return the header > > A comment should end with '.' Done. >> Source/WebCore/websockets/WebSocketExtensions.cpp:183 >> + // Parse extension-token > > ditto. Done. >> Source/WebCore/websockets/WebSocketExtensions.cpp:185 >> + m_failureReason = "Sec-WebSocket-Extensions header is invalid"; > > Should it end with '.'? Other failure messages in WebSocketHandshake class don't end with '.' so I don't append '.' here. I think we should keep consistency. >> Source/WebCore/websockets/WebSocketExtensions.cpp:190 >> + // Parse extension-parameters if exists > > A comment should end with '.' Done. >> Source/WebCore/websockets/WebSocketExtensions.cpp:200 >> + if (parser.consumeQuotedString() || parser.consumeToken()) > > I know this code works correctly now, but I needed to read the content of consumeQuotedString() carefully in order to confirm consumeQuotedString() never fail with unexpected ExtensionParser::m_current. I recommend introduce ExtensionParser::consumeQuotedStringOrToken(). Done. >> Source/WebCore/websockets/WebSocketExtensions.cpp:224 >> + } > > I have two questions: > > (1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"? > (2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it). (1) If my reading is correct, RFC6455 allows such case. I think we should accept such response if the extension allows such duplication. The current implementation doesn't allow the case so I changed the behavior. (2) You are right. Changed to just compare the tokens. >> Source/WebCore/websockets/WebSocketExtensions.cpp:225 >> + // There is no extension which can process the response > > A comment should end with '.' Done. >> Source/WebCore/websockets/WebSocketExtensions.h:44 >> +class WebSocketExtensionProcessor { > > The class should be moved to its own file. An implementation of WebSocketExtensionProcessor doesn't need to know about WebSocketExtensions class. Done. >> Source/WebCore/websockets/WebSocketExtensions.h:48 >> + const String extensionToken() const { return m_extensionToken; } > > Whey it returns "const String"? Removed const. >> Source/WebCore/websockets/WebSocketExtensions.h:71 >> +class WebSocketExtensions { > > I don't like the name "WebSocketExtensions". It's not a simple container of extensions. > Maybe "WebSocketExtensionController", "WebSocketExtensionDispatcher", or something. Changed the name to WebKitExtensionDispatcher. >> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:47 >> + virtual bool processResponse(const HashMap<String, String>&); > > should append OVERRIDE. Done. >> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:50 >> + > > The blank line is not needed. Done. >> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:66 >> + } > > You may write like > WebSocketExtensionTest() { } Done. >> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:71 >> + > > THe blank line is not needed. Done.

Comment on attachment 126255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126255&action=review > Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:60 > + bool consumeCharacter(char); > +private: need a blank line before private:. > Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:81 > + // A token character is a CHAR (octets 0 - 127) but not CTL (octets 0 - 31 and 127) > + return character > 31 && character < 127; This looks same as isASCIIPrintable() in wtf/ASCIIType.h. > Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:132 > +bool ExtensionParser::consumeQuotedStringOrToken() > +{ > + return consumeQuotedString() || consumeToken(); I recommend adding a comment that consumeToken() works well because consumeQuotedString() doesn't update m_current or makes it same as m_end on failure. > Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:166 > + for (size_t i = 1; i < numProcessors; ++i) { > + builder.append(", "); > + builder.append(m_processors[i]->handshakeString()); What happens if handshakString() contains \n or \0?

Comment on attachment 126255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126255&action=review > Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:121 > + builder.append(*m_current); > + ++m_current; What's the encoding of a quoted string?

Comment on attachment 126186 [details] added test rebaseline View in context: https://bugs.webkit.org/attachment.cgi?id=126186&action=review >>> Source/WebCore/websockets/WebSocketExtensions.cpp:224 >>> + } >> >> I have two questions: >> >> (1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"? >> (2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it). > > (1) If my reading is correct, RFC6455 allows such case. I think we should accept such response if the extension allows such duplication. The current implementation doesn't allow the case so I changed the behavior. > (2) You are right. Changed to just compare the tokens. For (1), how do we treat the case above, "foo;a=1;b=2", "foo;a=1" or "foo;b=2"? How about cases like "bar;x=3, bar;x=4"? This seems like a blackhole that the spec has missed describing, but anyway we need to determine what is the expected behavior for such cases.

Created attachment 126261 [details] Patch

Comment on attachment 126255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126255&action=review Hi kent-san, Thanks for review. >> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:60 >> +private: > > need a blank line before private:. Done. >> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:81 >> + return character > 31 && character < 127; > > This looks same as isASCIIPrintable() in wtf/ASCIIType.h. Removed the function and used isASCIIPrintable(). Thanks! >> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:121 >> + ++m_current; > > What's the encoding of a quoted string? According to http://tools.ietf.org/html/rfc2616#section-2.2, quoted strings MAY contain ISO-8859-1 characters. In general it can contain any octets except for non-printable characters. I changed to use String::fromUTF8() because WebSocketHandshake class uses it to parse header fields. >> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:132 >> + return consumeQuotedString() || consumeToken(); > > I recommend adding a comment that consumeToken() works well because consumeQuotedString() doesn't update m_current or makes it same as m_end on failure. Done. >> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:166 >> + builder.append(m_processors[i]->handshakeString()); > > What happens if handshakString() contains \n or \0? Added assertions to check handshakeString() doesn't contain these characters.

Comment on attachment 126261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126261&action=review > Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:162 > + ASSERT(!headerValue.contains('\n')); > + ASSERT(!headerValue.contains(static_cast<UChar>('\0'))); These assertions should be moved to addProcessor().

Comment on attachment 126186 [details] added test rebaseline View in context: https://bugs.webkit.org/attachment.cgi?id=126186&action=review Hi Yuta-san, >>>> Source/WebCore/websockets/WebSocketExtensions.cpp:224 >>>> + } >>> >>> I have two questions: >>> >>> (1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"? >>> (2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it). >> >> (1) If my reading is correct, RFC6455 allows such case. I think we should accept such response if the extension allows such duplication. The current implementation doesn't allow the case so I changed the behavior. >> (2) You are right. Changed to just compare the tokens. > > For (1), how do we treat the case above, "foo;a=1;b=2", "foo;a=1" or "foo;b=2"? How about cases like "bar;x=3, bar;x=4"? > > This seems like a blackhole that the spec has missed describing, but anyway we need to determine what is the expected behavior for such cases. I agree it's somewhat unclear. For now, I think extensions should determine how to treat these cases. Extensions may reject duplicate response or may override parameters, depending on what the corresponding spec requires. For example, "deflate-frame" extension will reject duplicate response because it uses a reserved frame flag and duplicate response doesn't make sense. The current implementation just delegates validation to extensions.

Created attachment 126264 [details] Patch

Comment on attachment 126261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126261&action=review >> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:162 >> + ASSERT(!headerValue.contains(static_cast<UChar>('\0'))); > > These assertions should be moved to addProcessor(). Done.

Comment on attachment 126264 [details] Patch Ok

Comment on attachment 126264 [details] Patch Thank you!

Comment on attachment 126186 [details] added test rebaseline View in context: https://bugs.webkit.org/attachment.cgi?id=126186&action=review >>>>> Source/WebCore/websockets/WebSocketExtensions.cpp:224 >>>>> + } >>>> >>>> I have two questions: >>>> >>>> (1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"? >>>> (2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it). >>> >>> (1) If my reading is correct, RFC6455 allows such case. I think we should accept such response if the extension allows such duplication. The current implementation doesn't allow the case so I changed the behavior. >>> (2) You are right. Changed to just compare the tokens. >> >> For (1), how do we treat the case above, "foo;a=1;b=2", "foo;a=1" or "foo;b=2"? How about cases like "bar;x=3, bar;x=4"? >> >> This seems like a blackhole that the spec has missed describing, but anyway we need to determine what is the expected behavior for such cases. > > I agree it's somewhat unclear. > > For now, I think extensions should determine how to treat these cases. Extensions may reject duplicate response or may override parameters, depending on what the corresponding spec requires. For example, "deflate-frame" extension will reject duplicate response because it uses a reserved frame flag and duplicate response doesn't make sense. The current implementation just delegates validation to extensions. Hm I'm okay with leaving the validation to each extension, but could you add comments describing this issue?

Comment on attachment 126186 [details] added test rebaseline View in context: https://bugs.webkit.org/attachment.cgi?id=126186&action=review >>>>>> Source/WebCore/websockets/WebSocketExtensions.cpp:224 >>>>>> + } >>>>> >>>>> I have two questions: >>>>> >>>>> (1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"? >>>>> (2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it). >>>> >>>> (1) If my reading is correct, RFC6455 allows such case. I think we should accept such response if the extension allows such duplication. The current implementation doesn't allow the case so I changed the behavior. >>>> (2) You are right. Changed to just compare the tokens. >>> >>> For (1), how do we treat the case above, "foo;a=1;b=2", "foo;a=1" or "foo;b=2"? How about cases like "bar;x=3, bar;x=4"? >>> >>> This seems like a blackhole that the spec has missed describing, but anyway we need to determine what is the expected behavior for such cases. >> >> I agree it's somewhat unclear. >> >> For now, I think extensions should determine how to treat these cases. Extensions may reject duplicate response or may override parameters, depending on what the corresponding spec requires. For example, "deflate-frame" extension will reject duplicate response because it uses a reserved frame flag and duplicate response doesn't make sense. The current implementation just delegates validation to extensions. > > Hm I'm okay with leaving the validation to each extension, but could you add comments describing this issue? I've added a comment on WebSocketExtensionProcessor::processResponse(). If the comment isn't enough, I'll add comments here.

Comment on attachment 126264 [details] Patch Attachment 126264 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11487198 New failing tests: platform/chromium/compositing/layout-width-change.html

Comment on attachment 126264 [details] Patch Rebaselined the test. Trying to cq+ again.

Comment on attachment 126264 [details] Patch Rejecting attachment 126264 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: git/webkit-commit-queue/Source/WebKit/chromium/ui --revision 121041 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 45>At revision 121041. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/11473219

Created attachment 126391 [details] Patch for landing

Comment on attachment 126391 [details] Patch for landing Clearing flags on attachment: 126391 Committed r107365: <http://trac.webkit.org/changeset/107365>

All reviewed patches have been landed. Closing bug.