Bug 37191 – [Qt] WebKit crash in WebCore::FrameLoader::loadResourceSynchronously()

• Component: Page Loading
• Status: RESOLVED FIXED

[Benjamin Poulain]

The robustness test crashed on the page: http://shop.nationalgeographic.com/ngs/browse/productDetail.jsp?productId=1076013&code=NG93906&source=foshopsc1 with WebKit trunk.

Backtrace:
#0  0x00007ffff7175034 in WebCore::FrameLoader::loadResourceSynchronously(WebCore::ResourceRequest const&, WebCore::StoredCredentials, WebCore::ResourceError&, WebCore::ResourceResponse&, WTF::Vector<char, 0ul>&) ()
   from /home/ikipou/build/webkit/oslo-staging-1_release_64/Release/lib/libQtWebKit.so.4
#1  0x00007ffff7169f95 in WebCore::DocumentThreadableLoader::loadRequest(WebCore::ResourceRequest const&, WebCore::SecurityCheckPolicy) () from /home/ikipou/build/webkit/oslo-staging-1_release_64/Release/lib/libQtWebKit.so.4
#2  0x00007ffff716b6c8 in WebCore::DocumentThreadableLoader::DocumentThreadableLoader(WebCore::Document*, WebCore::ThreadableLoaderClient*, WebCore::DocumentThreadableLoader::BlockingBehavior, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderOptions const&) () from /home/ikipou/build/webkit/oslo-staging-1_release_64/Release/lib/libQtWebKit.so.4
#3  0x00007ffff716b9fc in WebCore::DocumentThreadableLoader::loadResourceSynchronously(WebCore::Document*, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderClient&, WebCore::ThreadableLoaderOptions const&) ()
   from /home/ikipou/build/webkit/oslo-staging-1_release_64/Release/lib/libQtWebKit.so.4
#4  0x00007ffff733c65b in WebCore::XMLHttpRequest::createRequest(int&) () from /home/ikipou/build/webkit/oslo-staging-1_release_64/Release/lib/libQtWebKit.so.4
#5  0x00007ffff733cd1b in WebCore::XMLHttpRequest::send(WebCore::String const&, int&) () from /home/ikipou/build/webkit/oslo-staging-1_release_64/Release/lib/libQtWebKit.so.4
#6  0x00007ffff733cfea in WebCore::XMLHttpRequest::send(int&) () from /home/ikipou/build/webkit/oslo-staging-1_release_64/Release/lib/libQtWebKit.so.4
#7  0x00007ffff6ee7dbe in WebCore::JSXMLHttpRequest::send(JSC::ExecState*, JSC::ArgList const&) () from /home/ikipou/build/webkit/oslo-staging-1_release_64/Release/lib/libQtWebKit.so.4
#8  0x00007ffff6e7dd14 in WebCore::jsXMLHttpRequestPrototypeFunctionSend(JSC::ExecState*, JSC::JSObject*, JSC::JSValue, JSC::ArgList const&) () from /home/ikipou/build/webkit/oslo-staging-1_release_64/Release/lib/libQtWebKit.so.4

[Andreas Kling]

This happens because we reenter the event loop during processing of synchronous XHR. Qt is currently missing the necessary API to do avoid this, so I've opened an issue in the Qt bug tracker: http://bugreports.qt.nokia.com/browse/QTBUG-9667

[Markus Goetz]

I agree that the current implementation of WebCoreSynchronousLoader is not optimal.

Small note, it crashes for me with 4.5 but not with 4.6 git and 4.7 git (Linux, webkit included in the respective Qt)

[Benjamin Poulain]

*** Bug 37269 has been marked as a duplicate of this bug. ***

[Jesus Sanchez-Palencia]

Reproduced on Snow Leopard with Qt 4.7 trunk (HEAD 03f8f1df0d88f5ffe0b3120cffce614cbeefdb70) and WebKit trunk (r59155).

Backtrace: 

