[Qt] Assertion failure in DocumentLoader::commitData when loading a media document

[Benjamin Poulain]

Despite https://bugs.webkit.org/show_bug.cgi?id=48764 QtWebKit cannot handle media element as the document.

To reproduce, open http://www.nch.com.au/acm/sample.wav with a QtWebKit in debug.

The following assertion fail:
ASSERTION FAILED: m_frame->document()->parsing()
(../../../WebCore/loader/DocumentLoader.cpp:307 void WebCore::DocumentLoader::commitData(const char*, int))

[Joe Wild]

Created attachment 89853 [details]
Proposed patch for handling Audio as top level content

I did not check the review flag yet since I am in the process adding a test 
and generating Mac results. Will resubmit when that's ready.  

There a number of directions to go with this, so any feedback would be appreciated.

So what it looks like is happening.  When you select a link (<a>) to the
audio/x-wav content at url: http://www.nch.com.au/acm/sample.wav, I think
the following is supposed to happen.

  Load of url is issued.
  Content coming back is handled in MediaDocumentParser
    (./WebCore/html/MediaDocument.cpp). The content comes back 
    to MediaDocumentParser::appendBytes. 
  appendBytes calls createDocumentStructure() to create an HTML wrapper
   around video element with the src attribute of the original url.
  Then the current load of the url is terminated/finish and the generated HTML
   should be processed to handle the audio/x-wav content.

What seems to be going wrong is that the 1st load of the url continues
(not completely terminated), and that causes problems.

My best guess at a fix is just to cancel the 1st load:
  
  void MediaDocumentParser::createDocumentStructure()
  {
  ...
      frame->loader()->activeDocumentLoader()->mainResourceLoader()->setShouldBufferData(false);
  added ->  // Cancel the current load of url. Will handle inside the HTML video element created above.
  added ->  frame->loader()->activeDocumentLoader()->mainResourceLoader()->cancel();
  }

Another alternative would be to create the HTML skeleton earlier.
Canceling the 1st load seemed like the safest fix. It seems a little odd to me that
MediaDocumentParser::appendBytes is the function to create the HTML
wrapper skeleton.  I can see that is has been this way for a long
time.  Is there a reason we don't create the HTML wrapper when the
MediaDocumentParser is created?

Also, I'm not sure what the expected behavior should be.  At least,
it does not reset or assert with this fix, but the audio does not play in
QtTestBrowser on Linux.

[Joe Wild]

Created attachment 91036 [details]
Cancel load of top level media content before reset/assert

If this fix it correct it was very simple.  As several others have
pointed out, the continued load of content that will be handled by the
MediaDocument causes a reset/assert.  My fix just cancels that load.

I was not unable to find much information on how the
MediaDocument/Plugins should behave, so I focused on fixing the reset.
From what I can tell the HTML with Video Element that is generated is
never correctly processed due to unimplemented features (at least on
QtWebkit Linux).  Same happens with hand created HTML with a video
element.

Also, this error has been around for awhile.  These 2 errors were investigated
in 2009.  One was marked as a dup of the other, and the other was closed
as Resolved Invalid on 2009-09-08 with no comment why.

  https://bugs.webkit.org/show_bug.cgi?id=25427
  https://bugs.webkit.org/show_bug.cgi?id=25429

I added a test that just loads a .wav file as the top level content of an
<iframe> under

  LayoutTests/plugins/audio-top-level-wav-load.html.

For this to really be tested with DumpRenderTree on QtWebKit,
QTWEBKIT_PLUGIN_PATH has to be defined so the Plugins are loaded to
get the point of the reset/assert.  Not sure if this is the best place
to put this test.  Open to suggestions.  I picked LayoutTest/plugins over
LayoutTests/media because the latter are not run on QtWebkit Linux.

I did test on Mac QtWebkit.

[Simon Hausmann]

Comment on attachment 91036 [details]
Cancel load of top level media content before reset/assert

I don't think that this is the correct fix. The assertion is correct I believe, and this works in the other ports. The cancellation of the first load should be issued by FrameLoaderClientQt::committedLoad.

See also http://webkit.org/b/48762 and http://trac.webkit.org/changeset/51104

[Joe Wild]

https://bugs.webkit.org/show_bug.cgi?id=49468
Bug 49468 - [Qt] Assertion failure in DocumentLoader::commitData when loading a media document

Simon's suggestion to cancel the load in FrameLoaderClientQt::committedLoad
as is done in gtk is a good improvement.  Much better to do in platform
specific code that general webkit code.

