Bug 53973 – [Qt] Can't play HTML5 Video because of the URL is percent encoded twice

• Component: Media
• Status: RESOLVED FIXED

[Hui Huang]

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.

[Hui Huang]

Created attachment 81577 [details]
Proposed patch

[Hui Huang]

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.

[Benjamin Poulain]

Comment on attachment 81577 [details]
Proposed patch

Setting review flag.

[Benjamin Poulain]

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.

[Andreas Kling]

Comment on attachment 81577 [details]
Proposed patch

Needs a regression test.

[Hui Huang]

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.

[Simon Hausmann]

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 :)

[Hui Huang]

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 :)

[Hui Huang]

Created attachment 82039 [details]
New patch with regression test

[Hui Huang]

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.

[Hui Huang]

Created attachment 82049 [details]
New patch after resolving conflicts

[Harsha]

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

[Simon Hausmann]

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.

[Hui Huang]

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.

[Hui Huang]

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

[Hui Huang]

Created attachment 82540 [details]
New patch after fixing build problem without Qt Multimedia. Skipping test if Qt Multimedia is not available.

[Hui Huang]

Created attachment 82575 [details]
New patch to resolve conflicts

[Laszlo Gombos]

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.

[Hui Huang]

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 ?

[Laszlo Gombos]

Comment on attachment 82575 [details]
New patch to resolve conflicts

r=me.

[Hui Huang]

Thanks a lot.

(In reply to comment #20)
> (From update of attachment 82575 [details])
> r=me.

[WebKit Commit Bot]

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

[Ademar Reis]

(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?

[Laszlo Gombos]

Comment on attachment 82575 [details]
New patch to resolve conflicts

Try again.

[Yael]

(In reply to comment #24)
> (From update of attachment 82575 [details])
> Try again.

happened to me too. It worked on second try :-)

[WebKit Commit Bot]

Comment on attachment 82575 [details]
New patch to resolve conflicts

Clearing flags on attachment: 82575

Committed r78817: <http://trac.webkit.org/changeset/78817>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r78817 cherry-picked into qtwebkit-2.1.x with commit 10a21c6 <http://gitorious.org/webkit/qtwebkit/commit/10a21c6>

[Eric Seidel (no email)]

(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.