0   QtWebKit                      	0x0082de3f WTF::OwnPtr<WebCore::ApplicationCacheHost>::get() const + 9 (OwnPtr.h:54)
1   QtWebKit                      	0x0082de5a WebCore::DocumentLoader::applicationCacheHost() const + 22 (DocumentLoader.h:205)
2   QtWebKit                      	0x00916784 WebCore::FrameLoader::loadResourceSynchronously(WebCore::ResourceRequest const&, WebCore::StoredCredentials, WebCore::ResourceError&, WebCore::ResourceResponse&, WTF::Vector<char, 0ul>&) + 936 (FrameLoader.cpp:3274)
3   QtWebKit                      	0x0090f8f0 WebCore::DocumentThreadableLoader::loadRequest(WebCore::ResourceRequest const&, WebCore::SecurityCheckPolicy) + 1050 (DocumentThreadableLoader.cpp:325)
4   QtWebKit                      	0x00910e4f WebCore::DocumentThreadableLoader::DocumentThreadableLoader(WebCore::Document*, WebCore::ThreadableLoaderClient*, WebCore::DocumentThreadableLoader::BlockingBehavior, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderOptions const&) + 393 (DocumentThreadableLoader.cpp:74)
5   QtWebKit                      	0x00911326 WebCore::DocumentThreadableLoader::loadResourceSynchronously(WebCore::Document*, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderClient&, WebCore::ThreadableLoaderOptions const&) + 76 (DocumentThreadableLoader.cpp:50)

[Benjamin Poulain]

Removing the blocker.

None of the crashes we have with the synchronous calls can be fixed with 4.7. The synchronous API of QNAM is not implemented.

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> Removing the blocker.
> 
> None of the crashes we have with the synchronous calls can be fixed with 4.7. The synchronous API of QNAM is not implemented.

I guess we need to document that then.

[Alexey Proskuryakov]

*** Bug 39722 has been marked as a duplicate of this bug. ***

[Peter Hartmann]

Created attachment 74761 [details]
patch for using Qt's synchronous HTTP feature

Does this commit need a layout test? No idea, this is my first proposed patch to Webkit...

[Andreas Kling]

Comment on attachment 74761 [details]
patch for using Qt's synchronous HTTP feature

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

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:56
> +const QNetworkRequest::Attribute gSynchronousNetworkRequestAttribute = static_cast<QNetworkRequest::Attribute>(QNetworkRequest::DownloadBufferAttribute + 1);

Wow, this is kinda dirty. At the very least we need a comment explaining it.

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:226
> +        m_request.setAttribute(gSynchronousNetworkRequestAttribute, true);

Should we be restricting this to QT_VERSION >= something?
Perhaps it's not necessary and it'll fall back nicely to m_loadMode = Normal..

> WebCore/platform/network/qt/ResourceHandleQt.cpp:212
> +    // when using synchronous calls, we are finished when reaching this point

Capital W, period at the end of the line. </kenneth>

> WebCore/platform/network/qt/ResourceHandleQt.cpp:215
> +    d->m_job->forwardData();
> +    d->m_job->finish();

Should these really be called if (!reply->isFinished())?

[Early Warning System Bot]

Attachment 74761 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6394033

[Benjamin Poulain]

(In reply to comment #8)
> Does this commit need a layout test? No idea, this is my first proposed patch to Webkit...

I can't think of any way to do a layout test for this.

Testing would require embedding a small http server that you control from the page. It could probably be done as an auto test if you are very motivated :)

[Peter Hartmann]

Created attachment 74779 [details]
patch for using Qt's synchronous HTTP feature

update #1

[Peter Hartmann]

