Bug 38728 – WebSocket: Add WebSocketHandshakeResponse

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Yuta Kitamura]

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.

[Yuta Kitamura]

I want to wait for the handshake change (bug 35572) to be committed.

[Yuta Kitamura]

Created attachment 56330 [details]
Patch FYI (ukai's patch in bug 35572 is required)

[Yuta Kitamura]

(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,

[Fumitoshi Ukai]

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?

[Yuta Kitamura]

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

[Yuta Kitamura]

Created attachment 56674 [details]
Patch

[Fumitoshi Ukai]

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

[Yuta Kitamura]

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

[Yuta Kitamura]

Created attachment 57221 [details]
Patch

[Yuta Kitamura]

(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).

[Yuta Kitamura]

Created attachment 57223 [details]
Patch (conflict resolved for bots' sake)

[Yuta Kitamura]

Created attachment 57439 [details]
Patch (Add ChallengeResponse)

[Fumitoshi Ukai]

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?

[Yuta Kitamura]

(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...)

[Yuta Kitamura]

Created attachment 57514 [details]
Patch

[Fumitoshi Ukai]

Looks good to me.

[Shinichiro Hamaji]

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.

[Yuta Kitamura]

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

[Yuta Kitamura]

Created attachment 58124 [details]
Patch (Add a test)

[Yuta Kitamura]

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

[Yuta Kitamura]

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.

[Yuta Kitamura]

Created attachment 58353 [details]
Patch (Fix case of header names)

[Yuta Kitamura]

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:

[WebKit Review Bot]

Attachment 58353 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3260028

[Yuta Kitamura]

Created attachment 58445 [details]
Patch (Fix pbxproj file)

[WebKit Review Bot]

Attachment 58445 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3317104

[Alexey Proskuryakov]

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.

[Yuta Kitamura]

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.

[Yuta Kitamura]

Created attachment 59102 [details]
Patch (Address ap's comments)

[WebKit Review Bot]

Attachment 59102 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3340381

[Alexey Proskuryakov]

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

[Yuta Kitamura]

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

[Yuta Kitamura]

Created attachment 59354 [details]
Patch (Address comment #31)

[Yuta Kitamura]

I fixed the issues suggested in comment #31. Could you please review this patch again? Thanks.

[WebKit Review Bot]

Attachment 59354 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3292608

[Alexey Proskuryakov]

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.

[Hajime Morrita]

Committed r61671: <http://trac.webkit.org/changeset/61671>