Need to investigate further though.  Canceling here happens after the
canceled load is added to the history stack, leaving a wrong history stack.
Popping that element off the stack seems to fix the problem.  I just need
to verify that is the correct and best way to fix this history stack problem.

[Joe Wild]

Created attachment 92484 [details]
Moved canceling to FrameLoaderClientQt::committedLoad

I had some doubts about the test for this, so submitted as a separate
patch to review.

Cancel loads in FrameLoaderClientQt::committedLoad that will
be handled by the MediaDocument handler. If they are allowed
to continue they will assert/reset.

Canceling here can cause the history stack to end up in the wrong state,
so there is some code here to clean that up.

Also, I included an test case for this in a separate patch.  This test
should pass on all platforms, but is only really tested with the
right on Qt and when the right plugins are loaded.  Without these
the test won't get to the place where the error occurred.

It was starting to get messy to get this test to always work so I just
added a simple test.  Some of the things that stop this from working.
  * run-webkit-tests doesn't find the right plugins since it defines
    its own QTWEBKIT_PLUGIN_PATH value.  I had to copy the plugin to
    my local test directory.
  * Only Qt DumpRenderTree supports layoutTestController.handleErrorPages()
    needed to execute the code for this test.
  * And finally even under Qt this case is not handled since it only
    supports this error handling mechanism for the main frame.  I tried
    not using a "meta refresh" instead of the iframe for the test, but
    took the test to a generated page for the plugin where I had no control.

[Joe Wild]

Created attachment 92485 [details]
Test case for patch

Test that a .wav file can be successfully loaded in an <iframe>.

[WebKit Review Bot]

Created attachment 92532 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexis Menard (darktears)]

Comment on attachment 92484 [details]
Moved canceling to FrameLoaderClientQt::committedLoad

Please move the test case + the patch in the code into one patch. Thanks.

[Joe Wild]

Created attachment 94838 [details]
Put changes and tests together in one patch.

I put the change and test into one patch.

I put the test under platform/qt since it uses
layoutTestController.handleErrorPages() which is only available with
Qt and my change was in Qt files.

I have to admit that I hate this test for all the reasons mentioned
in comment #5, but don't have any good ideas of how to enable the plugin
support better for run-webkit-tests.  Suggestions welcome.

At least, I don't think this test should cause noise without the proper
plugin setup. It will just pass.  Problem is that it will likely not
fail when it is supposed to.

[WebKit Review Bot]

Comment on attachment 94838 [details]
Put changes and tests together in one patch.

Attachment 94838 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8733706

New failing tests:
platform/qt/plugins/audio-top-level-wav-load.html

[WebKit Review Bot]

Created attachment 94854 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Simon Hausmann]

Comment on attachment 94838 [details]
Put changes and tests together in one patch.

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

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:930
> +            // When Plugins and Error handling are enabled, the cancel operation
> +            // may put a duplicate item on the history stack. This only happens
> +            // on certain configurations.
> +            if (currentItemBeforeCancel && currentItemAfterCancel
> +                && currentItemBeforeCancel != currentItemAfterCancel
> +                && currentItemBeforeCancel->originalURL() == currentItemAfterCancel->originalURL()) {
> +                // There are 2 duplicate items on top of stack. Pop the top one.
> +                page->backForward()->setCurrentItem(currentItemBeforeCancel);

This doesn't seem right. Why do only we need this but not the other ports?

I suggest to do a build of another port (Gtk for example) and try to see where the history item is cleared in the other ports. Note that http://trac.webkit.org/changeset/51104 also changes shouldFallBack(). Either way I think we should forward-port 51104 to WebKit/qt.

[Alexis Menard (darktears)]

Comment on attachment 94838 [details]
Put changes and tests together in one patch.

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

>> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:930
>> +                page->backForward()->setCurrentItem(currentItemBeforeCancel);
> 
> This doesn't seem right. Why do only we need this but not the other ports?
> 
> I suggest to do a build of another port (Gtk for example) and try to see where the history item is cleared in the other ports. Note that http://trac.webkit.org/changeset/51104 also changes shouldFallBack(). Either way I think we should forward-port 51104 to WebKit/qt.

Sounds like a hack to me.  Why we have duplicates in the stack in the first place?

[Simon Hausmann]

Comment on attachment 94838 [details]
Put changes and tests together in one patch.

Alexis and I have doubts, see earlier comments. r- until it is clear why we need this history handling but the other ports don't. If there is a good reason, then it should be documented.

[Joe Wild]

Thanks.  I'm continuing to investigate along with a couple of other things I'm working on.  I'll see what Gtk does.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.