(In reply to comment #9)
> (From update of attachment 74761 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74761&action=review
> 
> > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:56
> > +const QNetworkRequest::Attribute gSynchronousNetworkRequestAttribute = static_cast<QNetworkRequest::Attribute>(QNetworkRequest::DownloadBufferAttribute + 1);
> 
> Wow, this is kinda dirty. At the very least we need a comment explaining it.

Yes, it is dirty, I made a comment explaining it, it will not be cleaner before 4.8...

> 
> > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:226
> > +        m_request.setAttribute(gSynchronousNetworkRequestAttribute, true);
> 
> Should we be restricting this to QT_VERSION >= something?
> Perhaps it's not necessary and it'll fall back nicely to m_loadMode = Normal..

Yes, we set the load mode to normal in the line below; Qt versions 4.7.1 and older just ignore the attribute, so that should be safe.

> 
> > WebCore/platform/network/qt/ResourceHandleQt.cpp:212
> > +    // when using synchronous calls, we are finished when reaching this point
> 
> Capital W, period at the end of the line. </kenneth>

ok, changed.

> 
> > WebCore/platform/network/qt/ResourceHandleQt.cpp:215
> > +    d->m_job->forwardData();
> > +    d->m_job->finish();
> 
> Should these really be called if (!reply->isFinished())?

Probably that would be ok because we check the load mode in that methods, but still, I think it is easier to understand the way you suggested; they are now just called when the method is finished. The waitForCompleted method is just called when we are not finished.

[Peter Hartmann]

(In reply to comment #13)
> (In reply to comment #9)
> > (From update of attachment 74761 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=74761&action=review
> > 
> > > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:56
> > > +const QNetworkRequest::Attribute gSynchronousNetworkRequestAttribute = static_cast<QNetworkRequest::Attribute>(QNetworkRequest::DownloadBufferAttribute + 1);
> > 
> > Wow, this is kinda dirty. At the very least we need a comment explaining it.
> 
> Yes, it is dirty, I made a comment explaining it, it will not be cleaner before 4.8...

btw. using the QNetworkRequest::HttpPipeliningWasUsedAttribute + 7 now is to make it compile with 4.6, the other attributes were not there then.

[Andreas Kling]

Comment on attachment 74779 [details]
patch for using Qt's synchronous HTTP feature

Looks good, r=me

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74779 [details]:

http/tests/appcache/foreign-fallback.html
fast/workers/storage/use-same-database-in-page-and-workers.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74779 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74779 [details]
patch for using Qt's synchronous HTTP feature

Clearing flags on attachment: 74779

Committed r72732: <http://trac.webkit.org/changeset/72732>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/72732 might have broken Qt Linux Release
The following tests are not passing:
dom/svg/level3/xpath/Attribute_Nodes_xmlns.svg
dom/svg/level3/xpath/Conformance_Expressions.svg
dom/svg/level3/xpath/Conformance_hasFeature_3.svg
dom/svg/level3/xpath/Conformance_hasFeature_null.svg
dom/svg/level3/xpath/Conformance_isSupported_empty.svg
dom/svg/level3/xpath/Element_Nodes.svg
dom/svg/level3/xpath/Text_Nodes.svg
dom/svg/level3/xpath/XPathEvaluator_createExpression_INVALID_EXPRESSION_ERR.svg
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_02.svg
dom/svg/level3/xpath/XPathEvaluator_createExpression_no_NS.svg
dom/svg/level3/xpath/XPathEvaluator_createNSResolver_document.svg
dom/svg/level3/xpath/XPathEvaluator_evaluate_INVALID_EXPRESSION_ERR.svg
dom/svg/level3/xpath/XPathEvaluator_evaluate_NOT_SUPPORTED_ERR.svg
dom/svg/level3/xpath/XPathEvaluator_evaluate_WRONG_DOCUMENT_ERR.svg
dom/svg/level3/xpath/XPathEvaluator_evaluate_documentElement.svg
dom/svg/level3/xpath/XPathExpression_evaluate_WRONG_DOCUMENT_ERR.svg
dom/svg/level3/xpath/XPathExpression_evaluate_documentElement.svg
dom/svg/level3/xpath/XPathNSResolver_lookupNamespaceURI_null.svg
dom/svg/level3/xpath/XPathNSResolver_lookupNamespaceURI_xml.svg
dom/svg/level3/xpath/XPathResult_booleanValue_false.svg

[Csaba Osztrogonác]

Reopen, because it was rolled out by http://trac.webkit.org/changeset/72736 and http://trac.webkit.org/changeset/72735

[Csaba Osztrogonác]

crashlogs: http://webkit.sed.hu/buildbot/results/x86-32%20Linux%20Qt%20Debug/r72734%20%2810488%29/results.html

[Benjamin Poulain]

Peter, what is the status of this?

[Peter Hartmann]

I will work on it again after soon, after I finished some other things...

[Markus Goetz]

@WebKit guys: Please note that we should also cherry pick this patch into Qt 4.7.

[Benjamin Poulain]

(In reply to comment #25)
> @WebKit guys: Please note that we should also cherry pick this patch into Qt 4.7.

Ademar, this is a very nasty bug. Could you make sure the patches find their way to releases and everything is tested correctly?

[Ademar Reis]

(In reply to comment #26)
> (In reply to comment #25)
> > @WebKit guys: Please note that we should also cherry pick this patch into Qt 4.7.
> 
> Ademar, this is a very nasty bug. Could you make sure the patches find their way to releases and everything is tested correctly?

Adding it as a blocker for 2.1. Will have to check manually how to submit the change for 2.0 (qt47) later.

[Peter Hartmann]

Created attachment 82767 [details]
patch for using Qt's synchronous HTTP feature with threaded architecture

the attachment fixes the crash in the browser, but causes several of the layout tests to crash or timeout, so it is not fully working yet.

[Ademar Reis]

(In reply to comment #28)
> Created an attachment (id=82767) [details]
> patch for using Qt's synchronous HTTP feature with threaded architecture
> 
> the attachment fixes the crash in the browser, but causes several of the layout tests to crash or timeout, so it is not fully working yet.

Peter, any plans to still work on that? The window to submit patches to 2.1 is about to close.

[Peter Hartmann]

(In reply to comment #29)
(...)
> Peter, any plans to still work on that? The window to submit patches to 2.1 is about to close.

I think I will look at it together with Jocelyn. How long can we submit patches to 2.1?

[Ademar Reis]

(In reply to comment #30)
> (In reply to comment #29)
> (...)
> > Peter, any plans to still work on that? The window to submit patches to 2.1 is about to close.
> 
> I think I will look at it together with Jocelyn. How long can we submit patches to 2.1?

We're waiting the green light from the Symbian team which is our main target for 2.1.0. Our expectation is to declare the next weekly release a RC.

[Ademar Reis]

Moving target from 2.1(.0) to 2.1.x

[Peter Hartmann]

(In reply to comment #30)
> (In reply to comment #29)
> (...)
> > Peter, any plans to still work on that? The window to submit patches to 2.1 is about to close.
> 
> I think I will look at it together with Jocelyn. How long can we submit patches to 2.1?

update: Qt 4.8 will contain threaded networking of its own, so there is no need anymore for webkit to have the QNetworkAccessManager in its own thread. Once the webkit threaded version has been reverted, we will look into this again.

[Ademar Reis]

(In reply to comment #33)
> (In reply to comment #30)
> > (In reply to comment #29)
> > (...)
> > > Peter, any plans to still work on that? The window to submit patches to 2.1 is about to close.
> > 
> > I think I will look at it together with Jocelyn. How long can we submit patches to 2.1?
> 
> update: Qt 4.8 will contain threaded networking of its own, so there is no need anymore for webkit to have the QNetworkAccessManager in its own thread. Once the webkit threaded version has been reverted, we will look into this again.

You're saying we should live with this bug until qt-4.8 is released? I thought this was a "very nasty bug that needs to be included in qt-4.7" (Comment #26).

[Peter Hartmann]

> You're saying we should live with this bug until qt-4.8 is released? I thought this was a "very nasty bug that needs to be included in qt-4.7" (Comment #26).

Maybe it would be a good idea to look into this earlier; however I am not working on Webkit anymore. Feel free to contact somebody else from that team.

[Andreas Kling]

Created attachment 83942 [details]
Proposed patch

Updated patch based on the earlier work.
Don't hook up signals from the synchronous reply if it's successfully completed in start().

[Kenneth Rohde Christiansen]

Comment on attachment 83942 [details]
Proposed patch

How is this tested? Who sets the loadMode?

[Andreas Kling]

(In reply to comment #37)
> (From update of attachment 83942 [details])
> How is this tested? Who sets the loadMode?

The loadMode is passed as an argument to the QNetworkReplyHandler constructor.

As for testing, synchronous XHR is covered by a plethora of layout tests. Targeting the crash is virtually impossible though, since what we end up is [random] reentrancy while waiting for request completion.

[Kenneth Rohde Christiansen]

OK I saw it now:

204     d->m_job = new QNetworkReplyHandler(handle.get(), QNetworkReplyHandler::LoadNormal);
 210    d->m_job = new QNetworkReplyHandler(handle.get(), QNetworkReplyHandler::LoadSynchronously);

Le'ts give it a shot.

[Andreas Kling]

Comment on attachment 83942 [details]
Proposed patch

Clearing flags on attachment: 83942

Committed r79795: <http://trac.webkit.org/changeset/79795>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

It is really nice you fixed this bug. Those crashes were such a pain.

Maybe a followup patch should #ifdef WebCoreSynchronousLoader for Qt 4.8? This way we won't forget to remove it when 4.9 is release.

[Ademar Reis]

Revision r79795 cherry-picked into qtwebkit-2.1.x with commit 757fbfd <http://gitorious.org/webkit/qtwebkit/commit/757fbfd>