Relax Content-Type parameter parsing

Relax Content-Type parameter parsing

Unfortunately Opera 12 (this could possibly be found in files generated by
other products as well, or by hand) and below does not write out its
Content-Type parameters according to the standard (start parameter contains
tspecials without being an unquoted-string) in some MHTML files. This patch
relaxes the parsing of Content-Type parameters to allow this.

R=tony@chromium.org
BUG=274407
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158314

[tony, 2013-08-13 17:35:00]

I am not an appropriate reviewer for this patch.  Changing the reviewers to Jay
(original author) and Adam (original reviewer).

Also, there should be a layout test or unit test with this change.

[abarth-chromium, 2013-08-13 19:53:30]

We certainly would need a test, but the bigger question is whether we should
make this change.  How does Internet Explorer handle these cases?

[Tiger, 2013-08-14 10:17:08]

On 2013/08/13 19:53:30, abarth wrote:
> We certainly would need a test, but the bigger question is whether we should
> make this change.  How does Internet Explorer handle these cases?

Both UnMHT (for Firefox) and IE (at least version 11 which I tested) handles
this completely fine. There where some confusions in earlier versions of the
related RFC, along with some invalid examples that might have caused this to be
wrong in Opera generated files. There are some more details about this here,
among many other things: http://people.dsv.su.se/~jpalme/ietf/mhtml.html

I'll generate some test MHTML files to test both the correct version and a
non-correct version.

[abarth-chromium, 2013-08-14 19:35:40]

On 2013/08/14 10:17:08, Tiger wrote:
> On 2013/08/13 19:53:30, abarth wrote:
> > We certainly would need a test, but the bigger question is whether we should
> > make this change.  How does Internet Explorer handle these cases?
> 
> Both UnMHT (for Firefox) and IE (at least version 11 which I tested) handles
> this completely fine. There where some confusions in earlier versions of the
> related RFC, along with some invalid examples that might have caused this to
be
> wrong in Opera generated files. There are some more details about this here,
> among many other things: http://people.dsv.su.se/%7Ejpalme/ietf/mhtml.html
> 
> I'll generate some test MHTML files to test both the correct version and a
> non-correct version.

Great!  Thanks for checking the behavior of other browsers.

[Tiger, 2013-08-15 10:00:19]

Waiting for a fix/solution to the MHTML file uploading problems in
https://chromiumcodereview.appspot.com/22292008/ before uploading MTHML files
here.

[Tiger, 2013-09-02 10:42:23]

The test files need to be committed separately. I've uploaded them to
<https://people.opera.com/tiger/patches/relaxed-content-type-parameters.tar.gz>.
Could anyone upload them for me?

[Tiger, 2013-09-12 08:23:37]

On 2013/09/02 10:42:23, Tiger wrote:
> The test files need to be committed separately. I've uploaded them to
>
<https://people.opera.com/tiger/patches/relaxed-content-type-parameters.tar.gz>.
> Could anyone upload them for me?

Test are now uploaded to the review. It looks like this should work now.
Awaiting review.

[Tiger, 2013-09-12 08:26:31]

Adding more reviewers, could anyone of you @tkent, @jochen, @jchaffraix take a
look a this?

[jochen (gone - plz use gerrit), 2013-09-12 09:46:21]

On 2013/09/12 08:26:31, Tiger wrote:
> Adding more reviewers, could anyone of you @tkent, @jochen, @jchaffraix take a
> look a this?

Adam already started, so I'll defer to him. I don't know much about mhtml
parsing.

[abarth-chromium, 2013-09-12 18:16:11]

https://chromiumcodereview.appspot.com/23043002/diff/10001/Source/core/platfo...
File Source/core/platform/network/ParsedContentType.cpp (left):

https://chromiumcodereview.appspot.com/23043002/diff/10001/Source/core/platfo...
Source/core/platform/network/ParsedContentType.cpp:67: if
(!isTokenCharacter(input[tokenEnd]))
I see.  The key is that you're trying to lift this requirement.

https://chromiumcodereview.appspot.com/23043002/diff/10001/Source/core/platfo...
File Source/core/platform/network/ParsedContentType.cpp (right):

