80165 – [Qt][WK2] Crash in Frame.cpp when loading index.hu

RESOLVED FIXED 80165

[Qt][WK2] Crash in Frame.cpp when loading index.hu

https://bugs.webkit.org/show_bug.cgi?id=80165

Summary [Qt][WK2] Crash in Frame.cpp when loading index.hu

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	Details Formatted Diff Diff
Patch (1.71 KB, patch) 2012-03-06 07:02 PST, Allan Sandfeld Jensen	no flags	Details Formatted Diff Diff
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).

zalan

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.

zalan

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.

zalan

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

zalan

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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Major

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS Linux

Product WebKit

Component WebCore Misc.

Assignee

Allan Sandfeld Jensen

Reported

2012-03-02 09:05 PST

Modified

2012-03-06 09:58 PST History

CC List

9 users Show

URL

Keywords

Depends on

80294

Blocks

Dependencies

tree graph