Bug 53973

Summary: [Qt] Can't play HTML5 Video because of the URL is percent encoded twice
Product: WebKit Reporter: Hui Huang <hui_huang>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, benjamin, commit-queue, eric, hausmann, hui_huang, laszlo.gombos, max.hong.shen, nancy.piedra, nanjundiah.harsha, yael
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://youtube.com/html5
Attachments:
Description Flags
Proposed patch
kling: review-
New patch with regression test
hui_huang: review-, hui_huang: commit-queue-
New patch after resolving conflicts
hui_huang: review-
New patch after fixing build problem without Qt Multimedia. Skipping test if Qt Multimedia is not available.
none
New patch to resolve conflicts none

Description Hui Huang 2011-02-07 20:41:06 PST
1. Build Webkit trunk on Ubuntu with Qt 4.7.2 and QtMobility 1.1.
2. Start QtTestBrowser.
3. Change User Agent to that of iPad.
4. Go to http://youtube.com/html5.
5. Go to the bottom of the page and select the link "Join the HTML5 Video trial".
6. Go to the top of the page and select the link "Browse".
7. Start wireshark or tcpdump to capture network traffic.
8. Select any video.

Video is not playing. In the PCAP trace generated by wireshark or tcpdump, HTTP 403 error is sent by the web server on the HTTP GET request for the video. The error can also be reproduced with other user agents such as the default user agent and Android 2.2.
Comment 1 Hui Huang 2011-02-07 21:10:33 PST
Created attachment 81577 [details]
Proposed patch
Comment 2 Hui Huang 2011-02-07 21:24:14 PST
The URL of the HTML5 Video has already been percent encoded by youtube. It is percent encoded again in MediaPlayerPrivateQt::commitLoad by the constructor QUrl::QUrl(QString). According QUrl class reference (http://doc.qt.nokia.com/latest/qurl.html#QUrl-2), this constructor assumes that the URL is not percent encoded. Since the URL is double percent encoded, the HTTP GET Request for the video is rejected by youtube.

The bug can be fixed by using QUrl::fromEncoded to construct QUrl with an encoded URL. Attached proposed patch.
Comment 3 Benjamin Poulain 2011-02-08 00:51:26 PST
Comment on attachment 81577 [details]
Proposed patch

Setting review flag.
Comment 4 Benjamin Poulain 2011-02-08 00:57:29 PST
When you add a patch, you should set the review flag and the commit-queue flag, otherwise, reviewers do not see your patch (because it is not in the review queue). Here is some more info: http://trac.webkit.org/wiki/QtWebKitContrib

I think a test is needed for this case.
Comment 5 Andreas Kling 2011-02-08 07:06:51 PST
Comment on attachment 81577 [details]
Proposed patch

Needs a regression test.
Comment 6 Hui Huang 2011-02-08 08:04:52 PST
Sorry, my mistake. Will add a test as recommended by you and Andreas.
 
(In reply to comment #4)
> When you add a patch, you should set the review flag and the commit-queue flag, otherwise, reviewers do not see your patch (because it is not in the review queue). Here is some more info: http://trac.webkit.org/wiki/QtWebKitContrib
> 
> I think a test is needed for this case.
Comment 7 Simon Hausmann 2011-02-09 05:33:55 PST
I think calling ascii() is scary and IMHO we should avoid it.

Wouldn't it be safer to convert the String into a KURL first, and then convert that to a QURL? These conversions we know are safe:

KUrl kurl(ParsedURLString, url);
QUrl qurl = kurl;

But maybe I'm just to afraid of these encoding issues :)
Comment 8 Hui Huang 2011-02-09 07:48:44 PST
Thanks a lot for the advice. I tried your solution. It works. I think it is better than my original code. I will use it in the new patch I am working on.

(In reply to comment #7)
> I think calling ascii() is scary and IMHO we should avoid it.
> 
> Wouldn't it be safer to convert the String into a KURL first, and then convert that to a QURL? These conversions we know are safe:
> 
> KUrl kurl(ParsedURLString, url);
> QUrl qurl = kurl;
> 
> But maybe I'm just to afraid of these encoding issues :)
Comment 9 Hui Huang 2011-02-10 13:41:19 PST
Created attachment 82039 [details]
New patch with regression test
Comment 10 Hui Huang 2011-02-10 14:06:10 PST
I attached a new patch with regression test and Simon's improvement. I spent quite some time trying to find out a way to create a test using HTML and JS only, but I can't find a good solution without the involvement of an external web server. Therefore, I added a test function in tst_qwebpage. Since the HTTP request is sent by QtMobility, QNetworkAccessManager can not be used to check the request URL. I used DumpRenderTreeSupportQt to access media content URL from the Media Player via video element in DOM. I tested the patch and the test function worked. Suggestions on improvements are appreciated.
Comment 11 Hui Huang 2011-02-10 14:30:57 PST
Created attachment 82049 [details]
New patch after resolving conflicts
Comment 12 Harsha 2011-02-15 04:19:17 PST
I was also working on this issue. I had a similar fix for the issue. :-).
I have tested the above patch on ubuntu and youtube html5 video playback works well.

Note: Similar fix is needed in the phonon path i.e in MediaPlayerPrivatePhonon.cpp
Comment 13 Simon Hausmann 2011-02-15 07:31:44 PST
Comment on attachment 82049 [details]
New patch after resolving conflicts

I like it, but I think the test will fail on platforms that don't have QtMobility installed.
Comment 14 Hui Huang 2011-02-15 07:55:51 PST
Thanks for the suggestion. I will make a new patch to check QtMobility availability when building qwebview test.

(In reply to comment #13)
> (From update of attachment 82049 [details])
> I like it, but I think the test will fail on platforms that don't have QtMobility installed.
Comment 15 Hui Huang 2011-02-15 14:02:13 PST
Thanks for the info. I think a new bug report with test steps is probably needed for Phonon, so that we can reproduce the problem and test the solution. 

(In reply to comment #12)
> I was also working on this issue. I had a similar fix for the issue. :-).
> I have tested the above patch on ubuntu and youtube html5 video playback works well.
> 
> Note: Similar fix is needed in the phonon path i.e in MediaPlayerPrivatePhonon.cpp
Comment 16 Hui Huang 2011-02-15 15:47:29 PST
Created attachment 82540 [details]
New patch after fixing build problem without Qt Multimedia. Skipping test if Qt Multimedia is not available.
Comment 17 Hui Huang 2011-02-15 19:46:35 PST
Created attachment 82575 [details]
New patch to resolve conflicts
Comment 18 Laszlo Gombos 2011-02-16 04:35:45 PST
Comment on attachment 82575 [details]
New patch to resolve conflicts

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

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:83
> +#if ENABLE(VIDEO) && ENABLE(QT_MULTIMEDIA)

Can we just use ENABLE(VIDEO) guard here - that is less code and the function could be perhaps reused for the phonon case as well ?

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:969
> +#if ENABLE(VIDEO) && ENABLE(QT_MULTIMEDIA)

Ditto.
Comment 19 Hui Huang 2011-02-16 07:11:32 PST
Thanks for the advice. The reason to use ENABLE(QT_MULTIMEDIA) is that
MediaPlayerPrivateQt.h can only be compiled with Qt Multimedia because of the dependency on QMediaPlayer. If only ENABLE(VIDEO) is used and Qt Multimedia is not available, there may be build problems. 

(In reply to comment #18)
> (From update of attachment 82575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82575&action=review
> 
> > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:83
> > +#if ENABLE(VIDEO) && ENABLE(QT_MULTIMEDIA)
> 
> Can we just use ENABLE(VIDEO) guard here - that is less code and the function could be perhaps reused for the phonon case as well ?
Comment 20 Laszlo Gombos 2011-02-16 11:31:29 PST
Comment on attachment 82575 [details]
New patch to resolve conflicts

r=me.
Comment 21 Hui Huang 2011-02-16 12:20:44 PST
Thanks a lot.

(In reply to comment #20)
> (From update of attachment 82575 [details])
> r=me.
Comment 22 WebKit Commit Bot 2011-02-16 16:47:19 PST
Comment on attachment 82575 [details]
New patch to resolve conflicts

Rejecting attachment 82575 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 1

Last 500 characters of output:
ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

Full output: http://queues.webkit.org/results/7927410
Comment 23 Ademar Reis 2011-02-17 04:26:28 PST
(In reply to comment #22)
> (From update of attachment 82575 [details])
> Rejecting attachment 82575 [details] from commit-queue.
> 
> Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 1
> 
> Last 500 characters of output:
> ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
>     step(tool, options).run(state)
>   File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run
>     if self._has_valid_reviewer(changelog_entry):
>   File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer
>     if changelog_entry.reviewer():
> AttributeError: 'NoneType' object has no attribute 'reviewer'
> 
> Full output: http://queues.webkit.org/results/7927410

A bug in commit-queue perhaps? Laszlo: can you please land it manually?
Comment 24 Laszlo Gombos 2011-02-17 05:16:39 PST
Comment on attachment 82575 [details]
New patch to resolve conflicts

Try again.
Comment 25 Yael 2011-02-17 05:36:47 PST
(In reply to comment #24)
> (From update of attachment 82575 [details])
> Try again.

happened to me too. It worked on second try :-)
Comment 26 WebKit Commit Bot 2011-02-17 05:40:01 PST
Comment on attachment 82575 [details]
New patch to resolve conflicts

Clearing flags on attachment: 82575

Committed r78817: <http://trac.webkit.org/changeset/78817>
Comment 27 WebKit Commit Bot 2011-02-17 05:40:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Ademar Reis 2011-02-17 12:43:48 PST
Revision r78817 cherry-picked into qtwebkit-2.1.x with commit 10a21c6 <http://gitorious.org/webkit/qtwebkit/commit/10a21c6>
Comment 29 Eric Seidel (no email) 2011-02-17 13:06:59 PST
(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 82575 [details] [details])
> > Rejecting attachment 82575 [details] [details] from commit-queue.
> > 
> > Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 1
> > 
> > Last 500 characters of output:
> > ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
> >     step(tool, options).run(state)
> >   File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run
> >     if self._has_valid_reviewer(changelog_entry):
> >   File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer
> >     if changelog_entry.reviewer():
> > AttributeError: 'NoneType' object has no attribute 'reviewer'
> > 
> > Full output: http://queues.webkit.org/results/7927410
> 
> A bug in commit-queue perhaps? Laszlo: can you please land it manually?

This generally happens when missing a ChangeLog (or ChangeLog entry).  Clearly we need to print a better message.