Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(201)

Issue 9198: This fixes bug http://code.google.com/p/chromium/issues/detail?id=4076, which... (Closed)

Created:
12 years, 1 month ago by ananta
Modified:
9 years, 6 months ago
Reviewers:
jam
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

This fixes bug http://code.google.com/p/chromium/issues/detail?id=4076, which was a hang while loading certain PDF files. This was a regression caused by support for NPN_RequestRead (PDF Fast Webview). We had incorrectly assumed that the Content Type showing up for partial HTTP Responses would always end with the boundary. A content type can show up like multipart/byteranges; boundary=--bound--; charSet=utf8. As a result we would look for the wrong boundary in the actual data resulting in a hang. The parsing code now accounts for this. Added a unit test to test this case. Bug=4076 R=jam Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=4842

Patch Set 1 #

Patch Set 2 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -1 line) Patch
M webkit/glue/multipart_response_delegate.cc View 1 1 chunk +6 lines, -1 line 0 comments Download
M webkit/glue/multipart_response_delegate_unittest.cc View 1 1 chunk +14 lines, -0 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
ananta
12 years, 1 month ago (2008-11-05 21:30:55 UTC) #1
jam
lgtm http://codereview.chromium.org/9198/diff/1/2 File webkit/glue/multipart_response_delegate.cc (right): http://codereview.chromium.org/9198/diff/1/2#newcode255 Line 255: boundary_end_offset = content_type.length(); shouldn't this be content_type_as_string.length()? ...
12 years, 1 month ago (2008-11-05 22:53:42 UTC) #2
ananta
12 years, 1 month ago (2008-11-06 00:04:27 UTC) #3
Done and submitted. 

Thanks for the review.

-Ananta
On 2008/11/05 22:53:42, John Abd-El-Malek wrote:
> lgtm
> 
> http://codereview.chromium.org/9198/diff/1/2
> File webkit/glue/multipart_response_delegate.cc (right):
> 
> http://codereview.chromium.org/9198/diff/1/2#newcode255
> Line 255: boundary_end_offset = content_type.length();
> shouldn't this be content_type_as_string.length()?  I'm not sure if the
> SringToStdString conversion can affect the length.
> 
> http://codereview.chromium.org/9198/diff/1/3
> File webkit/glue/multipart_response_delegate_unittest.cc (right):
> 
> http://codereview.chromium.org/9198/diff/1/3#newcode441
> Line 441: EXPECT_EQ(string("--bound--"),
> this can just be on one line..

Powered by Google App Engine
This is Rietveld 408576698