Bug 73092

Summary: InspectorServer: Add Generic HTTP Request Class. Generalized HTTP Parsing.
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: Web Inspector (Deprecated)Assignee: Jocelyn Turcotte <jturcotte>
Status: RESOLVED FIXED    
Severity: Enhancement CC: ap, dglazkov, hausmann, joepeck, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73094    
Attachments:
Description Flags
Original New Part 2 patch
none
Original New Part 3 patch
none
Patch
none
Patch
none
Diff between WebSocketHandshake::readHTTPHeaders and parseHTTPHeader.
none
Archive of layout-test-results from ec2-cr-linux-04
none
Patch
none
Patch hausmann: review+

Description Jocelyn Turcotte 2011-11-24 13:13:26 PST
Created attachment 116536 [details]
Original New Part 2 patch

Tracks the "New Part 2" and "New Part 3" patches of bug #51364.
Comment 1 Jocelyn Turcotte 2011-11-24 13:13:50 PST
Created attachment 116537 [details]
Original New Part 3 patch
Comment 2 Jocelyn Turcotte 2011-11-24 13:15:55 PST
Created attachment 116538 [details]
Patch
Comment 3 Jocelyn Turcotte 2011-11-24 13:30:31 PST
Please ignore the link with the Part 3 patch, it was merged with Part 4 in bug #73093 and not with Part 2.
Comment 4 Jocelyn Turcotte 2012-03-29 09:40:50 PDT
Created attachment 134601 [details]
Patch

Rebased patch.
Comment 5 Jocelyn Turcotte 2012-03-29 10:00:17 PDT
Created attachment 134607 [details]
Diff between WebSocketHandshake::readHTTPHeaders and parseHTTPHeader.

For review reference. (testing to see if this kind of stuff helps reviewing)
Comment 6 Joseph Pecoraro 2012-03-29 10:53:19 PDT
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
Comment 7 WebKit Review Bot 2012-03-29 17:15:46 PDT
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
Comment 8 WebKit Review Bot 2012-03-29 17:15:55 PDT
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
Comment 9 Jocelyn Turcotte 2012-03-30 02:44:36 PDT
Created attachment 134755 [details]
Patch

Fix the cr bot Sec-WebSocket-Extensions issue.
Comment 10 Jocelyn Turcotte 2012-03-30 03:47:26 PDT
(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
Comment 11 Kenneth Rohde Christiansen 2012-04-02 04:56:12 PDT
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?
Comment 12 Jocelyn Turcotte 2012-04-02 07:21:44 PDT
(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?
Comment 13 Jocelyn Turcotte 2012-04-02 07:24:07 PDT
Created attachment 135094 [details]
Patch

Rename HttpVersion to HTTPVersion
Comment 14 Jocelyn Turcotte 2012-04-03 06:34:12 PDT
Committed r113024: <http://trac.webkit.org/changeset/113024>