Bug 62808 – [Qt] ASSERTION FAILED in ResourceHandle::setDefersLoading causes crash

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Yi Shen]

To reproduce the crash,
1) Launch QtTestBrowser
2) Load the test page (see the attachment)
3) Click the button on the page, it pops an alert bot. 
4) Close the alert box causes the crash.

The calling stack,

ASSERTION FAILED: d->m_defersLoading != defers
../../../Source/WebCore/platform/network/ResourceHandle.cpp(162) : void WebCore::ResourceHandle::setDefersLoading(bool)

Program received signal SIGSEGV, Segmentation fault.
0x01dfef2d in WebCore::ResourceHandle::setDefersLoading (this=0x837d540, defers=false) at ../../../Source/WebCore/platform/network/ResourceHandle.cpp:162
162	    ASSERT(d->m_defersLoading != defers); // Deferring is not counted, so calling setDefersLoading() repeatedly is likely to be in error.
(gdb) i s
#0  0x01dfef2d in WebCore::ResourceHandle::setDefersLoading (this=0x837d540, defers=false) at ../../../Source/WebCore/platform/network/ResourceHandle.cpp:162
#1  0x01cc9233 in WebCore::ResourceLoader::setDefersLoading (this=0x837e338, defers=false) at ../../../Source/WebCore/loader/ResourceLoader.cpp:172
#2  0x01c87fab in setAllDefersLoading (loaders=..., defers=false) at ../../../Source/WebCore/loader/DocumentLoader.cpp:78
#3  0x01c8c546 in WebCore::DocumentLoader::setDefersLoading (this=0x8310308, defers=false) at ../../../Source/WebCore/loader/DocumentLoader.cpp:750
#4  0x01c99ce3 in WebCore::FrameLoader::setDefersLoading (this=0x830a280, defers=false) at ../../../Source/WebCore/loader/FrameLoader.cpp:253
#5  0x01d6dde9 in WebCore::Page::setDefersLoading (this=0x82512c8, defers=false) at ../../../Source/WebCore/page/Page.cpp:565
#6  0x01d81a74 in ~PageGroupLoadDeferrer (this=0xbfffd90c, __in_chrg=<value optimized out>) at ../../../Source/WebCore/page/PageGroupLoadDeferrer.cpp:72
#7  0x01cf3b1a in WebCore::Chrome::runJavaScriptAlert (this=0x824b670, frame=0x826eeb8, message=...) at ../../../Source/WebCore/page/Chrome.cpp:303
#8  0x01d0cd11 in WebCore::DOMWindow::alert (this=0x8245790, message=...) at ../../../Source/WebCore/page/DOMWindow.cpp:980
....

[Yi Shen]

Created attachment 97478 [details]
Test page

[Yi Shen]

If I comment out the assertion in ResourceHandle::setDefersLoading(bool), another assertion failure is observed later after click the button on test page several times.

Following is the trace log,

ASSERTION FAILED: m_suspended
../../../Source/WebCore/page/SuspendableTimer.cpp(76) : virtual void WebCore::SuspendableTimer::resume()

Program received signal SIGSEGV, Segmentation fault.
0x01d95908 in WebCore::SuspendableTimer::resume (this=0x9190d38)
    at ../../../Source/WebCore/page/SuspendableTimer.cpp:76
76	    ASSERT(m_suspended);
(gdb) i s
#0  0x01d95908 in WebCore::SuspendableTimer::resume (this=0x9190d38)
    at ../../../Source/WebCore/page/SuspendableTimer.cpp:76
#1  0x01a44f6b in WebCore::ScriptExecutionContext::resumeActiveDOMObjects (
    this=0x8fbb4f0)
    at ../../../Source/WebCore/dom/ScriptExecutionContext.cpp:259
#2  0x01d81a9f in ~PageGroupLoadDeferrer (this=0xbfffd90c, 
    __in_chrg=<value optimized out>)
    at ../../../Source/WebCore/page/PageGroupLoadDeferrer.cpp:75
#3  0x01cf3b1a in WebCore::Chrome::runJavaScriptAlert (this=0x81ecea8, 
    frame=0x8234da8, message=...)
    at ../../../Source/WebCore/page/Chrome.cpp:303
#4  0x01d0cd11 in WebCore::DOMWindow::alert (this=0x8223778, message=...)
    at ../../../Source/WebCore/page/DOMWindow.cpp:980

[Yi Shen]

Created attachment 97809 [details]
proposed fix

