Bug 53275 - Fix ContentType parameter parsing error
Summary: Fix ContentType parameter parsing error
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nancy Piedra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-27 17:54 PST by Yi Shen
Modified: 2011-04-07 11:46 PDT (History)
5 users (show)

See Also:


Attachments
first try (2.62 KB, patch)
2011-01-27 18:28 PST, Yi Shen
kling: review-
Details | Formatted Diff | Diff
add logs to help testing (3.75 KB, patch)
2011-01-28 12:23 PST, Yi Shen
ap: review-
Details | Formatted Diff | Diff
Patch to fix codecs parsing (4.41 KB, patch)
2011-04-05 17:16 PDT, Nancy Piedra
no flags Details | Formatted Diff | Diff
New patch based on Alexey's comments (4.43 KB, patch)
2011-04-06 05:01 PDT, Nancy Piedra
eric.carlson: review-
Details | Formatted Diff | Diff
Modified patch based on Eric's comments (4.42 KB, patch)
2011-04-07 06:18 PDT, Nancy Piedra
eric.carlson: review+
Details | Formatted Diff | Diff
Modified patch based on Eric's latest comments (4.55 KB, patch)
2011-04-07 06:40 PDT, Nancy Piedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yi Shen 2011-01-27 17:54:13 PST
The implementation of ContentType::parameter() is incorrect for some particular cases, take LayoutTests/media/video-can-play-type.html for an example,

testExpected("video.canPlayType('video/mpeg; Codecs="avc1.4D400C"')", "probably");

The ContentType::parameter() returns "&quot"
Comment 1 Yi Shen 2011-01-27 18:28:38 PST
Created attachment 80395 [details]
first try
Comment 2 Andreas Kling 2011-01-28 01:26:18 PST
Comment on attachment 80395 [details]
first try

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

> Source/WebCore/ChangeLog:10
> +        Refactor only, no new tests.

This is not a refactor, you're fixing a bug. We need a regression test. :)
Comment 3 Yi Shen 2011-01-28 07:27:29 PST
(In reply to comment #2)
> (From update of attachment 80395 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80395&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Refactor only, no new tests.
> 
> This is not a refactor, you're fixing a bug. We need a regression test. :)

Thanks Kling, I will work on making a test for it :)
Comment 4 Yi Shen 2011-01-28 12:23:27 PST
Created attachment 80481 [details]
add logs to help testing
Comment 5 Yi Shen 2011-01-28 12:37:19 PST
Kling, LayoutTests/media/video-can-play-type.html is supposed to be the test for this change, however, it may not help much because no one really uses the media type codecs. For example,MediaPlayerPrivateQTKit::supportsType() only checks whether the codecs is empty or not (this can explain why this bug exists so long without causing any trouble.).

I tried to fix it since MediaPlayerPrivateQt::supportsType() needs to pass correct codecs to QMediaPlayer::hasSupport(mime, QStringList(codec)). However, the hasSupport() may have other unsolved issues since it always returns "QtMultimediaKit::MaybeSupported", event the mime type is "video/blahblah". So all the layouttest is not really reliable.

Instead, I added some logs in the patch to help testing. If you have better idea to make a test for this change, please let me know. thx
Comment 6 Alexey Proskuryakov 2011-01-28 15:30:14 PST
Comment on attachment 80481 [details]
add logs to help testing

There is nothing in RFC 2616 that talks about SGML-style named entities, so """ is not magic in HTTP.

If there is an actual compatibility issue that affects existing Web sites, please provide more information about it.
Comment 7 Yi Shen 2011-01-28 16:51:46 PST
(In reply to comment #6)
> (From update of attachment 80481 [details])
> There is nothing in RFC 2616 that talks about SGML-style named entities, so """ is not magic in HTTP.
> 
> If there is an actual compatibility issue that affects existing Web sites, please provide more information about it.

Thanks Alexey for review :). One example for """ issue is LayoutTests/media/video-can-play-type.html.

testExpected("video.canPlayType('video/mpeg; Codecs="avc1.4D400C"')", "probably");

Also,if you check the ContentType::parameter() in ContentType.cpp and the 50th line seems not right,

size_t end = strippedType.find(';', start + 6); 

Note, the 'start' is the index of semi-colon, then why it adds 6 here?
Comment 8 Alexey Proskuryakov 2011-01-28 17:13:38 PST
(In reply to comment #7)
> One example for """ issue is LayoutTests/media/video-can-play-type.html.

