Bug 80165 - [Qt][WK2] Crash in Frame.cpp when loading index.hu
Summary: [Qt][WK2] Crash in Frame.cpp when loading index.hu
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Major
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on: 80294
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-02 09:05 PST by Andras Becsi
Modified: 2012-03-06 09:58 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Andras Becsi 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.
Comment 1 Alexey Proskuryakov 2012-03-02 13:04:46 PST
I cannot reproduce this in Safari (ToT WebKit). What does Qt do differently here?
Comment 2 Andras Becsi 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.
Comment 3 Allan Sandfeld Jensen 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).
Comment 4 zalan 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?
Comment 5 Allan Sandfeld Jensen 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.
Comment 6 zalan 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.
Comment 7 Allan Sandfeld Jensen 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.
Comment 8 zalan 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)
Comment 9 Allan Sandfeld Jensen 2012-03-05 09:03:43 PST
Created attachment 130148 [details]
Patch
Comment 10 zalan 2012-03-05 09:07:47 PST
lgtm
Comment 11 Allan Sandfeld Jensen 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.
Comment 12 Alexey Proskuryakov 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.
Comment 13 Antonio Gomes 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.
Comment 14 Alexis Menard (darktears) 2012-03-05 09:53:09 PST
Comment on attachment 130148 [details]
Patch

Possible to write a test case for it?
Comment 15 Allan Sandfeld Jensen 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.
Comment 16 Kenneth Rohde Christiansen 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?
Comment 17 Alexey Proskuryakov 2012-03-05 14:08:48 PST
The preferred style is to use a member variable directly, not an accessor function.
Comment 18 Kenneth Rohde Christiansen 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.
Comment 19 Allan Sandfeld Jensen 2012-03-06 07:02:41 PST
Created attachment 130374 [details]
Patch
Comment 20 WebKit Review Bot 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>
Comment 21 WebKit Review Bot 2012-03-06 09:58:06 PST
All reviewed patches have been landed.  Closing bug.