Bug 37191

Summary: [Qt] WebKit crash in WebCore::FrameLoader::loadResourceSynchronously()
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: Page LoadingAssignee: 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: P1 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

Description Benjamin Poulain 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
Comment 1 Andreas Kling 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
Comment 2 Markus Goetz 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)
Comment 3 Benjamin Poulain 2010-04-30 04:21:09 PDT
*** Bug 37269 has been marked as a duplicate of this bug. ***
Comment 4 Jesus Sanchez-Palencia 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)
Comment 5 Benjamin Poulain 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.
Comment 6 Kenneth Rohde Christiansen 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.
Comment 7 Alexey Proskuryakov 2010-06-04 23:07:34 PDT
*** Bug 39722 has been marked as a duplicate of this bug. ***
Comment 8 Peter Hartmann 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...
Comment 9 Andreas Kling 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())?
Comment 10 Early Warning System Bot 2010-11-24 08:18:51 PST
Attachment 74761 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6394033
Comment 11 Benjamin Poulain 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 :)
Comment 12 Peter Hartmann 2010-11-24 10:33:31 PST
Created attachment 74779 [details]
patch for using Qt's synchronous HTTP feature

update #1
Comment 13 Peter Hartmann 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.
Comment 14 Peter Hartmann 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.
Comment 15 Andreas Kling 2010-11-25 06:15:56 PST
Comment on attachment 74779 [details]
patch for using Qt's synchronous HTTP feature

Looks good, r=me
Comment 16 WebKit Commit Bot 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.
Comment 17 WebKit Commit Bot 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.
Comment 18 WebKit Commit Bot 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>
Comment 19 WebKit Commit Bot 2010-11-25 08:19:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Review Bot 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
Comment 21 Csaba Osztrogon√°c 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
Comment 23 Benjamin Poulain 2011-01-07 11:45:42 PST
Peter, what is the status of this?
Comment 24 Peter Hartmann 2011-01-10 00:59:02 PST
I will work on it again after soon, after I finished some other things...
Comment 25 Markus Goetz 2011-02-08 03:29:09 PST
@WebKit guys: Please note that we should also cherry pick this patch into Qt 4.7.
Comment 26 Benjamin Poulain 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?
Comment 27 Ademar Reis 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.
Comment 28 Peter Hartmann 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.
Comment 29 Ademar Reis 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.
Comment 30 Peter Hartmann 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?
Comment 31 Ademar Reis 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.
Comment 32 Ademar Reis 2011-02-21 06:23:31 PST
Moving target from 2.1(.0) to 2.1.x
Comment 33 Peter Hartmann 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.
Comment 34 Ademar Reis 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).
Comment 35 Peter Hartmann 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.
Comment 36 Andreas Kling 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().
Comment 37 Kenneth Rohde Christiansen 2011-02-26 08:02:55 PST
Comment on attachment 83942 [details]
Proposed patch

How is this tested? Who sets the loadMode?
Comment 38 Andreas Kling 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.
Comment 39 Kenneth Rohde Christiansen 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.
Comment 40 Andreas Kling 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>
Comment 41 Andreas Kling 2011-02-26 09:12:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 Benjamin Poulain 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.
Comment 43 Ademar Reis 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>