Bug 38728 - WebSocket: Add WebSocketHandshakeResponse
Summary: WebSocket: Add WebSocketHandshakeResponse
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Yuta Kitamura
URL:
Keywords:
Depends on: 35572
Blocks: 38831 40768
  Show dependency treegraph
 
Reported: 2010-05-07 00:17 PDT by Yuta Kitamura
Modified: 2010-06-23 00:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch FYI (ukai's patch in bug 35572 is required) (20.26 KB, patch)
2010-05-18 00:35 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch (19.68 KB, patch)
2010-05-20 23:01 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch (22.11 KB, patch)
2010-05-27 04:35 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch (conflict resolved for bots' sake) (22.10 KB, patch)
2010-05-27 04:47 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch (Add ChallengeResponse) (23.70 KB, patch)
2010-05-31 02:01 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch (23.75 KB, patch)
2010-05-31 20:58 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch (Add a test) (30.59 KB, patch)
2010-06-08 02:00 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch (Fix case of header names) (31.83 KB, patch)
2010-06-10 03:25 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch (Fix pbxproj file) (32.16 KB, patch)
2010-06-10 22:19 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch (Address ap's comments) (31.97 KB, patch)
2010-06-18 06:25 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch (Address comment #31) (33.17 KB, patch)
2010-06-22 01:43 PDT, Yuta Kitamura
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuta Kitamura 2010-05-07 00:17:20 PDT
In https://bugs.webkit.org/show_bug.cgi?id=34784, WebSocketHandshakeRequest class was added (to help passing handshake information to Web Inspector). We also want WebSocketHandshakeResponse.
Comment 1 Yuta Kitamura 2010-05-07 00:22:23 PDT
I want to wait for the handshake change (bug 35572) to be committed.
Comment 2 Yuta Kitamura 2010-05-18 00:35:00 PDT
Created attachment 56330 [details]
Patch FYI (ukai's patch in bug 35572 is required)
Comment 3 Yuta Kitamura 2010-05-18 00:41:13 PDT
(In reply to comment #2)
> Created an attachment (id=56330) [details]
> Patch FYI (ukai's patch in bug 35572 is required)

Alexey and Ukai-san, could you take a look at this patch?

Thanks,
Comment 4 Fumitoshi Ukai 2010-05-18 23:28:29 PDT
Comment on attachment 56330 [details]
Patch FYI (ukai's patch in bug 35572 is required)


> @@ -313,21 +334,24 @@ void WebSocketHandshake::clearScriptExecutionContext()
>  int WebSocketHandshake::readServerHandshake(const char* header, size_t len)
>  {
>      m_mode = Incomplete;
> +    int statusCode;
> +    String statusText;
>      size_t lineLength;
> -    const String& code = extractResponseCode(header, len, lineLength);
> -    if (code.isNull()) {
> +    if (!parseResponseLine(header, len, statusCode, statusText, lineLength)) {
>          // Just hasn't been received yet.
>          return -1;
>      }
> -    if (code.isEmpty()) {
> +    if (!statusCode) {
>          m_mode = Failed;
>          m_context->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "No response code found: " + trimConsoleMessage(header, lineLength), 0, clientOrigin());
>          return len;
>      }
> -    LOG(Network, "response code: %s", code.utf8().data());
> -    if (code != "101") {
> +    LOG(Network, "response code: %d", statusCode);
> +    m_response.setStatusCode(statusCode);
> +    m_response.setStatusText(statusText);

Don't we want to know whole status line?  (or is status text only ok?)

> +WebSocketHandshakeResponse WebSocketHandshake::serverHandshakeResponse() const

Do we copy WebSocketHandshakeResponse?
Comment 5 Yuta Kitamura 2010-05-18 23:46:16 PDT
(In reply to comment #4)
> > -    LOG(Network, "response code: %s", code.utf8().data());
> > -    if (code != "101") {
> > +    LOG(Network, "response code: %d", statusCode);
> > +    m_response.setStatusCode(statusCode);
> > +    m_response.setStatusText(statusText);
> 
> Don't we want to know whole status line?  (or is status text only ok?)
> 

Current Web Inspector does not display HTTP-version part of the response, so I don't think we need it for now. We can easily add it when it becomes necessary.


> > +WebSocketHandshakeResponse WebSocketHandshake::serverHandshakeResponse() const
> 
> Do we copy WebSocketHandshakeResponse?

Oops, we can return const reference here. I will fix it.
Comment 6 Yuta Kitamura 2010-05-20 23:01:45 PDT
Created attachment 56674 [details]
Patch
Comment 7 Fumitoshi Ukai 2010-05-23 22:52:02 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > > -    LOG(Network, "response code: %s", code.utf8().data());
> > > -    if (code != "101") {
> > > +    LOG(Network, "response code: %d", statusCode);
> > > +    m_response.setStatusCode(statusCode);
> > > +    m_response.setStatusText(statusText);
> > 
> > Don't we want to know whole status line?  (or is status text only ok?)
> > 
> 
> Current Web Inspector does not display HTTP-version part of the response, so I don't think we need it for now. We can easily add it when it becomes necessary.

So, do we need status text?  It's now only for informational and not important for websocket handshake.
I'm also not sure statusCode = 0 is good to indicate bad status code...
 
> @@ -345,6 +369,7 @@ int WebSocketHandshake::readServerHandshake(const char* header, size_t len)
>          m_mode = Failed;
>          return len;
>      }
> +    m_response.setHeaderFields(headers);

It might be better to use m_response's HTTPHeaderMap for readHTTPHeaders, rather than coping HTTPHeaderMap to WebSocketHandshakeResponse.
Comment 8 Yuta Kitamura 2010-05-24 06:15:36 PDT
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > > -    LOG(Network, "response code: %s", code.utf8().data());
> > > > -    if (code != "101") {
> > > > +    LOG(Network, "response code: %d", statusCode);
> > > > +    m_response.setStatusCode(statusCode);
> > > > +    m_response.setStatusText(statusText);
> > > 
> > > Don't we want to know whole status line?  (or is status text only ok?)
> > > 
> > 
> > Current Web Inspector does not display HTTP-version part of the response, so I don't think we need it for now. We can easily add it when it becomes necessary.
> 
> So, do we need status text?  It's now only for informational and not important for websocket handshake.

I thought that Web Inspector sends both status code and status text, but it turned out I was wrong. Web Inspector does not use the status text; the front-end looks up the predefined set of status messages.

So basically I'm open to your suggestion. But I still think it's nice to obtain the reason phrase and show it to users, because the reason phrase cannot be uniquely determined from the status code in some cases (for example, "101 Switching Protocols" from HTTP/1.1 vs "101 WebSocket Protocol Handshake" from WebSocket protocol spec). I'll open a new bug for Web Inspector.

> I'm also not sure statusCode = 0 is good to indicate bad status code...

Do you like -1? Or to rewrite the function in some other way? I don't have a strong opinion, and I can update it as you like. I chose 0 just because 0 is not a valid HTTP status code.

> 
> > @@ -345,6 +369,7 @@ int WebSocketHandshake::readServerHandshake(const char* header, size_t len)
> >          m_mode = Failed;
> >          return len;
> >      }
> > +    m_response.setHeaderFields(headers);
> 
> It might be better to use m_response's HTTPHeaderMap for readHTTPHeaders, rather than coping HTTPHeaderMap to WebSocketHandshakeResponse.

Sure.
Comment 9 Yuta Kitamura 2010-05-27 04:35:24 PDT
Created attachment 57221 [details]
Patch
Comment 10 Yuta Kitamura 2010-05-27 04:41:06 PDT
(In reply to comment #9)
> Created an attachment (id=57221) [details]
> Patch

In regard to HTTP status messages, I did not change the code (still collecting HTTP status message) because Web Inspector now respects an HTTP status message that a server returns (see bug 39595).
Comment 11 Yuta Kitamura 2010-05-27 04:47:05 PDT
Created attachment 57223 [details]
Patch (conflict resolved for bots' sake)
Comment 12 Yuta Kitamura 2010-05-31 02:01:36 PDT
Created attachment 57439 [details]
Patch (Add ChallengeResponse)
Comment 13 Fumitoshi Ukai 2010-05-31 19:11:23 PDT
Comment on attachment 57439 [details]
Patch (Add ChallengeResponse)


> diff --git a/WebCore/websockets/WebSocketHandshakeResponse.h b/WebCore/websockets/WebSocketHandshakeResponse.h

> +   String statusText() const;

return const reference?

> +    ChallengeResponse challengeResponse() const;

return const reference instead of copy of ChallengeResponse?
Comment 14 Yuta Kitamura 2010-05-31 20:15:49 PDT
(In reply to comment #13)
> (From update of attachment 57439 [details])
> 
> > diff --git a/WebCore/websockets/WebSocketHandshakeResponse.h b/WebCore/websockets/WebSocketHandshakeResponse.h
> 
> > +   String statusText() const;
> 
> return const reference?

Sure.

> 
> > +    ChallengeResponse challengeResponse() const;
> 
> return const reference instead of copy of ChallengeResponse?

Sure. (Copying 16 bytes sounds cheap to me, though...)
Comment 15 Yuta Kitamura 2010-05-31 20:58:34 PDT
Created attachment 57514 [details]
Patch
Comment 16 Fumitoshi Ukai 2010-06-01 02:10:44 PDT
Looks good to me.
Comment 17 Shinichiro Hamaji 2010-06-02 23:16:30 PDT
Comment on attachment 57514 [details]
Patch

Putting r- to ask if we really don't need a new test.

WebCore/websockets/WebSocketHandshake.cpp:84
 +      if (statusCodeString.length() != 3) // Status code must consist of three digits.
I'm not sure, but the code around here looks redundant to me. Cannot we just rely on the result of WTFString::toInt ? Specifically

bool ok = false;
statusCode = statusCodeString.toInt(&ok);
if (!ok || statusCode < 100 || statusCode >= 600) {
    statusCode = 0;
    return true;
}

wont' work?


WebCore/websockets/WebSocketHandshakeResponse.h:63
 +      void setChallengeResponse(const unsigned char challengeResponse[16]);
I'd take signed char here so we don't need reinterpret_cast, but it's OK as is.

WebCore/websockets/WebSocketHandshake.cpp:80
 +      if (!space1 || !space2 || *(p - 1) != '\r') // The line must end with "\r\n".
So, now we are rejecting responses like "HTTP/1.0 101 WebSocket Protocol Handshake\n" and before this change we don't reject this? If so, I think it would be better to test this change.
Comment 18 Yuta Kitamura 2010-06-07 04:48:14 PDT
(In reply to comment #17)
> (From update of attachment 57514 [details])
> Putting r- to ask if we really don't need a new test.
> 
> WebCore/websockets/WebSocketHandshake.cpp:84
>  +      if (statusCodeString.length() != 3) // Status code must consist of three digits.
> I'm not sure, but the code around here looks redundant to me. Cannot we just rely on the result of WTFString::toInt ? Specifically
>

Maybe. What I did was a faithful trace of the step 29 to 31 in http://html5.org/complete/network.html#opening-handshake-0.

We need to reject cases like "0101", so I think we need to check the string length at least.

> bool ok = false;
> statusCode = statusCodeString.toInt(&ok);
> if (!ok || statusCode < 100 || statusCode >= 600) {
>     statusCode = 0;
>     return true;
> }
> 
> wont' work?

Oops. Will fix.

> 
> 
> WebCore/websockets/WebSocketHandshakeResponse.h:63
>  +      void setChallengeResponse(const unsigned char challengeResponse[16]);
> I'd take signed char here so we don't need reinterpret_cast, but it's OK as is.
> 
> WebCore/websockets/WebSocketHandshake.cpp:80
>  +      if (!space1 || !space2 || *(p - 1) != '\r') // The line must end with "\r\n".
> So, now we are rejecting responses like "HTTP/1.0 101 WebSocket Protocol Handshake\n" and before this change we don't reject this? If so, I think it would be better to test this change.

Not sure. I'll check.
Comment 19 Yuta Kitamura 2010-06-08 02:00:55 PDT
Created attachment 58124 [details]
Patch (Add a test)
Comment 20 Yuta Kitamura 2010-06-08 02:01:35 PDT
(In reply to comment #18)
> > bool ok = false;
> > statusCode = statusCodeString.toInt(&ok);
> > if (!ok || statusCode < 100 || statusCode >= 600) {
> >     statusCode = 0;
> >     return true;
> > }
> > 
> > wont' work?
> 
> Oops. Will fix.
> 

Turned out that this code snippet is different from my original patch ("!ok" does not appear). I think my patch has no problem.

> > 
> > 
> > WebCore/websockets/WebSocketHandshakeResponse.h:63
> >  +      void setChallengeResponse(const unsigned char challengeResponse[16]);
> > I'd take signed char here so we don't need reinterpret_cast, but it's OK as is.
> > 
> > WebCore/websockets/WebSocketHandshake.cpp:80
> >  +      if (!space1 || !space2 || *(p - 1) != '\r') // The line must end with "\r\n".
> > So, now we are rejecting responses like "HTTP/1.0 101 WebSocket Protocol Handshake\n" and before this change we don't reject this? If so, I think it would be better to test this change.
> 
> Not sure. I'll check.

I added a test.
Comment 21 Yuta Kitamura 2010-06-08 02:08:33 PDT
Comment on attachment 58124 [details]
Patch (Add a test)

> -    if (code.isEmpty()) {
> +    if (!statusCode) {
>          m_mode = Failed;
> -        m_context->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "No response code found: " + trimConsoleMessage(header, lineLength), 0, clientOrigin());
> +        m_context->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "Invalid response line: " + escapeControlCharacters(trimConsoleMessage(header, lineLength + 1)), 0, clientOrigin());
>          return len;
>      }

Let me clarify this code: I added a code that escapes control characters so that they does not appear in console.
Comment 22 Yuta Kitamura 2010-06-10 03:25:11 PDT
Created attachment 58353 [details]
Patch (Fix case of header names)
Comment 23 Yuta Kitamura 2010-06-10 03:31:37 PDT
Comment on attachment 58353 [details]
Patch (Fix case of header names)

I updated the patch to preserve the case of header names (not to make header names lower-case). The following is the difference since the last patch.

> @@ -495,26 +551,27 @@ const char* WebSocketHandshake::readHTTPHeaders(const char* start, const char* e
>              return 0;
>          }
>          LOG(Network, "name=%s value=%s", nameStr.string().utf8().data(), valueStr.utf8().data());
> -        headers->add(nameStr, valueStr);
> +        m_response.addHeaderField(nameStr, valueStr);
>      }
>      ASSERT_NOT_REACHED();
>      return 0;
>  }
>  
> -bool WebSocketHandshake::processHeaders(const HTTPHeaderMap& headers)
> +bool WebSocketHandshake::processHeaders()
>  {
> +    const HTTPHeaderMap& headers = m_response.headerFields();
>      for (HTTPHeaderMap::const_iterator it = headers.begin(); it != headers.end(); ++it) {
>          switch (m_mode) {
>          case Normal:
> -            if (it->first == "sec-websocket-origin")
> +            if (equalIgnoringCase(it->first, "sec-websocket-origin"))
>                  m_wsOrigin = it->second;
> -            else if (it->first == "sec-websocket-location")
> +            else if (equalIgnoringCase(it->first, "sec-websocket-location"))
>                  m_wsLocation = it->second;
> -            else if (it->first == "sec-websocket-protocol")
> +            else if (equalIgnoringCase(it->first, "sec-websocket-protocol"))
>                  m_wsProtocol = it->second;
> -            else if (it->first == "set-cookie")
> +            else if (equalIgnoringCase(it->first, "set-cookie"))
>                  m_setCookie = it->second;
> -            else if (it->first == "set-cookie2")
> +            else if (equalIgnoringCase(it->first, "set-cookie2"))
>                  m_setCookie2 = it->second;
>              continue;
>          case Incomplete:
Comment 24 WebKit Review Bot 2010-06-10 03:38:39 PDT
Attachment 58353 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3260028
Comment 25 Yuta Kitamura 2010-06-10 22:19:20 PDT
Created attachment 58445 [details]
Patch (Fix pbxproj file)
Comment 26 WebKit Review Bot 2010-06-13 23:35:45 PDT
Attachment 58445 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3317104
Comment 27 Alexey Proskuryakov 2010-06-17 10:31:46 PDT
Comment on attachment 58445 [details]
Patch (Fix pbxproj file)

+// Return true if we have received enough data to parse a line.

There is a readServerHandshake() function in the same file that returns the same information differently (it returns handshake length, and -1 if it can't be determined yet). That's highly confusing.

+static bool parseResponseLine(const char* header, int len, int& statusCode, String& statusText, size_t& lineLength)

FWIW, "statusText" is called "reason phrase" in RFC 2616.

<...>
+    if (statusCode < 100 || statusCode >= 600) {
+        statusCode = 0;
+        return true;
+    }

This function doesn't validate other parts of the string but from its signature, it looks as if it did. I'd probably call it "extractStatusCodeAndText" to avoid confusion - or make it check that the response line starts with HTTP-Version and other requirements.

In particular, it seems out of place to check numeric value of statusCode in parsing function.

+// Format a console message so that control characters look pretty.
+static String escapeControlCharacters(const String& string)

Formatting for console is definitely not something that belongs to this file. If we want control characters to look pretty, then we probably always want them to, and this code should be in addConsoleMessage() implementation file.

+        if (string[i] < 0x20 || string[i] == 0x7F)

This is not how a control character is defined. We have an isPrintableChar() function in wtf/Unicode.h - one should be sure to iterate by 32-bit code points to use it.

+#include <cstring>

We usually include string.h. I'm not actually sure if this include is needed, or config.h takes care of that.

>      const char* headerFields = strnstr(header, "\r\n", len); // skip status line

We already know the length of status line from parseResponseLine(), why search for CRLF again?

This patch generally looks good, but I think that there were enough comments for another review round.
Comment 28 Yuta Kitamura 2010-06-18 06:15:51 PDT
Thank you for your review.

(In reply to comment #27)
> +static bool parseResponseLine(const char* header, int len, int& statusCode, String& statusText, size_t& lineLength)
> 
> FWIW, "statusText" is called "reason phrase" in RFC 2616.

Actually I considered that, but I decided to go with "statusText" because there was existing code using that name (WebCore/platform/network/ResourceResponseBase.h). I guess "statusText" is not confusing and is acceptable.

I want to fix other issues mentioned in comment #27.
Comment 29 Yuta Kitamura 2010-06-18 06:25:53 PDT
Created attachment 59102 [details]
Patch (Address ap's comments)
Comment 30 WebKit Review Bot 2010-06-18 08:02:21 PDT
Attachment 59102 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3340381
Comment 31 Alexey Proskuryakov 2010-06-18 09:50:42 PDT
Comment on attachment 59102 [details]
Patch (Address ap's comments)

>          s += "...";

You didn't change this code, but ellipsis should use its own Unicode character, not three dots. And, "trimConsoleMessage()" might also be out of place in this file, I'm not sure.

+int WebSocketHandshake::readStatusLine(const char* header, size_t len, int& statusCode, String& statusText)

We prefer to not abbreviate "length". Also, a more descriptive name could be used here (maybe headerLength).

+    m_response.setChallengeResponse(reinterpret_cast<const unsigned char*>(p));

Doesn't static_cast work here?

+    const HTTPHeaderMap& headers = m_response.headerFields();
     for (HTTPHeaderMap::const_iterator it = headers.begin(); it != headers.end(); ++it) {

I missed this yesterday: it seems that we don't need to iterate any more, do we? There is no order in HTTPHeaderMap. But it's not new with your patch.

r=me
Comment 32 Yuta Kitamura 2010-06-22 01:41:30 PDT
(In reply to comment #31)
> +    m_response.setChallengeResponse(reinterpret_cast<const unsigned char*>(p));
> 
> Doesn't static_cast work here?

No, because unsigned char* and char* is incompatible. But the following works:
  static_cast<const unsigned char*>(static_cast<const void*>(p))

Perhaps this is better.
Comment 33 Yuta Kitamura 2010-06-22 01:43:32 PDT
Created attachment 59354 [details]
Patch (Address comment #31)
Comment 34 Yuta Kitamura 2010-06-22 01:47:18 PDT
I fixed the issues suggested in comment #31. Could you please review this patch again? Thanks.
Comment 35 WebKit Review Bot 2010-06-22 01:55:53 PDT
Attachment 59354 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3292608
Comment 36 Alexey Proskuryakov 2010-06-22 08:41:11 PDT
Comment on attachment 59354 [details]
Patch (Address comment #31)

+        s.append(static_cast<UChar>(0x2026)); // U+2026 Horizontal ellipsis.

This should use a character constant from CharacterNames.h. We already have one for ellipsis.

r=me with this change.
Comment 37 Hajime Morrita 2010-06-23 00:38:27 PDT
Committed r61671: <http://trac.webkit.org/changeset/61671>