Bug 37191

Summary:

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

Product:

WebKit

Reporter:

Benjamin Poulain <benjamin>

Component:

Page Loading

Assignee:

Andreas Kling <kling>

Status:

RESOLVED FIXED

Severity:

Critical

CC:

abarth, ademar, benjamin, cmarcelo, commit-queue, eric, hausmann, jesus, kenneth, kling, kokilakr, laszlo.gombos, luiz, markus, ossy, peter.hartmann, webkit-ews, webkit.review.bot

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

http://shop.nationalgeographic.com/ngs/browse/productDetail.jsp?productId=1076013&code=NG93906&source=foshopsc1

Bug Depends on:

50089

Bug Blocks:

Attachments:

Description	Flags
patch for using Qt's synchronous HTTP feature	none
patch for using Qt's synchronous HTTP feature	none
patch for using Qt's synchronous HTTP feature with threaded architecture	none
Proposed patch	none

Benjamin Poulain

Reported 2010-04-07 01:17:36 PDT

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

Attachments
patch for using Qt's synchronous HTTP feature (4.94 KB, patch) 2010-11-24 08:08 PST, Peter Hartmann	no flags	Details Formatted Diff Diff
patch for using Qt's synchronous HTTP feature (5.12 KB, patch) 2010-11-24 10:33 PST, Peter Hartmann	no flags	Details Formatted Diff Diff
patch for using Qt's synchronous HTTP feature with threaded architecture (11.06 KB, patch) 2011-02-17 01:11 PST, Peter Hartmann	no flags	Details Formatted Diff Diff
Proposed patch (5.74 KB, patch) 2011-02-26 07:45 PST, Andreas Kling	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Andreas Kling

Comment 1 2010-04-07 06:20:40 PDT

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

Comment 2 2010-04-07 07:21:22 PDT

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

Comment 3 2010-04-30 04:21:09 PDT

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

Jesus Sanchez-Palencia

Comment 4 2010-05-12 13:48:07 PDT

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

Comment 5 2010-05-12 14:11:17 PDT

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

Comment 6 2010-05-12 14:23:17 PDT

(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

Comment 7 2010-06-04 23:07:34 PDT

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

Peter Hartmann

Comment 8 2010-11-24 08:08:09 PST

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 9 2010-11-24 08:18:43 PST

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

Comment 10 2010-11-24 08:18:51 PST

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

Benjamin Poulain

Comment 11 2010-11-24 10:06:49 PST

(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

Comment 12 2010-11-24 10:33:31 PST

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

Peter Hartmann

Comment 13 2010-11-24 10:36:24 PST

(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

Comment 14 2010-11-24 10:42:41 PST

(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 15 2010-11-25 06:15:56 PST

Comment on attachment 74779 [details] patch for using Qt's synchronous HTTP feature Looks good, r=me

WebKit Commit Bot

Comment 16 2010-11-25 07:31:00 PST

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 ap@webkit.org and dumi@chromium.org. The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 17 2010-11-25 08:17:00 PST

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 dumi@chromium.org. The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 18 2010-11-25 08:19:53 PST

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

Comment 19 2010-11-25 08:19:59 PST

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 20 2010-11-25 08:44:32 PST

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

Comment 21 2010-11-25 09:51:48 PST

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

Csaba Osztrogonác

Comment 22 2010-11-25 09:53:09 PST

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

Benjamin Poulain

Comment 23 2011-01-07 11:45:42 PST

Peter, what is the status of this?

Peter Hartmann

Comment 24 2011-01-10 00:59:02 PST

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

Markus Goetz

Comment 25 2011-02-08 03:29:09 PST

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

Benjamin Poulain

Comment 26 2011-02-08 03:42:49 PST

(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

Comment 27 2011-02-08 07:49:49 PST

(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

Comment 28 2011-02-17 01:11:57 PST

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

Comment 29 2011-02-17 12:50:08 PST

(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

Comment 30 2011-02-18 01:57:42 PST

(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

Comment 31 2011-02-18 05:42:42 PST

(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

Comment 32 2011-02-21 06:23:31 PST

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

Peter Hartmann

Comment 33 2011-02-21 06:27:29 PST

(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

Comment 34 2011-02-21 06:43:26 PST

(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

Comment 35 2011-02-22 08:43:09 PST

> 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

Comment 36 2011-02-26 07:45:08 PST

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 37 2011-02-26 08:02:55 PST

Comment on attachment 83942 [details] Proposed patch How is this tested? Who sets the loadMode?

Andreas Kling

Comment 38 2011-02-26 08:07:32 PST

(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

Comment 39 2011-02-26 08:09:42 PST

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 40 2011-02-26 09:12:40 PST

Comment on attachment 83942 [details] Proposed patch Clearing flags on attachment: 83942 Committed r79795: <http://trac.webkit.org/changeset/79795>

Andreas Kling

Comment 41 2011-02-26 09:12:51 PST

All reviewed patches have been landed. Closing bug.

Benjamin Poulain

Comment 42 2011-02-26 16:35:21 PST

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

Comment 43 2011-02-28 14:03:04 PST

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

Note You need to log in before you can comment on or make changes to this bug.