Bug 73092 – InspectorServer: Add Generic HTTP Request Class. Generalized HTTP Parsing.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Jocelyn Turcotte]

Created attachment 116536 [details]
Original New Part 2 patch

Tracks the "New Part 2" and "New Part 3" patches of bug #51364.

[Jocelyn Turcotte]

Created attachment 116537 [details]
Original New Part 3 patch

[Jocelyn Turcotte]

Created attachment 116538 [details]
Patch

[Jocelyn Turcotte]

Please ignore the link with the Part 3 patch, it was merged with Part 4 in bug #73093 and not with Part 2.

[Jocelyn Turcotte]

Created attachment 134601 [details]
Patch

Rebased patch.

[Jocelyn Turcotte]

Created attachment 134607 [details]
Diff between WebSocketHandshake::readHTTPHeaders and parseHTTPHeader.

For review reference. (testing to see if this kind of stuff helps reviewing)

[Joseph Pecoraro]

Since this spawned from one of my earlier patches on bug 51364, Alexey made
some review comments that are still valid:
<https://bugs.webkit.org/show_bug.cgi?id=51364#c45>

- parseHTTPHeader assumes UTF-8 encoding (okay for WebSockets, not okay for generic HTTP)
- possibility for localization of error messages

[WebKit Review Bot]

Comment on attachment 134601 [details]
Patch

Attachment 134601 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12257001

New failing tests:
http/tests/websocket/tests/hybi/fragmented-binary-frames.html
http/tests/websocket/tests/hixie76/websocket-pending-activity.html
http/tests/websocket/tests/hybi/handshake-challenge-randomness.html
http/tests/websocket/tests/hybi/deflate-frame-invalid-parameter.html
http/tests/websocket/tests/hybi/close-code-and-reason.html
http/tests/websocket/tests/hybi/fragmented-control-frame.html
http/tests/inspector/inspect-element.html
accessibility/aria-disabled.html
http/tests/websocket/tests/hybi/frame-lengths.html
fast/loader/text-document-wrapping.html
fast/frames/lots-of-objects.html
http/tests/websocket/tests/hybi/fragmented-frames.html
http/tests/websocket/tests/hybi/broken-utf8.html
http/tests/websocket/tests/hybi/close-on-unload-reference-in-parent.html
http/tests/websocket/tests/hybi/close-on-unload-and-force-gc.html
http/tests/websocket/tests/hybi/compressed-control-frame.html
http/tests/websocket/tests/hybi/close.html
http/tests/websocket/tests/hybi/close-unref-websocket.html
http/tests/websocket/tests/hybi/close-on-navigate-new-location.html
http/tests/websocket/tests/hybi/bufferedAmount-after-close.html
http/tests/websocket/tests/hybi/deflate-frame-comp-bit-onoff.html
http/tests/websocket/tests/hybi/alert-in-event-handler.html
http/tests/websocket/tests/hybi/cross-origin.html
http/tests/websocket/tests/hybi/client-close.html
http/tests/websocket/tests/hybi/deflate-frame-parameter.html
http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html
http/tests/websocket/tests/hybi/close-on-unload.html
http/tests/websocket/tests/hybi/extensions.html
http/tests/websocket/tests/hybi/close-event.html

[WebKit Review Bot]

Created attachment 134701 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Jocelyn Turcotte]

Created attachment 134755 [details]
Patch

Fix the cr bot Sec-WebSocket-Extensions issue.

[Jocelyn Turcotte]

(In reply to comment #6)
> Since this spawned from one of my earlier patches on bug 51364, Alexey made
> some review comments that are still valid:
> <https://bugs.webkit.org/show_bug.cgi?id=51364#c45>
> 
> - parseHTTPHeader assumes UTF-8 encoding (okay for WebSockets, not okay for generic HTTP)
> - possibility for localization of error messages

The errors messages will probably not reach any real user ever in the case of the remote inspector and I probably won't spend time on it.
I've opened bug #82714 for the header encoding, this patch just moved the code so I can address it separately.
Thanks

[Kenneth Rohde Christiansen]

Comment on attachment 134755 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:286
> +        InspectorInstrumentation::willSendWebSocketHandshakeRequest(m_document, m_identifier, *m_handshake->clientHandshakeRequest());

.get() ?

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:90
> +static String trimInputSample(const char* p, size_t len)

length?

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:327
> +    RefPtr<WebSocketHandshakeRequest> request = WebSocketHandshakeRequest::create("GET", m_url);
>      if (m_useHixie76Protocol)
> -        request.addHeaderField("Upgrade", "WebSocket");
> +        request->addHeaderField("Upgrade", "WebSocket");

Isn't this the one that isnt supported?

> Source/WebCore/platform/network/HTTPParsers.cpp:96
> +static const size_t maxInputSampleSize = 128;
> +static String trimInputSample(const char* p, size_t length)
> +{

You seem to have similar code elsewhere

> Source/WebCore/platform/network/HTTPParsers.cpp:411
> +size_t parseHTTPRequestLine(const char* data, size_t length, String& failureReason, String& method, String& url, HttpVersion& httpVersion)

why is this HttpVersion and not HTTPVersion?

> Source/WebCore/platform/network/HTTPParsers.h:70
> +enum HttpVersion { Unknown, HTTP_1_0, HTTP_1_1 };

HTTPVersion?

[Jocelyn Turcotte]

(In reply to comment #11)
> > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:286
> > +        InspectorInstrumentation::willSendWebSocketHandshakeRequest(m_document, m_identifier, *m_handshake->clientHandshakeRequest());
> 
> .get() ?
> 
The function takes a reference.

> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:90
> > +static String trimInputSample(const char* p, size_t len)
> 
> length?
> 
I think len is better for consistency in this file.

> > Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:327
> > +    RefPtr<WebSocketHandshakeRequest> request = WebSocketHandshakeRequest::create("GET", m_url);
> >      if (m_useHixie76Protocol)
> > -        request.addHeaderField("Upgrade", "WebSocket");
> > +        request->addHeaderField("Upgrade", "WebSocket");
> 
> Isn't this the one that isnt supported?
> 
That's the client code, which still supports both. The old one is guarded by m_useHixie76Protocol.

> > Source/WebCore/platform/network/HTTPParsers.cpp:96
> > +static const size_t maxInputSampleSize = 128;
> > +static String trimInputSample(const char* p, size_t length)
> > +{
> 
> You seem to have similar code elsewhere
> 
Yes, I wasn't sure where to expose this, I don't want to pollute the namespace directly for such a simple method so I duplicated it. What would you suggest?

[Jocelyn Turcotte]

Created attachment 135094 [details]
Patch

Rename HttpVersion to HTTPVersion

[Jocelyn Turcotte]

Committed r113024: <http://trac.webkit.org/changeset/113024>