I saw there is one logical error for this assertion failure is that, for Qt's implementation of ResourceHandle::platformSetDefersLoading & NetworkReplyHandler::setLoadingDeferred, the deferred loading data get forwarded to document loader too early.

When resume a page loading, webkit calls DocumentLoader::setDefersLoading(false), which first calls setDefersLoading to false on the main resource loader, then on all the sub resource loaders. For Qt, once the main resource loader gets resumed, it forwards all the deferred loading data to the document loader & parser, which leads to create some new sub-resource loaders. Next, when document loader resumes the sub resource loaders, it calls setDefersLoading(false) on these new created sub-resource loaders, which is incorrect by assumption (that's why the assertion fails).

To fix this issue, one solution could be forwarding the deferred data after DocumentLoader::setDefersLoading(false) finished. So, I added a timer (the timeout is set to 0) in my patch. Please let me know if there is better solution. Thanks a lot.

[Alexis Menard (darktears)]

Comment on attachment 97809 [details]
proposed fix

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

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:416
> +void QNetworkReplyHandler::setDeferSignalToFalse()

Am I missing something or this function is not used anywhere?

[Yi Shen]

(In reply to comment #4)
> (From update of attachment 97809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97809&action=review
> 
> > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:416
> > +void QNetworkReplyHandler::setDeferSignalToFalse()
> 
> Am I missing something or this function is not used anywhere?

Alexis, I will rework this patch since it has a small error. (I tested&copied the patch from another branch, but missed something :( )

[Benjamin Poulain]

Quick comments. No idea if the patch is the right solution (maybe Luiz can help here) but I don't like the patch in general because:
-deferring the change of a setter is a bad idea, that leads to hard to read code. You should have imperative way of setting deferred with queueing.
-You don't need a QTimer here since the current object is a QObject. A single shot timer on self would be cleaner (QBasicTimer for example)
-It looks like you just want to have a signal in the end of the event queue. You probably don't even need a timer here.

[Yi Shen]

(In reply to comment #6)
> Quick comments. No idea if the patch is the right solution (maybe Luiz can help here) but I don't like the patch in general because:
> -deferring the change of a setter is a bad idea, that leads to hard to read code. You should have imperative way of setting deferred with queueing.
> -You don't need a QTimer here since the current object is a QObject. A single shot timer on self would be cleaner (QBasicTimer for example)
> -It looks like you just want to have a signal in the end of the event queue. You probably don't even need a timer here.

Thanks for the comments. I have reworked and tested the patch yesterday and realized it is not a correct way to fix this assertion failure -- I saw the same assertion failure occurs later (the calling stack is a different one) if I keep hitting the test button on the test page.

PS: Also, I noticed that there is no crash for the release build (which has no assertion of course) when I run my test page on the QtTestBrowser. So, I tried to remove/comment out all the assertion failures I got during the test debug build. After removed about 7 assert code, I can run my test page without any problem ...

[Luiz Agostini]

ap, it seems that you have introduced the assertion in ResourceHandle::setDefersLoading (http://trac.webkit.org/changeset/61768). Do you agree with removing it?

In mac port, may connection callbacks get called inside a wkSetNSURLConnectionDefersCallbacks(d->m_connection.get(), true) call?

[Alexey Proskuryakov]

> ap, it seems that you have introduced the assertion in ResourceHandle::setDefersLoading (http://trac.webkit.org/changeset/61768). Do you agree with removing it?

I don't understand the reason for this question. There doesn't seem to be any mention of this assertion being wrong in the discussion above - and instead, there is a comment saying that even if it's removed, another assertion fires later.

[Yi Shen]

(In reply to comment #9)
> > ap, it seems that you have introduced the assertion in ResourceHandle::setDefersLoading (http://trac.webkit.org/changeset/61768). Do you agree with removing it?
> 
> I don't understand the reason for this question. There doesn't seem to be any mention of this assertion being wrong in the discussion above - and instead, there is a comment saying that even if it's removed, another assertion fires later.


(In reply to comment #8)
> ap, it seems that you have introduced the assertion in ResourceHandle::setDefersLoading (http://trac.webkit.org/changeset/61768). Do you agree with removing it?
> 
> In mac port, may connection callbacks get called inside a wkSetNSURLConnectionDefersCallbacks(d->m_connection.get(), true) call?

Hi Luiz, I just debugged mac port, and it seems the connection callbacks DO NOT get called inside wkSetNSURLConnectionDefersCallbacks(). See the call stack below. 

#0 0x1018c7f01 in WebCore::Document::implicitOpen at Document.cpp:1984
#1 0x101900eac in WebCore::DocumentWriter::begin at DocumentWriter.cpp:145
#2 0x101a78476 in WebCore::FrameLoader::receivedFirstData at FrameLoader.cpp:576
#3 0x101a786e6 in WebCore::FrameLoader::willSetEncoding at FrameLoader.cpp:976
#4 0x10190032b in WebCore::DocumentWriter::setEncoding at DocumentWriter.cpp:242
#5 0x1018e9d3f in WebCore::DocumentLoader::commitData at DocumentLoader.cpp:319
#6 0x1010a8679 in -[WebFrame(WebInternal) _commitData:] at WebFrame.mm:869
#7 0x1010d25c2 in -[WebHTMLRepresentation finishedLoadingWithDataSource:] at WebHTMLRepresentation.mm:219
#8 0x10108d3b3 in -[WebDataSource(WebInternal) _finishedLoading] at WebDataSource.mm:229
#9 0x1010b3d64 in WebFrameLoaderClient::finishedLoading at WebFrameLoaderClient.mm:886
#10 0x101a7c2fb in WebCore::FrameLoader::finishedLoadingDocument at FrameLoader.cpp:2099
#11 0x1018e9f59 in WebCore::DocumentLoader::finishedLoading at DocumentLoader.cpp:287
#12 0x101a7b787 in WebCore::FrameLoader::finishedLoading at FrameLoader.cpp:2065
#13 0x10200cf13 in WebCore::MainResourceLoader::didFinishLoading at MainResourceLoader.cpp:485
#14 0x10200e818 in WebCore::MainResourceLoader::continueAfterContentPolicy at MainResourceLoader.cpp:320
#15 0x10200e8ef in WebCore::MainResourceLoader::continueAfterContentPolicy at MainResourceLoader.cpp:334
#16 0x10200e919 in WebCore::MainResourceLoader::callContinueAfterContentPolicy at MainResourceLoader.cpp:326
#17 0x1020c96fc in WebCore::PolicyCallback::call at PolicyCallback.cpp:114
#18 0x1020ca003 in WebCore::PolicyChecker::continueAfterContentPolicy at PolicyChecker.cpp:191
#19 0x1010b3101 in WebFrameLoaderClient::receivedPolicyDecison at WebFrameLoaderClient.mm:1340
#20 0x1010b3196 in -[WebFramePolicyListener receivedPolicyDecision:] at WebFrameLoaderClient.mm:2076
#21 0x1010af500 in -[WebFramePolicyListener use] at WebFrameLoaderClient.mm:2091
#22 0x100039818 in ??
#23 0x7fff84c7696c in __invoking___
#24 0x7fff84c7683d in -[NSInvocation invoke]
#25 0x7fff84c92711 in -[NSInvocation invokeWithTarget:]
#26 0x10115c91b in -[_WebSafeForwarder forwardInvocation:] at WebView.mm:2896
#27 0x7fff84c7398c in ___forwarding___
#28 0x7fff84c6fa68 in __forwarding_prep_0___
#29 0x1010b4af2 in WebFrameLoaderClient::dispatchDecidePolicyForResponse at WebFrameLoaderClient.mm:756
#30 0x1020ca4e6 in WebCore::PolicyChecker::checkContentPolicy at PolicyChecker.cpp:108
#31 0x10200edda in WebCore::MainResourceLoader::didReceiveResponse at MainResourceLoader.cpp:429
#32 0x10200cdb7 in WebCore::MainResourceLoader::handleEmptyLoad at MainResourceLoader.cpp:518
#33 0x10200d5b2 in WebCore::MainResourceLoader::loadNow at MainResourceLoader.cpp:584
#34 0x10200d9f7 in WebCore::MainResourceLoader::load at MainResourceLoader.cpp:613
#35 0x1018e86e2 in WebCore::DocumentLoader::startLoadingMainResource at DocumentLoader.cpp:822
#36 0x101a7592c in WebCore::FrameLoader::continueLoadAfterWillSubmitForm at FrameLoader.cpp:2319
#37 0x101a7db4e in WebCore::FrameLoader::continueLoadAfterNavigationPolicy at FrameLoader.cpp:2851
#38 0x101a7db9a in WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy at FrameLoader.cpp:2724
#39 0x1020c9769 in WebCore::PolicyCallback::call at PolicyCallback.cpp:103
#40 0x1020ca359 in WebCore::PolicyChecker::continueAfterNavigationPolicy at PolicyChecker.cpp:164
#41 0x1010b3101 in WebFrameLoaderClient::receivedPolicyDecison at WebFrameLoaderClient.mm:1340
#42 0x1010b3196 in -[WebFramePolicyListener receivedPolicyDecision:] at WebFrameLoaderClient.mm:2076
#43 0x1010af500 in -[WebFramePolicyListener use] at WebFrameLoaderClient.mm:2091
#44 0x10002c445 in ??
#45 0x7fff84c7696c in __invoking___
#46 0x7fff84c7683d in -[NSInvocation invoke]
#47 0x7fff84c92711 in -[NSInvocation invokeWithTarget:]
#48 0x10115c91b in -[_WebSafeForwarder forwardInvocation:] at WebView.mm:2896
#49 0x7fff84c7398c in ___forwarding___
#50 0x7fff84c6fa68 in __forwarding_prep_0___
#51 0x1010b442e in WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction at WebFrameLoaderClient.mm:778
#52 0x1020ca94e in WebCore::PolicyChecker::checkNavigationPolicy at PolicyChecker.cpp:89
#53 0x101a7e03d in WebCore::FrameLoader::loadWithDocumentLoader at FrameLoader.cpp:1367
#54 0x101a7ee5a in WebCore::FrameLoader::loadWithNavigationAction at FrameLoader.cpp:1279
#55 0x101a8020e in WebCore::FrameLoader::loadURL at FrameLoader.cpp:1222
#56 0x101a80f38 in WebCore::FrameLoader::loadURLIntoChildFrame at FrameLoader.cpp:814
#57 0x1010b2ab0 in WebFrameLoaderClient::createFrame at WebFrameLoaderClient.mm:1433
#58 0x10235a415 in WebCore::SubframeLoader::loadSubframe at SubframeLoader.cpp:265
#59 0x10235a70f in WebCore::SubframeLoader::loadOrRedirectSubframe at SubframeLoader.cpp:240
#60 0x10235b0c3 in WebCore::SubframeLoader::requestFrame at SubframeLoader.cpp:83
#61 0x101b56ba1 in WebCore::HTMLFrameElementBase::openURL at HTMLFrameElementBase.cpp:102
#62 0x101b56d0a in WebCore::HTMLFrameElementBase::setNameAndOpenURL at HTMLFrameElementBase.cpp:153
#63 0x101b56dc4 in WebCore::HTMLFrameElementBase::insertedIntoDocument at HTMLFrameElementBase.cpp:187
#64 0x101b5a3ce in WebCore::HTMLIFrameElement::insertedIntoDocument at HTMLIFrameElement.cpp:150
#65 0x101758102 in WebCore::ContainerNode::parserAddChild at ContainerNode.cpp:690
#66 0x101b1a0cb in WebCore::HTMLConstructionSite::attach<WebCore::Element> at HTMLConstructionSite.cpp:99
#67 0x101b1837a in WebCore::HTMLConstructionSite::attachToCurrent at HTMLConstructionSite.cpp:259
#68 0x101b1869c in WebCore::HTMLConstructionSite::insertHTMLElement at HTMLConstructionSite.cpp:289
#69 0x101ba44cc in WebCore::HTMLTreeBuilder::processGenericRawTextStartTag at HTMLTreeBuilder.cpp:2796
#70 0x101ba8777 in WebCore::HTMLTreeBuilder::processStartTagForInBody at HTMLTreeBuilder.cpp:956
#71 0x101ba9a38 in WebCore::HTMLTreeBuilder::processStartTag at HTMLTreeBuilder.cpp:1230
#72 0x101babb12 in WebCore::HTMLTreeBuilder::processToken at HTMLTreeBuilder.cpp:481
#73 0x101bb078c in WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken at HTMLTreeBuilder.cpp:462
#74 0x101bb0876 in WebCore::HTMLTreeBuilder::constructTreeFromToken at HTMLTreeBuilder.cpp:452
#75 0x101b2fde7 in WebCore::HTMLDocumentParser::pumpTokenizer at HTMLDocumentParser.cpp:276
#76 0x101b30119 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible at HTMLDocumentParser.cpp:175
#77 0x101b301cb in WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution at HTMLDocumentParser.cpp:478
#78 0x101b30512 in WebCore::HTMLDocumentParser::notifyFinished at HTMLDocumentParser.cpp:523
#79 0x1016fba36 in WebCore::CachedResource::didAddClient at CachedResource.cpp:288
#80 0x10170d269 in WebCore::CachedScript::didAddClient at CachedScript.cpp:63
#81 0x1016fcc17 in WebCore::CachedResource::switchClientsToRevalidatedResource at CachedResource.cpp:483
#82 0x10203f9c4 in WebCore::MemoryCache::revalidationSucceeded at MemoryCache.cpp:122
#83 0x10170bad0 in WebCore::CachedResourceRequest::didReceiveResponse at CachedResourceRequest.cpp:215
#84 0x10235bc73 in WebCore::SubresourceLoader::didReceiveResponse at SubresourceLoader.cpp:141
#85 0x10228e209 in WebCore::ResourceLoader::didReceiveResponse at ResourceLoader.cpp:437
#86 0x1022892d3 in -[WebCoreResourceHandleAsDelegate connection:didReceiveResponse:] at ResourceHandleMac.mm:796
#87 0x7fff87340b67 in _NSURLConnectionDidReceiveResponse
#88 0x7fff88ff0ba4 in URLConnectionClient::_clientSendDidReceiveResponse
#89 0x7fff89057a78 in URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload
#90 0x7fff89057c1a in URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload
#91 0x7fff88fde825 in URLConnectionClient::processEvents
#92 0x7fff88fde600 in MultiplexerSource::perform
#93 0x7fff84c3e401 in __CFRunLoopDoSources0
#94 0x7fff84c3c5f9 in __CFRunLoopRun
#95 0x7fff84c3bdbf in CFRunLoopRunSpecific
#96 0x7fff83eb47ee in RunCurrentEventLoopInMode
#97 0x7fff83eb45f3 in ReceiveNextEventCommon
#98 0x7fff83eb44ac in BlockUntilNextEventMatchingListInMode
#99 0x7fff815f8e64 in _DPSNextEvent (100 of 105 frames) (100 of 105 frames) (100 of 105 frames) (100 of 105 frames)
#100 0x7fff815f87a9 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
#101 0x100015ff6 in ??
#102 0x7fff815be48b in -[NSApplication run]
#103 0x7fff815b71a8 in NSApplicationMain
#104 0x100009f18 in ??

[Yi Shen]

Created attachment 98351 [details]
Remove all failed assertions

Hi Alexey, please take a look at this attachment which contains all the assertions need to get removed for my test page ...

[Yi Shen]

Created attachment 98400 [details]
proposal patch without changelog

Will test it on Mac tomorrow

[Alexey Proskuryakov]

Comment on attachment 98400 [details]
proposal patch without changelog

r- for lack of ChangeLog. I don't understand why the fix for a platforms specific issue requires changes to cross-platforms code.

[Yi Shen]

(In reply to comment #13)
> (From update of attachment 98400 [details])
> r- for lack of ChangeLog. I don't understand why the fix for a platforms specific issue requires changes to cross-platforms code.

I try to make Page::setDefersLoading(false) support synchronous content delivery (just like Qt does). Will come a new patch.

[Yi Shen]

Created attachment 98522 [details]
Make setDefersLoading support delivering content syncronously.

[Alexey Proskuryakov]

I still don't understand why a fix for a platforms specific issue requires changes to cross-platforms code.

This is one of the most involved mechanisms in page loading, and making changes to it needs to pass a very high bar of clarity and usefulness.

[Yael]

I think the right solution here is what Benjamin said in comment #6.

You should set deferred to false synchronously, but resume the load asynchronously. And take into account that deferred could have been set to true before the load resumed.

[Yi Shen]

Comment on attachment 98522 [details]
Make setDefersLoading support delivering content syncronously.

Yael & Alexey, thank u both for the comments :) I have been convinced that using asynchronous content delivery is the right way to go. Will update the patch. Thanks!

[Luiz Agostini]

(In reply to comment #9)
> > ap, it seems that you have introduced the assertion in ResourceHandle::setDefersLoading (http://trac.webkit.org/changeset/61768). Do you agree with removing it?
> 
> I don't understand the reason for this question. There doesn't seem to be any mention of this assertion being wrong in the discussion above - and instead, there is a comment saying that even if it's removed, another assertion fires later.

I was considering that if callbacks get called inside wkSetNSURLConnectionDefersCallbacks() then mac would probably hit the same assertion.

[Luiz Agostini]

Now considering that the assertion is right we must make sure that no callback will get called inside our ResourceHandle::platformSetDefersLoading() implementation as well by resuming the load asynchronously. You said that it does not completely solve the problem but I think that we should do it anyway.

I would prefer to keep the changes in Qt specific code as well. We should at least understand why we are hitting the assertions while the other ports are not.

[Yi Shen]

Created attachment 98732 [details]
updated with Yael's comments

[Luiz Agostini]

Comment on attachment 98732 [details]
updated with Yael's comments

LGTM.

[Yi Shen]

(In reply to comment #20)
> Now considering that the assertion is right we must make sure that no callback will get called inside our ResourceHandle::platformSetDefersLoading() implementation as well by resuming the load asynchronously. You said that it does not completely solve the problem but I think that we should do it anyway.
> 
> I would prefer to keep the changes in Qt specific code as well. We should at least understand why we are hitting the assertions while the other ports are not.

Thanks for comments, Luiz. For Mac port, it doesn't resume the load synchronously, so it is not hitting the assertions.

The new patch makes the load asynchronously for Qt, and I didn't see any further assertion failures with the latest code. The problem I mentioned in comment #7 is not reproducible after update my environment. Yael said she also saw a similar issue a few days ago and it disappeared later. I assume it is a separated issue and has been fixed by someone ...

[Yi Shen]

(In reply to comment #16)
> I still don't understand why a fix for a platforms specific issue requires changes to cross-platforms code.
> 
> This is one of the most involved mechanisms in page loading, and making changes to it needs to pass a very high bar of clarity and usefulness.

Alexey, actually, I saw there is a weired behavior on safari when runs my test page. After I click the test button, a iframe gets created and starts loading cnn.com page. The page loading then gets deferred by an JS alert box. But for Safari, I saw sometimes, the cnn page continues loading until finished, even the JS alert box is showing. In other words, it seems the page loading is not suspended by the alert box. This problem can not be reproduced every time, but I did see it happens. Is it a bug for safari? (Sorry, I don't have a Mac, so I didn't dig into this issue.)

[Alexey Proskuryakov]

Yes, this does sound like something worth investigating.

[Benjamin Poulain]

Comment on attachment 98732 [details]
updated with Yael's comments

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

Why don't you explain the problem and why this patch is the right solution to solve it?

> Source/WebCore/ChangeLog:9
> +        Resume the page loading asynchronously.
> +

You don't explain anything about the patch here.

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:153
> +    : QObject(0)

That is unecessary.
And actually not correct, this should be nullptr.

[Yi Shen]

Created attachment 100150 [details]
updated with Benjamin's comments

Also, add an automated test for this issue.

[WebKit Review Bot]

Comment on attachment 100150 [details]
updated with Benjamin's comments

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

New failing tests:
loader/load-defer-resume-crash.html

[WebKit Review Bot]

Created attachment 100151 [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

[Luiz Agostini]

Comment on attachment 100150 [details]
updated with Benjamin's comments

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

Does any of our bots run tests in debug mode? The test will not be effective if they don't.

> LayoutTests/loader/load-defer-resume-crash.html:14
> +        setTimeout("layoutTestController.setDefersLoading(false); if(window.layoutTestController) layoutTestController.notifyDone();",1000);

You are testing for the existence of window.layoutTestController just after using it.

[Yi Shen]

Created attachment 100157 [details]
Skip the new test for chromiun

Skip this test for chromiun which has no LayoutTestController::setDefersLoading implementation.

[Yi Shen]

Created attachment 100299 [details]
updated with Luiz's comments

[Yi Shen]

Created attachment 100333 [details]
updated Changelog & use invoke instead of signal-slot connection

[Benjamin Poulain]

Comment on attachment 100333 [details]
updated Changelog & use invoke instead of signal-slot connection

Great.

[Yi Shen]

(In reply to comment #34)
> (From update of attachment 100333 [details])
> Great.
Thanks :-)

[WebKit Review Bot]

Comment on attachment 100333 [details]
updated Changelog & use invoke instead of signal-slot connection

Rejecting attachment 100333 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 1

Last 500 characters of output:
da8ccab0fd7b9659026565f00b2340646f223e0c
r90771 = ec37526b3fb47a1432abd69d0fa631fedf9fa4bf
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9014293

[Benjamin Poulain]

Created attachment 100343 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 100343 [details]
Patch for landing

Clearing flags on attachment: 100343

Committed r90779: <http://trac.webkit.org/changeset/90779>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yi Shen]

The patch may fail the layout-test on the buildbot?? Need investigation.

[Yi Shen]

Created attachment 100361 [details]
rollout 90779

rollout the change to fix the buildbot

[Yi Shen]

Created attachment 100362 [details]
rollout 90779

[WebKit Review Bot]

Comment on attachment 100362 [details]
rollout 90779

Clearing flags on attachment: 100362

Committed r90784: <http://trac.webkit.org/changeset/90784>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it was rolled out.

[Yi Shen]

Created attachment 100537 [details]
Fix the layout test

If QNetworkReplyHandler is set to synchronous load, it has to deliver content synchronously. Otherwise, it causes problems in some cases. e.g.

void ResourceHandle::loadResourceSynchronously(...) {
    ... ...
    RefPtr<ResourceHandle> handle = adoptRef(new ResourceHandle(request, &syncLoader, true, false));
    ... ...
    d->m_job = new QNetworkReplyHandler(handle.get(), QNetworkReplyHandler::SynchronousLoad, true);
    d->m_job->setLoadingDeferred(false);
}

When exiting above function, the instance of ResourceHandle gets destroyed , which also destroys the QNetworkReplyHandler. In this case, the d->m_job->setLoadingDeferred() has to be done synchronously, otherwise, it will perform on a destroyed instance.

I have run most layout tests successfully on my local machine after fix my local apache2 sever (Which is the reason why I didn't see the test failures on my local env yesterday :$). But still, there is one small issue for my apache2, which blocks some tests related to https. So, I will continue working on the test until I can make sure everything works right.

[Benjamin Poulain]

Damn it, the synchronous load hit us again.

We should split the path to make sure we don't abuse the side effects of ::setLoadingDeferred() in the synchronous case. The synchronous case should explicitly flush the reply handler.

[Luiz Agostini]

(In reply to comment #46)
> Created an attachment (id=100537) [details]
> Fix the layout test
> 
> If QNetworkReplyHandler is set to synchronous load, it has to deliver content synchronously. Otherwise, it causes problems in some cases. e.g.
> 
> void ResourceHandle::loadResourceSynchronously(...) {
>     ... ...
>     RefPtr<ResourceHandle> handle = adoptRef(new ResourceHandle(request, &syncLoader, true, false));
>     ... ...
>     d->m_job = new QNetworkReplyHandler(handle.get(), QNetworkReplyHandler::SynchronousLoad, true);
>     d->m_job->setLoadingDeferred(false);

This was done this way for d->m_job to be assigned before actually sending the request. Otherwise some callbacks could come before d->m_job could get assigned. setLoadingDeferred was abused a bit but the other options were not that better.

As the queue is owned by the QNetworkReplyHandler object an option would be to add a 'synchronous' flag to the class QNetworkReplyHandlerCallQueue and make it resume synchronously when needed.

[Yi Shen]

(In reply to comment #48)
> (In reply to comment #46)
> > Created an attachment (id=100537) [details] [details]
> > Fix the layout test
> > 
> > If QNetworkReplyHandler is set to synchronous load, it has to deliver content synchronously. Otherwise, it causes problems in some cases. e.g.
> > 
> > void ResourceHandle::loadResourceSynchronously(...) {
> >     ... ...
> >     RefPtr<ResourceHandle> handle = adoptRef(new ResourceHandle(request, &syncLoader, true, false));
> >     ... ...
> >     d->m_job = new QNetworkReplyHandler(handle.get(), QNetworkReplyHandler::SynchronousLoad, true);
> >     d->m_job->setLoadingDeferred(false);
> 
> This was done this way for d->m_job to be assigned before actually sending the request. Otherwise some callbacks could come before d->m_job could get assigned. setLoadingDeferred was abused a bit but the other options were not that better.
> 
> As the queue is owned by the QNetworkReplyHandler object an option would be to add a 'synchronous' flag to the class QNetworkReplyHandlerCallQueue and make it resume synchronously when needed.

Luiz, do u agree with Benjamin that we should explicitly flush the reply handler in the synchronous case?

The latest patch I made is using QNetworkReplyHandler's 'synchronous' flag to determine whether it should resume synchronously. And I have run the layout test on it and seems it works fine.

[Yi Shen]

(In reply to comment #47)
> Damn it, the synchronous load hit us again.
> 
> We should split the path to make sure we don't abuse the side effects of ::setLoadingDeferred() in the synchronous case. The synchronous case should explicitly flush the reply handler.

Indeed :) It makes code clear to explicitly flush the reply handler in this case. If we go this way, we need to add a new interface for QNetworkReplyHandler, like QNetworkReplyHandler::flush(), right?

[Luiz Agostini]

(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #46)
> > > Created an attachment (id=100537) [details] [details] [details]
> > > Fix the layout test
> > > 
> > > If QNetworkReplyHandler is set to synchronous load, it has to deliver content synchronously. Otherwise, it causes problems in some cases. e.g.
> > > 
> > > void ResourceHandle::loadResourceSynchronously(...) {
> > >     ... ...
> > >     RefPtr<ResourceHandle> handle = adoptRef(new ResourceHandle(request, &syncLoader, true, false));
> > >     ... ...
> > >     d->m_job = new QNetworkReplyHandler(handle.get(), QNetworkReplyHandler::SynchronousLoad, true);
> > >     d->m_job->setLoadingDeferred(false);
> > 
> > This was done this way for d->m_job to be assigned before actually sending the request. Otherwise some callbacks could come before d->m_job could get assigned. setLoadingDeferred was abused a bit but the other options were not that better.
> > 
> > As the queue is owned by the QNetworkReplyHandler object an option would be to add a 'synchronous' flag to the class QNetworkReplyHandlerCallQueue and make it resume synchronously when needed.
> 
> Luiz, do u agree with Benjamin that we should explicitly flush the reply handler in the synchronous case?

Sure. I do. But I am not sure about what would be the API.

I think that we could have different classes for the synchronous and asynchronous cases. Then the asynchronous one could have setLoadingDeferred and the synchronous could have its own different interface. I was thinking about it at the time that I was working in this code, but I am not sure if it's worth.

I think that we could solve this problem here without changes to the interfaces and then open new bugs for further refactorings.

> 
> The latest patch I made is using QNetworkReplyHandler's 'synchronous' flag to determine whether it should resume synchronously. And I have run the layout test on it and seems it works fine.

[Yi Shen]

(In reply to comment #51)
> (In reply to comment #49)
> > (In reply to comment #48)
> > > (In reply to comment #46)
> > > > Created an attachment (id=100537) [details] [details] [details] [details]
> > > > Fix the layout test
> > > > 
> > > > If QNetworkReplyHandler is set to synchronous load, it has to deliver content synchronously. Otherwise, it causes problems in some cases. e.g.
> > > > 
> > > > void ResourceHandle::loadResourceSynchronously(...) {
> > > >     ... ...
> > > >     RefPtr<ResourceHandle> handle = adoptRef(new ResourceHandle(request, &syncLoader, true, false));
> > > >     ... ...
> > > >     d->m_job = new QNetworkReplyHandler(handle.get(), QNetworkReplyHandler::SynchronousLoad, true);
> > > >     d->m_job->setLoadingDeferred(false);
> > > 
> > > This was done this way for d->m_job to be assigned before actually sending the request. Otherwise some callbacks could come before d->m_job could get assigned. setLoadingDeferred was abused a bit but the other options were not that better.
> > > 
> > > As the queue is owned by the QNetworkReplyHandler object an option would be to add a 'synchronous' flag to the class QNetworkReplyHandlerCallQueue and make it resume synchronously when needed.
> > 
> > Luiz, do u agree with Benjamin that we should explicitly flush the reply handler in the synchronous case?
> 
> Sure. I do. But I am not sure about what would be the API.
> 
> I think that we could have different classes for the synchronous and asynchronous cases. Then the asynchronous one could have setLoadingDeferred and the synchronous could have its own different interface. I was thinking about it at the time that I was working in this code, but I am not sure if it's worth.
> 
> I think that we could solve this problem here without changes to the interfaces and then open new bugs for further refactorings.
> 
> > 
> > The latest patch I made is using QNetworkReplyHandler's 'synchronous' flag to determine whether it should resume synchronously. And I have run the layout test on it and seems it works fine.

Thanks Luiz, let's see what Benjamin thinks. Also, what do u think my patch, is it the same solution as you proposed?

[Benjamin Poulain]

Comment on attachment 100537 [details]
Fix the layout test

Moving to review queue. The new patch looks good to me, but so was the previous one, so I would like a second opinion.

[Luiz Agostini]

Comment on attachment 100537 [details]
Fix the layout test

LGTM

[Benjamin Poulain]

Comment on attachment 100537 [details]
Fix the layout test

Let's go for it then :)
Yi, I don't cq+ now. You can land it when you have time to check everything goes well so Ossy does not have to yell at us :)

[Yi Shen]

Comment on attachment 100537 [details]
Fix the layout test

Thanks benjamin's review. Change it to cq+ and will keep my eyes on the buildbot.

[WebKit Review Bot]

Comment on attachment 100537 [details]
Fix the layout test

Clearing flags on attachment: 100537

Committed r91189: <http://trac.webkit.org/changeset/91189>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.