Bug 80165

Summary: [Qt][WK2] Crash in Frame.cpp when loading index.hu
Product: WebKit Reporter: Andras Becsi <abecsi>
Component: WebCore Misc.Assignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Major CC: allan.jensen, ap, hausmann, jturcotte, kenneth, manyoso, tonikitoo, webkit.review.bot, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Linux   
Bug Depends on: 80294    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Andras Becsi
Reported 2012-03-02 09:05:38 PST
When loading the news site index.hu in Qt MiniBrowser the suspending code introduced in r109548 tries to call a methond on a null pointer in Source/WebCore/page/Frame.cpp:318. 0x00007ffff5042184 in WebCore::Frame::setDocument (this=0x1c0fd90, newDoc=...) at ../../../../Source/WebCore/page/Frame.cpp:318 318 document()->suspendScriptedAnimationControllerCallbacks(); (gdb) bt #0 0x00007ffff5042184 in WebCore::Frame::setDocument (this=0x1c0fd90, newDoc=...) at ../../../../Source/WebCore/page/Frame.cpp:318 #1 0x00007ffff4f8919c in WebCore::FrameLoader::clear (this=0x1c0fe50, clearWindowProperties=true, clearScriptObjects=true, clearFrameView=true) at ../../../../Source/WebCore/loader/FrameLoader.cpp:548 #2 0x00007ffff4f82d8b in WebCore::DocumentWriter::begin (this=0x1c17050, urlReference=..., dispatch=false, ownerDocument=0x0) at ../../../../Source/WebCore/loader/DocumentWriter.cpp:128 #3 0x00007ffff4f89329 in WebCore::FrameLoader::receivedFirstData (this=0x1c0fe50) at ../../../../Source/WebCore/loader/FrameLoader.cpp:576 #4 0x00007ffff4f8b1ae in WebCore::FrameLoader::willSetEncoding (this=0x1c0fe50) at ../../../../Source/WebCore/loader/FrameLoader.cpp:989 #5 0x00007ffff4f8376c in WebCore::DocumentWriter::setEncoding (this=0x1c17050, name=..., userChosen=false) at ../../../../Source/WebCore/loader/DocumentWriter.cpp:239 #6 0x00007ffff4f774c1 in WebCore::DocumentLoader::commitData (this=0x1c16f30, bytes=0x1580a78 "<html><head><title>Edigital :: </title><meta http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\">\n<!--[if lt IE 7]>\n<script language=\"JavaScript\">\nfunction correctPNG() // correctly handle PN"..., length=4639) at ../../../../Source/WebCore/loader/DocumentLoader.cpp:325 #7 0x00007ffff47ecaf1 in WebKit::WebFrameLoaderClient::committedLoad (this=0x1c0f060, loader=0x1c16f30, data=0x1580a78 "<html><head><title>Edigital :: </title><meta http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\">\n<!--[if lt IE 7]>\n<script language=\"JavaScript\">\nfunction correctPNG() // correctly handle PN"..., length=4639) at ../../../../Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:839 #8 0x00007ffff4f773e0 in WebCore::DocumentLoader::commitLoad (this=0x1c16f30, data=0x1580a78 "<html><head><title>Edigital :: </title><meta http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\">\n<!--[if lt IE 7]>\n<script language=\"JavaScript\">\nfunction correctPNG() // correctly handle PN"..., length=4639) at ../../../../Source/WebCore/loader/DocumentLoader.cpp:313 #9 0x00007ffff4f77612 in WebCore::DocumentLoader::receivedData (this=0x1c16f30, data=0x1580a78 "<html><head><title>Edigital :: </title><meta http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\">\n<!--[if lt IE 7]>\n<script language=\"JavaScript\">\nfunction correctPNG() // correctly handle PN"..., length=4639) at ../../../../Source/WebCore/loader/DocumentLoader.cpp:339 #10 0x00007ffff4fab7eb in WebCore::MainResourceLoader::addData (this=0x1c218a0, data=0x1580a78 "<html><head><title>Edigital :: </title><meta http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\">\n<!--[if lt IE 7]>\n<script language=\"JavaScript\">\nfunction correctPNG() // correctly handle PN"..., length=4639, allAtOnce=false) at ../../../../Source/WebCore/loader/MainResourceLoader.cpp:170 #11 0x00007ffff4fb9544 in WebCore::ResourceLoader::didReceiveData (this=0x1c218a0, data=0x1580a78 "<html><head><title>Edigital :: </title><meta http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\">\n<!--[if lt IE 7]>\n<script language=\"JavaScript\">\nfunction correctPNG() // correctly handle PN"..., length=4639, encodedDataLength=-1, allAtOnce=false) at ../../../../Source/WebCore/loader/ResourceLoader.cpp:287 #12 0x00007ffff4fad03d in WebCore::MainResourceLoader::didReceiveData (this=0x1c218a0, data=0x1580a78 "<html><head><title>Edigital :: </title><meta http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\">\n<!--[if lt IE 7]>\n<script language=\"JavaScript\">\nfunction correctPNG() // correctly handle PN"..., length=4639, encodedDataLength=-1, allAtOnce=false) at ../../../../Source/WebCore/loader/MainResourceLoader.cpp:464 #13 0x00007ffff4fb9ecd in WebCore::ResourceLoader::didReceiveData (this=0x1c218a0, data=0x1580a78 "<html><head><title>Edigital :: </title><meta http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\">\n<!--[if lt IE 7]>\n<script language=\"JavaScript\">\nfunction correctPNG() // correctly handle PN"..., length=4639, encodedDataLength=-1) at ../../../../Source/WebCore/loader/ResourceLoader.cpp:441 #14 0x00007ffff53c487e in WebCore::QNetworkReplyHandler::forwardData (this=0x1c203e0) at ../../../../Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:569 #15 0x00007ffff53c1b93 in WebCore::QNetworkReplyHandlerCallQueue::flush (this=0x1c20418) at ../../../../Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:196 ... Null-checking document() before suspending naturally fixes the above issue but the same page ASSERTs in Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:74 ASSERT(!suspended()) as soon as you start interacting with it.
Attachments
Patch (1.53 KB, patch)
2012-03-05 09:03 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (1.71 KB, patch)
2012-03-06 07:02 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2012-03-02 13:04:46 PST
I cannot reproduce this in Safari (ToT WebKit). What does Qt do differently here?
Andras Becsi
Comment 2 2012-03-05 03:59:34 PST
(In reply to comment #1) > I cannot reproduce this in Safari (ToT WebKit). What does Qt do differently here? I still see the crash in the web process with ToT WebKit and Qt MiniBrowser, so I'll mark this as a Qt specific bug. Judging from the stacktrace the bug could originate in the Qt networking stack. CCing people who might know what's going on here.
Allan Sandfeld Jensen
Comment 3 2012-03-05 06:28:33 PST
This is apparently two different issues. The first one is a missing guard for m_doc in Frame::setDocument. The problem in XMLHttpRequest is deeper. The problem is the threaded loader can not be suspended, and this means progress events can still be emitted while the XMLHttpRequest object is suspended. XMLHttpRequest will set canSuspend() to false in this case, but that does not guarantee that it will not be asked to suspend anyway in some cases (for pageCache for instance).
alan
Comment 4 2012-03-05 06:48:45 PST
so how did we end up with suspended frames while loading? Is it normal to see suspendActiveDOMObjectsAndAnimations() calls at this phase of constructing a page?
Allan Sandfeld Jensen
Comment 5 2012-03-05 07:09:53 PST
(In reply to comment #4) > so how did we end up with suspended frames while loading? Is it normal to see suspendActiveDOMObjectsAndAnimations() calls at this phase of constructing a page? No, but if the page is loading continuously via xmlhttprequest, then it is inevitable we will end up with a suspended frame during a pan while a xmlhttprequest is running.
alan
Comment 6 2012-03-05 07:21:25 PST
(In reply to comment #5) > (In reply to comment #4) > > so how did we end up with suspended frames while loading? Is it normal to see suspendActiveDOMObjectsAndAnimations() calls at this phase of constructing a page? > > No, but if the page is loading continuously via xmlhttprequest, then it is inevitable we will end up with a suspended frame during a pan while a xmlhttprequest is running. I can repro crash without any page interaction (panning, zooming) by just simply loading the page.
Allan Sandfeld Jensen
Comment 7 2012-03-05 07:53:37 PST
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > so how did we end up with suspended frames while loading? Is it normal to see suspendActiveDOMObjectsAndAnimations() calls at this phase of constructing a page? > > > > No, but if the page is loading continuously via xmlhttprequest, then it is inevitable we will end up with a suspended frame during a pan while a xmlhttprequest is running. > > I can repro crash without any page interaction (panning, zooming) by just simply loading the page. I meant the second assertion we are seeing after guarding m_doc is triggered by page interaction. You are right though, the first one crash is actually a bit odd. It is a unset document call so probably during a redirection or reload of something in the page, but there still shouldn't be any calls suspending active-dom-objects before the users starts interacting with the page. It could be that flickable is interpreting some reported state-changes from the WebProcess as a user-input and starts animating them.
alan
Comment 8 2012-03-05 08:05:25 PST
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > (In reply to comment #4) > > > > so how did we end up with suspended frames while loading? Is it normal to see suspendActiveDOMObjectsAndAnimations() calls at this phase of constructing a page? > > > > > > No, but if the page is loading continuously via xmlhttprequest, then it is inevitable we will end up with a suspended frame during a pan while a xmlhttprequest is running. > > > > I can repro crash without any page interaction (panning, zooming) by just simply loading the page. > > I meant the second assertion we are seeing after guarding m_doc is triggered by page interaction. > > You are right though, the first one crash is actually a bit odd. It is a unset document call so probably during a redirection or reload of something in the page, but there still shouldn't be any calls suspending active-dom-objects before the users starts interacting with the page. > > It could be that flickable is interpreting some reported state-changes from the WebProcess as a user-input and starts animating them. Yea, could very well be. Before the if (document()) check is added (which seems to be checked everywhere in setDocument, so having null m_doc looks like a valid state), we should see why the suspend is triggered in this case. Having DOM objects suspended while loading (and resumed at some point) could potentially make complete load time longer. (so i guess in that sense, it's good that we've got this crash, otherwise it would have gone unnoticed)
Allan Sandfeld Jensen
Comment 9 2012-03-05 09:03:43 PST
Created attachment 130148 [details] Patch
alan
Comment 10 2012-03-05 09:07:47 PST
lgtm
Allan Sandfeld Jensen
Comment 11 2012-03-05 09:09:29 PST
Finally for the problem in XMLHttpRequestProgressEventThrottle.cpp, something like the following solves the problem: void XMLHttpRequestProgressEventThrottle::dispatchEvent(PassRefPtr<Event> .. { - ASSERT(!suspended()); + if (suspended()) { + // Discard the current stored event and only store the latest. + m_pausedEvent = event; + return; + } The problem though is that this still loses progress events that the xmlhttprequest can emit while suspended. I can imagine cases where that might be a problem, though all regression-tests complete successfully.
Alexey Proskuryakov
Comment 12 2012-03-05 09:34:54 PST
So the explanation of why this doesn't crash on Mac is that this is new Qt-only code, added in r109548. Please use a separate bug for the XMLHttpRequest issue. It seems possible that the concept of "suspend" introduced in r109548 is just not appropriate for WebKit though.
Antonio Gomes
Comment 13 2012-03-05 09:41:53 PST
(In reply to comment #12) > So the explanation of why this doesn't crash on Mac is that this is new Qt-only code, added in r109548. > > Please use a separate bug for the XMLHttpRequest issue. It seems possible that the concept of "suspend" introduced in r109548 is just not appropriate for WebKit though. Yes. We've ran into this before, and advised in the original bug.
Alexis Menard (darktears)
Comment 14 2012-03-05 09:53:09 PST
Comment on attachment 130148 [details] Patch Possible to write a test case for it?
Allan Sandfeld Jensen
Comment 15 2012-03-05 13:12:01 PST
(In reply to comment #13) > (In reply to comment #12) > > So the explanation of why this doesn't crash on Mac is that this is new Qt-only code, added in r109548. > > > > Please use a separate bug for the XMLHttpRequest issue. It seems possible that the concept of "suspend" introduced in r109548 is just not appropriate for WebKit though. > > Yes. > > We've ran into this before, and advised in the original bug. That makes no sense. The bugs encountered here are independent of the new "type" of suspend, and existed with the existing suspend code. It was just harder to reproduce before and not triggered as often. So if nothing else this is exposing a range of tricky suspend bugs so they can be more easily fixed.
Kenneth Rohde Christiansen
Comment 16 2012-03-05 14:00:12 PST
Comment on attachment 130148 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130148&action=review > Source/WebCore/page/Frame.cpp:-320 > + m_doc->suspendScriptedAnimationControllerCallbacks(); > animation()->suspendAnimationsForDocument(document()); > - document()->suspendActiveDOMObjects(ActiveDOMObject::PageWillBeSuspended); One line uses document() (the line unchanged) others use m_doc, isn't it inline anyway?
Alexey Proskuryakov
Comment 17 2012-03-05 14:08:48 PST
The preferred style is to use a member variable directly, not an accessor function.
Kenneth Rohde Christiansen
Comment 18 2012-03-05 14:49:25 PST
(In reply to comment #17) > The preferred style is to use a member variable directly, not an accessor function. OK, good to know. Then we should change it for the animation()->suspendAnimationsForDocument(document()); line as well.
Allan Sandfeld Jensen
Comment 19 2012-03-06 07:02:41 PST
Created attachment 130374 [details] Patch
WebKit Review Bot
Comment 20 2012-03-06 09:58:01 PST
Comment on attachment 130374 [details] Patch Clearing flags on attachment: 130374 Committed r109931: <http://trac.webkit.org/changeset/109931>
WebKit Review Bot
Comment 21 2012-03-06 09:58:06 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.