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
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
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)
*** Bug 37269 has been marked as a duplicate of this bug. ***
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)
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.
(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.
*** Bug 39722 has been marked as a duplicate of this bug. ***
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 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())?
Attachment 74761 [details] did not build on qt: Build output: http://queues.webkit.org/results/6394033
(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 :)
Created attachment 74779 [details] patch for using Qt's synchronous HTTP feature update #1
(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.
(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 on attachment 74779 [details] patch for using Qt's synchronous HTTP feature Looks good, r=me
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.
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 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>
All reviewed patches have been landed. Closing bug.
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
Reopen, because it was rolled out by http://trac.webkit.org/changeset/72736 and http://trac.webkit.org/changeset/72735
crashlogs: http://webkit.sed.hu/buildbot/results/x86-32%20Linux%20Qt%20Debug/r72734%20%2810488%29/results.html
Peter, what is the status of this?
I will work on it again after soon, after I finished some other things...
@WebKit guys: Please note that we should also cherry pick this patch into Qt 4.7.
(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?
(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.
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.
(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.
(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?
(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.
Moving target from 2.1(.0) to 2.1.x
(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.
(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).
> 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.
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 on attachment 83942 [details] Proposed patch How is this tested? Who sets the loadMode?
(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.
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 on attachment 83942 [details] Proposed patch Clearing flags on attachment: 83942 Committed r79795: <http://trac.webkit.org/changeset/79795>
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.
Revision r79795 cherry-picked into qtwebkit-2.1.x with commit 757fbfd <http://gitorious.org/webkit/qtwebkit/commit/757fbfd>