I don't know why this subtest was originally added. This could be an intentional test for an incorrect parameter, or a simple mistake (perhaps \" should have been used instead of ").

Nothing in the test's history indicates that " should be parsed as a quotation mark.

> Also,if you check the ContentType::parameter() in ContentType.cpp and the 50th line seems not right,
> 
> size_t end = strippedType.find(';', start + 6); 
> 
> Note, the 'start' is the index of semi-colon, then why it adds 6 here?

Yes, this part of your patch seems fine. I didn't verify it in detail for off by one errors and such. Please add tests for the issues you fixed.
Comment 9 Yi Shen 2011-02-02 10:13:54 PST
(In reply to comment #8)
> (In reply to comment #7)
> > One example for """ issue is LayoutTests/media/video-can-play-type.html.
> 
> I don't know why this subtest was originally added. This could be an intentional test for an incorrect parameter, or a simple mistake (perhaps \" should have been used instead of ").
> 
> Nothing in the test's history indicates that " should be parsed as a quotation mark.
> 
> > Also,if you check the ContentType::parameter() in ContentType.cpp and the 50th line seems not right,
> > 
> > size_t end = strippedType.find(';', start + 6); 
> > 
> > Note, the 'start' is the index of semi-colon, then why it adds 6 here?
> 
> Yes, this part of your patch seems fine. I didn't verify it in detail for off by one errors and such. Please add tests for the issues you fixed.

I have reported a qtmobility bug, http://bugreports.qt.nokia.com/browse/QTMOBILITY-1091, once it gets fixed, then we can run LayoutTests/media/video-can-play-type.html to test this change.
Comment 10 Nancy Piedra 2011-04-02 13:11:36 PDT
I spoke to Yi and he says he is not working on this error any longer.
I started working on a related error (https://bugs.webkit.org/show_bug.cgi?id=42094) so I told Yi I'd take over this error.

I looked into the test case video-can-play-type.html and I think there is an issue with the content.  I don't think " is a valid way to specify quotes in javascript.  I think you should use \" instead.

However, when I fix this it still doesn't solve the problem with the test case failing because MediaPlayPrivateQt::supportsType does not return the right values.

I will create a separate error for fixing the test content.  If that gets approved then I think maybe this error can be closed as not valid since the problem is in the test content and not the code.  I don't think the ContentType class should have to parse out the entity references (ie, ").

The issue with video-can-play-type.html failing for Qt can be fixed under 42094.
Comment 11 Nancy Piedra 2011-04-02 16:57:01 PDT
Actually we may still need part of this fix to parse out the quotation marks.
Comment 12 Nancy Piedra 2011-04-03 13:49:49 PDT
Created the following bug to change """ to '\"' in the layout test.
https://bugs.webkit.org/show_bug.cgi?id=57728
Comment 13 Nancy Piedra 2011-04-04 05:39:24 PDT
After looking at the specification for the codecs paramter, http://tools.ietf.org/html/rfc4281, I think the intention of the ContentType class was to return everything after the '=' since single quotes and double quotes have special meaning to that paramter.  So, I'm not sure that the quotes should be parsed out.

However, I'm not sure about the '+ 6' that Yi mentions above.  I will look intot that.
Comment 14 Alexey Proskuryakov 2011-04-04 09:09:07 PDT
Quote handling in Content-Type is governed by rfc2616, not by rfc4281. Please see sections 3.7 and 3.6 in <http://www.ietf.org/rfc/rfc2616.txt>:

       parameter               = attribute "=" value
       attribute               = token
       value                   = token | quoted-string
Comment 15 Nancy Piedra 2011-04-04 09:28:45 PDT
I was looking at the following section of the HTML5 spec which references RFC4281.  I will look at your references more carefully also.


4.8.10.3 MIME types

A media resource can be described in terms of its type, specifically a MIME type, in some cases with a codecs parameter. (Whether the codecs parameter is allowed or not depends on the MIME type.) [RFC4281]
Comment 16 Nancy Piedra 2011-04-04 10:04:25 PDT
After looking at RFC2616, I conclude that double quotes should be parsed out but not single quotes.


Here are excerpts from RFC2616 for reference:
Parameters are in the form of attribute/value pairs.

       parameter               = attribute "=" value
       attribute               = token
       value                   = token | quoted-string

A string of text is parsed as a single word if it is quoted using double-quote marks. 

       quoted-string  = ( <"> *(qdtext | quoted-pair ) <"> )
       qdtext         = <any TEXT except <">>



Alexey - Could you also look at https://bugs.webkit.org/show_bug.cgi?id=57728 and see if you agree?
Comment 17 Alexey Proskuryakov 2011-04-04 10:48:08 PDT
> double quotes should be parsed out but not single quotes

Yes, that's correct.
Comment 18 Nancy Piedra 2011-04-05 17:16:25 PDT
Created attachment 88340 [details]
Patch to fix codecs parsing

Attached patch to parse out quotes from 'codecs' parameter according to RFC2616 mentioned above.  
Also, added some tests to the existing video-can-play-type.html layout test to test the codecs parsing.
Comment 19 Alexey Proskuryakov 2011-04-05 17:48:16 PDT
Comment on attachment 88340 [details]
Patch to fix codecs parsing

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

I only had a brief look yet, and didn't verify that the parsing code is correct.

> Source/WebCore/platform/ContentType.cpp:50
> +                size_t quote = notFound, end = notFound;

Please don't declare two variables on one line.
Comment 20 Nancy Piedra 2011-04-06 05:01:22 PDT
Created attachment 88397 [details]
New patch based on Alexey's comments

Put the two variable declarations on separate lines.
Comment 21 Eric Carlson 2011-04-06 12:27:02 PDT
Comment on attachment 88397 [details]
New patch based on Alexey's comments

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

> Source/WebCore/ChangeLog:5
> +        Parse quotes from content type codecs parameter

This comment is incorrect, the change is for any parameter not just "codecs".

> Source/WebCore/platform/ContentType.cpp:48
> -            start = strippedType.find('=', start + 6);
> +            start = strippedType.find('=', start + 1);

This isn't right, it starts the search at the beginning of the parameter name instead of at the end.

> Source/WebCore/platform/ContentType.cpp:52
> +                size_t quote = notFound;
> +                size_t end = notFound;
> +                if ((quote = strippedType.find('\"', start + 1)) != notFound && (end = strippedType.find('\"', start + 2)) != notFound) // find dobule quote

The initializations to notFound are not helpful because you always set quote and end to strippedType.find().

I think it would be easier to read if you made the assignments outside of the test, but that may just be me.
Comment 22 Nancy Piedra 2011-04-06 12:34:17 PDT
Thanks!  I will rework the patch.
Comment 23 Nancy Piedra 2011-04-07 06:18:06 PDT
Created attachment 88616 [details]
Modified patch based on Eric's comments
Comment 24 Eric Carlson 2011-04-07 06:25:37 PDT
Comment on attachment 88616 [details]
Modified patch based on Eric's comments

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

r+ with the minor changes suggested. 

Thanks!

> Source/WebCore/ChangeLog:7
> +
> +        Parse quotes from content type parameters
> +        https://bugs.webkit.org/show_bug.cgi?id=53275
> +

It would be helpful to have a comment about how this change is tested, eg. something like the comment you are adding to the LayoutTests ChangeLog.

> Source/WebCore/platform/ContentType.cpp:52
> +                if (quote != notFound && end != notFound) // find dobule quote

Typo: dobule -> double. However, the comment doesn't really add anything for someone reading the code later so I would just remove it.
Comment 25 Nancy Piedra 2011-04-07 06:40:58 PDT
Created attachment 88621 [details]
Modified patch based on Eric's latest comments
Comment 26 Eric Carlson 2011-04-07 06:47:48 PDT
Comment on attachment 88621 [details]
Modified patch based on Eric's latest comments

Thanks!
Comment 27 WebKit Commit Bot 2011-04-07 11:46:39 PDT
Comment on attachment 88621 [details]
Modified patch based on Eric's latest comments

Clearing flags on attachment: 88621

Committed r83191: <http://trac.webkit.org/changeset/83191>
Comment 28 WebKit Commit Bot 2011-04-07 11:46:46 PDT
All reviewed patches have been landed.  Closing bug.