https://chromiumcodereview.appspot.com/23043002/diff/10001/Source/core/platfo...
Source/core/platform/network/ParsedContentType.cpp:64: if (quoted && tokenStart
!= tokenEnd && input[tokenEnd] == '\"' && input[tokenEnd - 1] != '\\')
I don't think this is correct.  Consider the following case:

foo="\\"

This code thinks that the second \ escapes the ", which isn't correct.  The
second \ is escaped by the first \.  To do this correctly, you need to do what
the parseQuotedString code did and scan the string from left to right and keep
track of whether lastCharacterWasBackslash.

[Tiger, 2013-09-16 10:05:03]

On 2013/09/12 18:16:11, abarth wrote:
>
https://chromiumcodereview.appspot.com/23043002/diff/10001/Source/core/platfo...
> File Source/core/platform/network/ParsedContentType.cpp (left):
> 
>
https://chromiumcodereview.appspot.com/23043002/diff/10001/Source/core/platfo...
> Source/core/platform/network/ParsedContentType.cpp:67: if
> (!isTokenCharacter(input[tokenEnd]))
> I see.  The key is that you're trying to lift this requirement.
> 
>
https://chromiumcodereview.appspot.com/23043002/diff/10001/Source/core/platfo...
> File Source/core/platform/network/ParsedContentType.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/23043002/diff/10001/Source/core/platfo...
> Source/core/platform/network/ParsedContentType.cpp:64: if (quoted &&
tokenStart
> != tokenEnd && input[tokenEnd] == '\"' && input[tokenEnd - 1] != '\\')
> I don't think this is correct.  Consider the following case:
> 
> foo="\\"
> 
> This code thinks that the second \ escapes the ", which isn't correct.  The
> second \ is escaped by the first \.  To do this correctly, you need to do what
> the parseQuotedString code did and scan the string from left to right and keep
> track of whether lastCharacterWasBackslash.

Yes, this is true. I've adjusted the parsing to look out for this. Good catch,
thanks!

[abarth-chromium, 2013-09-16 19:41:41]

lgtm

https://chromiumcodereview.appspot.com/23043002/diff/20001/Source/core/platfo...
File Source/core/platform/network/ParsedContentType.cpp (right):

https://chromiumcodereview.appspot.com/23043002/diff/20001/Source/core/platfo...
Source/core/platform/network/ParsedContentType.cpp:64: while (tokenEnd <
inputLength) {
Rather than calling input[tokenEnd] multiple times, we should just read it into
a local variable in the loop:

UChar c = input[tokenEnd];

It's slightly more efficient because input[tokenEnd] involves a branch.

[commit-bot: I haz the power, 2013-09-17 11:21:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/23043002/29001

[commit-bot: I haz the power, 2013-09-17 11:22:01]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1272.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
05f7475b0500967e91c30638b3a6643bad73036a..6007fdf1b843a81b2618f7fed2ab71617bb914c6
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1272,3 +1272,4 @@ crbug.com/276769
http/tests/security/contentSecurityPolicy/source-list-parsing-n
 crbug.com/286081 inspector/debugger/long-resume-vs-reload.html [ Failure Pass
Timeout ]
 crbug.com/285703 [ Debug ]
fast/table/table-rowspan-crash-with-huge-rowspan-cells.html [ Crash Pass ]
 crbug.com/286287 http/tests/inspector/network/network-initiator.html [ Failure
Pass ]
+

[commit-bot: I haz the power, 2013-09-17 13:03:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/23043002/45001

[commit-bot: I haz the power, 2013-09-17 14:22:13]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-09-17 15:35:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/23043002/45001

[commit-bot: I haz the power, 2013-09-17 17:00:39]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-09-17 22:12:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/23043002/45001

[commit-bot: I haz the power, 2013-09-17 23:47:24]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-09-18 07:30:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/23043002/45001

[commit-bot: I haz the power, 2013-09-18 09:05:11]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[Tiger, 2013-09-19 10:18:17]

Added an additional fix to not the last token if it was quoted but didn't have
an end quote.

[commit-bot: I haz the power, 2013-09-25 08:29:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/23043002/80001

[commit-bot: I haz the power, 2013-09-25 09:35:25]

Change committed as 158314